No longer disables irqs when mucking with pcpui
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 15 Oct 2012 21:16:51 +0000 (14:16 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 16 Oct 2012 21:42:20 +0000 (14:42 -0700)
For things like cur_proc, owning_{proc,vcoreid}, and cur_tf, we don't
disable interrupts when mucking with them in proc code.  We used to need
this when proc mgmt KMSGs were immediate.  So long as we continue to not
muck with that state from IRQ handlers or IMMED KMSGs, we're okay.

kern/arch/i686/trap.c
kern/src/process.c

index 732d6fc..e9e8f08 100644 (file)
@@ -327,7 +327,7 @@ static void abort_halt(struct trapframe *tf)
 void trap(struct trapframe *tf)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       /* Copy out the TF for now, set tf to point to it.  */
+       /* Copy out the TF for now */
        if (!in_kernel(tf))
                set_current_tf(pcpui, tf);
 
@@ -427,7 +427,7 @@ static void send_eoi(uint32_t trap_nr)
 void irq_handler(struct trapframe *tf)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       /* Copy out the TF for now, set tf to point to it. */
+       /* Copy out the TF for now */
        if (!in_kernel(tf))
                set_current_tf(pcpui, tf);
        /* Coupled with cpu_halt() and smp_idle() */
@@ -506,10 +506,12 @@ void sysenter_init(void)
 void sysenter_callwrapper(struct trapframe *tf)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       /* Copy out the TF for now, set tf to point to it. */
+       /* Copy out the TF for now */
        if (!in_kernel(tf))
                set_current_tf(pcpui, tf);
-       /* Once we've set_current_tf, we can enable interrupts */
+       /* Once we've set_current_tf, we can enable interrupts.  This used to be
+        * mandatory (we had immediate KMSGs that would muck with cur_tf).  Now it
+        * should only help for sanity/debugging. */
        enable_irq();
 
        if (in_kernel(tf))
index d7c108c..a972b7f 100644 (file)
@@ -438,7 +438,6 @@ static bool scp_is_vcctx_ready(struct preempt_data *vcpd)
  * documentation talks about this a bit). */
 void proc_run_s(struct proc *p)
 {
-       int8_t state = 0;
        uint32_t coreid = core_id();
        struct per_cpu_info *pcpui = &per_cpu_info[coreid];
        struct preempt_data *vcpd = &p->procdata->vcore_preempt_data[0];
@@ -463,10 +462,7 @@ void proc_run_s(struct proc *p)
                        __seq_end_write(&p->procinfo->coremap_seqctr);
                        /* incref, since we're saving a reference in owning proc later */
                        proc_incref(p, 1);
-                       /* disable interrupts to protect cur_tf, owning_proc, and current */
-                       disable_irqsave(&state);
-                       /* wait til ints are disabled before unlocking, in case someone else
-                        * grabs the lock and IPIs us before we get set up in cur_tf */
+                       /* lock was protecting the state and VC mapping, not pcpui stuff */
                        spin_unlock(&p->proc_lock);
                        /* redundant with proc_startcore, might be able to remove that one*/
                        __set_proc_current(p);
@@ -499,7 +495,6 @@ void proc_run_s(struct proc *p)
                                /* this is one of the few times cur_tf != &actual_tf */
                                pcpui->cur_tf = &p->env_tf;
                        }
-                       enable_irqsave(&state);
                        /* When the calling core idles, it'll call restartcore and run the
                         * _S process's context. */
                        return;
@@ -760,7 +755,6 @@ void proc_destroy(struct proc *p)
 int proc_change_to_m(struct proc *p)
 {
        int retval = 0;
-       int8_t state = 0;
        spin_lock(&p->proc_lock);
        /* in case userspace erroneously tries to change more than once */
        if (__proc_is_mcp(p))
@@ -777,15 +771,11 @@ int proc_change_to_m(struct proc *p)
                         * it in the preempt slot so that we can also save the silly
                         * state. */
                        struct preempt_data *vcpd = &p->procdata->vcore_preempt_data[0];
-                       disable_irqsave(&state);        /* protect cur_tf */
-                       /* Note this won't play well with concurrent proc kmsgs, but
-                        * since we're _S and locked, we shouldn't have any. */
                        assert(current_tf);
                        /* Copy uthread0's context to the notif slot */
                        vcpd->notif_tf = *current_tf;
                        clear_owning_proc(core_id());   /* so we don't restart */
                        save_fp_state(&vcpd->preempt_anc);
-                       enable_irqsave(&state);
                        /* Userspace needs to not fuck with notif_disabled before
                         * transitioning to _M. */
                        if (vcpd->notif_disabled) {
@@ -833,16 +823,13 @@ error_out:
  * by the proc. */
 uint32_t __proc_change_to_s(struct proc *p, uint32_t *pc_arr)
 {
-       int8_t state = 0;
        uint32_t num_revoked;
        printk("[kernel] trying to transition _M -> _S (deprecated)!\n");
        assert(p->state == PROC_RUNNING_M); // TODO: (ACR) async core req
        /* save the context, to be restarted in _S mode */
-       disable_irqsave(&state);        /* protect cur_tf */
        assert(current_tf);
        p->env_tf = *current_tf;
        clear_owning_proc(core_id());   /* so we don't restart */
-       enable_irqsave(&state);
        env_push_ancillary_state(p); // TODO: (HSS)
        /* sending death, since it's not our job to save contexts or anything in
         * this case. */
@@ -920,20 +907,10 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
        struct per_cpu_info *pcpui = &per_cpu_info[pcoreid];
        struct vcore *vc;
        struct preempt_data *vcpd;
-       int8_t state = 0;
-       /* Need to lock before checking the vcoremap to find out who we are, in case
-        * we're getting __preempted and __startcored, from a remote core (in which
-        * case we might have come in thinking we were vcore X, but had X preempted
-        * and Y restarted on this pcore, and we suddenly are the wrong vcore
-        * yielding).  Arguably, this is incredibly rare, since you'd need to
-        * preempt the core, then decide to give it back with another grant in
-        * between. */
+       /* Need to lock to prevent concurrent vcore changes (online, inactive, the
+        * mapping, etc).  This plus checking the nr_preempts is enough to tell if
+        * our vcoreid and cur_tf ought to be here still or if we should abort */
        spin_lock(&p->proc_lock); /* horrible scalability.  =( */
-       /* Need to disable before even reading vcoreid, since we could be unmapped
-        * by a __preempt or __death.  _S also needs ints disabled, so we'll just do
-        * it immediately.  Finally, we need to disable AFTER locking.  It is a bug
-        * to grab the lock while irq's are disabled. */
-       disable_irqsave(&state);
        switch (p->state) {
                case (PROC_RUNNING_S):
                        if (!being_nice) {
@@ -966,13 +943,13 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                                 * hasn't already written notif_pending will have seen WAITING,
                                 * and will be spinning while we do this. */
                                __proc_save_context_s(p, current_tf);
-                               spin_unlock(&p->proc_lock);     /* note irqs are not enabled yet */
+                               spin_unlock(&p->proc_lock);
                        } else {
                                /* yielding to allow other processes to run.  we're briefly
                                 * WAITING, til we are woken up */
                                __proc_set_state(p, PROC_WAITING);
                                __proc_save_context_s(p, current_tf);
-                               spin_unlock(&p->proc_lock);     /* note irqs are not enabled yet */
+                               spin_unlock(&p->proc_lock);
                                /* immediately wake up the proc (makes it runnable) */
                                proc_wakeup(p);
                        }
@@ -1064,15 +1041,13 @@ out_failed:
        /* for some reason we just want to return, either to take a KMSG that cleans
         * us up, or because we shouldn't yield (ex: notif_pending). */
        spin_unlock(&p->proc_lock);
-       enable_irqsave(&state);
        return;
 out_yield_core:                                /* successfully yielded the core */
        proc_decref(p);                 /* need to eat the ref passed in */
-       /* Clean up the core and idle.  Need to do this before enabling interrupts,
-        * since once we call __sched_put_idle_core(), we could get a startcore. */
+       /* Clean up the core and idle. */
        clear_owning_proc(pcoreid);     /* so we don't restart */
        abandon_core();
-       smp_idle();                             /* will reenable interrupts */
+       smp_idle();
 }
 
 /* Sends a notification (aka active notification, aka IPI) to p's vcore.  We
@@ -1303,7 +1278,13 @@ void proc_give(struct proc *p, uint32_t pcoreid)
  * out). */
 uint32_t proc_get_vcoreid(struct proc *p)
 {
-       return per_cpu_info[core_id()].owning_vcoreid;
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       if (pcpui->owning_proc == p) {
+               return pcpui->owning_vcoreid;
+       } else {
+               warn("Asked for vcoreid for %p, but %p is pwns", p, pcpui->owning_proc);
+               return (uint32_t)-1;
+       }
 }
 
 /* TODO: make all of these static inlines when we gut the env crap */
@@ -1556,14 +1537,12 @@ void __unmap_vcore(struct proc *p, uint32_t vcoreid)
 
 /* Stop running whatever context is on this core and load a known-good cr3.
  * Note this leaves no trace of what was running. This "leaves the process's
- * context.  Also, we want interrupts disabled, to not conflict with kmsgs
- * (__launch_kthread, proc mgmt, etc).
+ * context.
  *
  * This does not clear the owning proc.  Use the other helper for that. */
 void abandon_core(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       assert(!irq_is_enabled());
        /* Syscalls that don't return will ultimately call abadon_core(), so we need
         * to make sure we don't think we are still working on a syscall. */
        pcpui->cur_sysc = 0;
@@ -1577,7 +1556,6 @@ void clear_owning_proc(uint32_t coreid)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[coreid];
        struct proc *p = pcpui->owning_proc;
-       assert(!irq_is_enabled());
        pcpui->owning_proc = 0;
        pcpui->owning_vcoreid = 0xdeadbeef;
        pcpui->cur_tf = 0;                      /* catch bugs for now (will go away soon) */
@@ -1595,15 +1573,12 @@ struct proc *switch_to(struct proc *new_p)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        struct proc *old_proc;
-       int8_t irq_state = 0;
-       disable_irqsave(&irq_state);
        old_proc = pcpui->cur_proc;                                     /* uncounted ref */
        /* If we aren't the proc already, then switch to it */
        if (old_proc != new_p) {
                pcpui->cur_proc = new_p;                                /* uncounted ref */
                lcr3(new_p->env_cr3);
        }
-       enable_irqsave(&irq_state);
        return old_proc;
 }
 
@@ -1612,15 +1587,12 @@ struct proc *switch_to(struct proc *new_p)
 void switch_back(struct proc *new_p, struct proc *old_proc)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       int8_t irq_state = 0;
        if (old_proc != new_p) {
-               disable_irqsave(&irq_state);
                pcpui->cur_proc = old_proc;
                if (old_proc)
                        lcr3(old_proc->env_cr3);
                else
                        lcr3(boot_cr3);
-               enable_irqsave(&irq_state);
        }
 }
 
@@ -1735,18 +1707,13 @@ int proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        struct preempt_data *caller_vcpd;
        struct vcore *caller_vc, *new_vc;
        struct event_msg preempt_msg = {0};
-       int8_t state = 0;
        int retval = -EAGAIN;   /* by default, try again */
        /* Need to not reach outside the vcoremap, which might be smaller in the
         * future, but should always be as big as max_vcores */
        if (new_vcoreid >= p->procinfo->max_vcores)
                return -EINVAL;
-       /* Need to lock before reading the vcoremap, like in yield.  Need to do this
-        * before disabling irqs (deadlock with incoming proc mgmt kmsgs) */
+       /* Need to lock to prevent concurrent vcore changes, like in yield. */
        spin_lock(&p->proc_lock);
-       /* Need to disable before even reading caller_vcoreid, since we could be
-        * unmapped by a __preempt or __death, like in yield. */
-       disable_irqsave(&state);
        /* new_vcoreid is already runing, abort */
        if (vcore_is_mapped(p, new_vcoreid)) {
                retval = -EBUSY;
@@ -1843,7 +1810,6 @@ int proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        /* Fall through to exit */
 out_locked:
        spin_unlock(&p->proc_lock);
-       enable_irqsave(&state);
        return retval;
 }