Sorted some races with proc IPIs, poorly.
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 20 Aug 2009 22:13:25 +0000 (15:13 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 31 Aug 2009 21:07:54 +0000 (14:07 -0700)
In general, sending IPIs and having separate IPIs for startcore, death,
and preempt just sucks on x86.  Next commit will replace all the TODO:
(AM) with some x86 active messaging support, so we can be guaranteed
in-order receipt of messages sent in-order.

Also, here's the deal with the vcoremap:
- the vcoremap is the list of cores allocated to a process,  regardless
  of whether or not it is running there yet (or still).
- you can only mess with a process's vcoremap when you hold its lock.
- you can only mess with a core (send it a proc_mgmt IPI/AM) when you hold
  the lock of the process that has its vcoremapping.

kern/arch/i386/process.c
kern/arch/i386/trap.h
kern/include/smp.h
kern/src/env.c
kern/src/manager.c
kern/src/process.c
user/apps/roslib/mhello.c

index 73ad437..e76981c 100644 (file)
@@ -15,6 +15,11 @@ void __startcore(trapframe_t *tf)
        struct proc *p_to_run = per_cpu_info[coreid].p_to_run;
        trapframe_t local_tf, *tf_to_pop = per_cpu_info[coreid].tf_to_pop;
 
+       /* Once we copied in the message and are in the handler, we can allow future
+        * proc_management IPIs to this core. */
+       // TODO: replace this ghetto with an active message (AM)
+       per_cpu_info[coreid].proc_ipi_pending = 0;
+
        /* EOI - we received the interrupt.  probably no issues with receiving
         * further interrupts in this function.  need to do this before
         * proc_startcore.  incidentally, interrupts are off in this handler
@@ -36,7 +41,9 @@ void __startcore(trapframe_t *tf)
 }
 
 /* Interrupt handler to stop running whatever context is on this core and to
- * idle.  Note this leaves no trace of what was running. */
+ * idle.  Note this leaves no trace of what was running.
+ * It's okay if death comes to a core that's already idling and has no current.
+ * It could happen if a process decref'd before proc_startcore could incref. */
 void __death(trapframe_t *tf)
 {
        /* If we are currently running an address space on our core, we need a known
@@ -48,8 +55,10 @@ void __death(trapframe_t *tf)
                lcr3(boot_cr3);
                proc_decref(current);
                current = NULL;
-       } else {
-               warn("Sent a DEATH IPI to a core with no current process!");
        }
+       // As above, signal that it's okay to send us a proc mgmt IPI
+       // TODO: replace this ghetto with an active message (AM)
+       per_cpu_info[core_id()].proc_ipi_pending = 0;
+       lapic_send_eoi();
        smp_idle();             
 }
index 0d1d81d..a04ebee 100644 (file)
@@ -35,8 +35,9 @@
 
 /* IPIs */
 /* Direct/Hardwired IPIs.  Hardwired in trapentry.S
- * DEATH must come before (have lower priority than) STARTCORE.  see comments in
- * i386/process.c's proc_run() for details. */
+ * Never send a process management IPI without making sure per_cpu_info's
+ * proc_ipi_pending is clear. */
+// TODO: replace this ghetto with an active message (AM)
 #define I_DEATH     230
 #define I_STARTCORE 231
 /* smp_call_function IPIs, keep in sync with NUM_HANDLER_WRAPPERS (and < 16)
index 617c8fd..05d4ca2 100644 (file)
 struct per_cpu_info {
        uint32_t lock;
        bool preempt_pending;
+       /* this flag is only ever set when holding the global scheduler lock, and
+        * only cleared (without a lock) by the core in a proc_mgmt IPI.  it is
+        * somewhat arch specific (no in order active messages).  same with the
+        * p_to_run and tf_to_pop */
+       // TODO: replace this ghetto with an active message (AM)
+       bool proc_ipi_pending;
        /* a proc_startcore IPI will run these.  replace with a dispatch map? */
        struct proc *p_to_run;
        trapframe_t *SAFE tf_to_pop;
index 27a2699..b6ad69b 100644 (file)
@@ -277,7 +277,7 @@ env_alloc(env_t **newenv_store, envid_t parent_id)
        /* init SharC state */
        sharC_env_init(&e->sharC_env);
 #endif
-       e->num_vcores = 0; // only used in RUNN*_M states
+       e->num_vcores = 0;
        memset(&e->vcoremap, 0, sizeof(e->vcoremap));
 
        memset(&e->env_ancillary_state, 0, sizeof(e->env_ancillary_state));
index 5617b49..d7aca05 100644 (file)
@@ -38,10 +38,15 @@ proc_set_state(p, PROC_RUNNABLE_S);
 proc_set_state(p, PROC_RUNNING_S);
 proc_set_state(p, PROC_RUNNABLE_M);
 // set vcoremap with dispatch plan.  usually done by schedule()
+spin_lock_irqsave(&p->proc_lock);
 p->num_vcores = 5;
 for (int i = 0; i < 5; i++)
        p->vcoremap[i] = i + 1; // vcore0 -> pcore1, etc, for 3 cores
+spin_unlock_irqsave(&p->proc_lock);
 proc_run(p);
+printk("Killing p\n");
+proc_destroy(p);
+printk("Killed p\n");
 udelay(5000000);
 panic("This is okay");
 
index 69ad08b..84e2e7d 100644 (file)
@@ -124,6 +124,12 @@ void proc_run(struct proc *p)
                        return;
                case (PROC_RUNNABLE_S):
                        proc_set_state(p, PROC_RUNNING_S);
+                       // We will want to know where this process is running, even if it is
+                       // only in RUNNING_S.  can use the vcoremap, which makes death easy.
+                       // we may need the pcoremap entry to mark it as a RUNNING_S core, or
+                       // else update it here. (TODO) (PCORE)
+                       p->num_vcores = 0;
+                       p->vcoremap[0] = core_id();
                        spin_unlock_irqsave(&p->proc_lock);
                        // This normally doesn't return, but might error out in the future.
                        proc_startcore(p, &p->env_tf);
@@ -148,14 +154,30 @@ void proc_run(struct proc *p)
                         * after the death IPI.  Otherwise, it would look like a new
                         * process.  So we hold the lock to make sure our IPI went out
                         * before a possible death IPI.
-                        * Likewise, we need interrupts to be disabled, in case one of the
-                        * IPIs was for us, and reenable them after letting go of the lock.
-                        * This is done by spin_lock_irqsave, so be careful if you change
-                        * this.
-                        * Note there is no guarantee this core's interrupts were on, so it
-                        * may not get the IPI for a while... */
-                       for (int i = 0; i < p->num_vcores; i++)
+                        * - This turns out to not be enough, although it is necessary.  We
+                        *   also need to make sure no other proc management IPIs are sent,
+                        *   since IPIs can be received out of order, hence the use of the
+                        *   pending flag. 
+                       // TODO: replace this ghetto with an active message (AM)
+                        * - Be *very* careful with this, since there may be a deadlock when
+                        *   sending an IPI to yourself when another is outstanding.  This
+                        *   shouldn't happen, since we should be holding some sort of
+                        *   global proc management lock when sending any of these IPIs out.
+                        * - Likewise, we need interrupts to be disabled, in case one of the
+                        *   IPIs was for us, and reenable them after letting go of the
+                        *   lock.  This is done by spin_lock_irqsave, so be careful if you
+                        *   change this.
+                        * - This can also be done far more intelligently / efficiently,
+                        *   like skipping in case it's busy and coming back later.
+                        * - Note there is no guarantee this core's interrupts were on, so
+                        *   it may not get the IPI for a while... */
+                       for (int i = 0; i < p->num_vcores; i++) {
+                               // TODO: replace this ghetto with an active message (AM)
+                               while(per_cpu_info[p->vcoremap[i]].proc_ipi_pending)
+                                       cpu_relax();
+                               per_cpu_info[p->vcoremap[i]].proc_ipi_pending = TRUE;
                                send_ipi(p->vcoremap[i], 0, I_STARTCORE);
+                       }
                        spin_unlock_irqsave(&p->proc_lock);
                        break;
                default:
@@ -197,8 +219,10 @@ void proc_run(struct proc *p)
  * some "bundling" code.
  */
 void proc_startcore(struct proc *p, trapframe_t *tf) {
-       // TODO it's possible to be DYING, but it's a rare race.  remove this soon.
-       assert(p->state & (PROC_RUNNING_S | PROC_RUNNING_M));
+       // it's possible to be DYING, but it's a rare race.
+       //if (p->state & (PROC_RUNNING_S | PROC_RUNNING_M))
+       //      printk("dying before (re)startcore on core %d\n", core_id());
+
        // sucks to have ints disabled when doing env_decref and possibly freeing
        disable_irq();
        if (per_cpu_info[core_id()].preempt_pending) {
@@ -208,13 +232,14 @@ void proc_startcore(struct proc *p, trapframe_t *tf) {
        }
        /* If the process wasn't here, then we need to load its address space. */
        if (p != current) {
-               if (proc_incref(p))
+               if (proc_incref(p)) {
                        // getting here would mean someone tried killing this while we tried
                        // to start one of it's contexts (from scratch, o/w we had it's CR3
                        // loaded already)
-                       // if this happens, the death-IPI ought to be on its way...  we can
-                       // either wait, or just cleanup_core() and smp_idle.
-                       panic("Proc is dying, handle me!"); // TODO
+                       // if this happens, a no-op death-IPI ought to be on its way...  we can
+                       // just smp_idle()
+                       smp_idle();
+               }
                lcr3(p->env_cr3);
                // we unloaded the old cr3, so decref it (if it exists)
                // TODO: Consider moving this to wherever we really "mean to leave the
@@ -258,23 +283,46 @@ void proc_destroy(struct proc *p)
        // Note this code relies on this lock disabling interrupts, similar to
        // proc_run.
        spin_lock_irqsave(&p->proc_lock);
-       // Could save the state and do this outside the lock
        switch (p->state) {
                case PROC_DYING:
                        return; // someone else killed this already.
                case PROC_RUNNABLE_S:
                case PROC_RUNNABLE_M:
+                       // Think about other lists, like WAITING, or better ways to do this
                        deschedule_proc(p);
                        break;
+               case PROC_RUNNING_S:
+                       /* We could use the IPI regardless, but the common case here
+                        * probably isn't remote death. */
+                       if (current == p) {
+                               lcr3(boot_cr3);
+                               proc_decref(p); // this decref is for the cr3
+                               current = NULL;
+                       } else {
+                               // TODO: replace this ghetto with an active message (AM)
+                               while(per_cpu_info[p->vcoremap[0]].proc_ipi_pending)
+                                       cpu_relax();
+                               per_cpu_info[p->vcoremap[0]].proc_ipi_pending = TRUE;
+                               send_ipi(p->vcoremap[0], 0, I_DEATH);
+                       }
+                       smp_idle();
+               case PROC_RUNNING_M:
+                       /* Send the DEATH IPI to every core running this process.  See notes
+                        * above about issues with this while loop and proc_ipi_pending */
+                       for (int i = 0; i < p->num_vcores; i++) {
+                               // TODO: replace this ghetto with an active message (AM)
+                               while(per_cpu_info[p->vcoremap[i]].proc_ipi_pending)
+                                       cpu_relax();
+                               per_cpu_info[p->vcoremap[i]].proc_ipi_pending = TRUE;
+                               send_ipi(p->vcoremap[i], 0, I_DEATH);
+                       }
+                       /* TODO: will need to update the pcoremap that's currently in
+                        * schedule */
+                       break;
                default:
-                       // Think about other lists, or better ways to do this
+                       panic("Weird state in proc_destroy");
        }
        proc_set_state(p, PROC_DYING);
-       /* Send the DEATH IPI to every core running this process */
-       for (int i = 0; i < p->num_vcores; i++) {
-               send_ipi(p->vcoremap[i], 0, I_DEATH);
-       }
-       /* TODO: will need to update the pcoremap that's currently in schedule */
 
        atomic_dec(&num_envs);
        /* TODO: (REF) dirty hack.  decref currently uses a lock, but needs to use
index 3fec025..6b71134 100644 (file)
@@ -20,7 +20,7 @@ void udelay(uint64_t usec, uint64_t tsc_freq)
 int main(int argc, char** argv)
 {
        cprintf("Multi-Goodbye, world, from PID: %d!\n", sys_getpid());
-       //while(1);
+       while(1);
        udelay(5000000, 1995014570); // KVM's freq.  Whatever.
 
        return 0;