Process management via active messages
authorBarret Rhoden <brho@cs.berkeley.edu>
Sat, 22 Aug 2009 01:19:16 +0000 (18:19 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 31 Aug 2009 21:10:35 +0000 (14:10 -0700)
Replaces those horribly hackish send_ipi's with active messages.  Note
that the messages are being sent with the sync wrapper, which is just a
while(send) cpu_relax().  Meaning that it can deadlock the system,
especially when sending to yourself.  We need to do something a little
more intelligent for these situations (which is an arch-independent
issue).

Also fixed the x86 active messages to handle functions that don't return
(which was the point of all this in the first place).

kern/arch/i386/process.c
kern/arch/i386/trap.c
kern/arch/i386/trap.h
kern/arch/i386/trapentry.S
kern/include/env.h
kern/include/process.h
kern/include/smp.h
kern/include/trap.h
kern/src/manager.c
kern/src/process.c
kern/src/smp.c

index c559978..a02d3c8 100644 (file)
@@ -8,23 +8,15 @@
 #include <assert.h>
 #include <stdio.h>
 
-/* Interrupt handler to start a process's context on this core. */
-void __startcore(trapframe_t *tf)
+/* Active message handler to start a process's context on this core.  Tightly
+ * coupled with proc_run() */
+void __startcore(trapframe_t *tf, uint32_t srcid, uint32_t a0, uint32_t a1,
+                 uint32_t a2)
 {
        uint32_t coreid = core_id();
-       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
-        * anyways, since it's set up as an interrupt gate for now. */
-       lapic_send_eoi();
+       struct proc *p_to_run = (struct proc *SAFE) TC(a0);
+       trapframe_t local_tf;
+       trapframe_t *tf_to_pop = (trapframe_t *SAFE) TC(a1);
 
        printk("Startcore on core %d\n", coreid);
        assert(p_to_run);
@@ -40,11 +32,12 @@ void __startcore(trapframe_t *tf)
        proc_startcore(p_to_run, tf_to_pop);
 }
 
-/* Interrupt handler to stop running whatever context is on this core and to
- * idle.  Note this leaves no trace of what was running.
+/* Active message handler to stop running whatever context is on this core and
+ * to 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)
+void __death(trapframe_t *tf, uint32_t srcid, uint32_t a0, uint32_t a1,
+             uint32_t a2)
 {
        /* If we are currently running an address space on our core, we need a known
         * good pgdir before releasing the old one.  This is currently the major
@@ -56,9 +49,5 @@ void __death(trapframe_t *tf)
                proc_decref(current);
                current = NULL;
        }
-       // 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();     
+       smp_idle();
 }
index ef5e1df..6d4c3f4 100644 (file)
@@ -386,6 +386,8 @@ uint32_t send_active_message(uint32_t dst, amr_t pc, uint32_t arg0,
                break;
        }
        spin_unlock_irqsave(&per_cpu_info[dst].amsg_lock);
+       // since we touched memory the other core will touch (the lock), we don't
+       // need an wmb_f()
        if (!retval)
                send_ipi(dst, 0, I_ACTIVE_MSG);
        return retval;
@@ -403,6 +405,7 @@ void __active_message(trapframe_t *tf)
        struct per_cpu_info *myinfo = &per_cpu_info[core_id()];
        active_message_t amsg;
 
+       lapic_send_eoi();
        while (1) { // will break out when we find an empty amsg
                /* Get the message */
                spin_lock_irqsave(&myinfo->amsg_lock);
@@ -413,12 +416,18 @@ void __active_message(trapframe_t *tf)
                                               NUM_ACTIVE_MESSAGES;
                } else { // was no PC in the current active message, meaning we do nothing
                        spin_unlock_irqsave(&myinfo->amsg_lock);
-                       lapic_send_eoi();
                        return;
                }
+               /* In case the function doesn't return (which is common: __startcore,
+                * __death, etc), there is a chance we could lose an amsg.  We can only
+                * have up to two interrupts outstanding, and if we never return, we
+                * never deal with any other amsgs.  This extra IPI hurts performance
+                * but is only necessary if there is another outstanding message in the
+                * buffer, but makes sure we never miss out on an amsg. */
+               if (myinfo->active_msgs[myinfo->amsg_current].pc)
+                       send_ipi(core_id(), 0, I_ACTIVE_MSG);
                spin_unlock_irqsave(&myinfo->amsg_lock);
                /* Execute the active message */
                amsg.pc(tf, amsg.srcid, amsg.arg0, amsg.arg1, amsg.arg2);
        }
-
 }
index 23fc032..d458d48 100644 (file)
 #define T_DEFAULT   0xdeadbeef         // catchall
 
 /* IPIs */
-/* Direct/Hardwired IPIs.  Hardwired in trapentry.S
- * 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
-#define I_ACTIVE_MSG 232
+/* Testing IPI (used in testing.c) */
+#define I_TESTING              230
 /* smp_call_function IPIs, keep in sync with NUM_HANDLER_WRAPPERS (and < 16)
  * it's important that this begins with 0xf0.  check i386/trap.c for details. */
-#define I_SMP_CALL0 0xf0 // 240
-#define I_SMP_CALL1 0xf1
-#define I_SMP_CALL2 0xf2
-#define I_SMP_CALL3 0xf3
-#define I_SMP_CALL4 0xf4
+#define I_SMP_CALL0    0xf0 // 240
+#define I_SMP_CALL1    0xf1
+#define I_SMP_CALL2    0xf2
+#define I_SMP_CALL3    0xf3
+#define I_SMP_CALL4    0xf4
 #define I_SMP_CALL_LAST I_SMP_CALL4
-/* Testing IPI (used in testing.c) */
-#define I_TESTING 0xff
+/* Direct/Hardwired IPIs.  Hardwired in trapentry.S */
+#define I_ACTIVE_MSG   255
 
 /* Number of active messages available per core (arbitrary) */
 #define NUM_ACTIVE_MESSAGES 5
index 632e2db..2cac166 100644 (file)
@@ -17,7 +17,7 @@
  * a trap.  It pushes a trap number onto the stack, then jumps to _alltraps.
  * It also builds this traps portion of the trap_tbl.
  * Use TRAPHANDLER for traps where the CPU automatically pushes an error code.
- */ 
+ */
 #define TRAPHANDLER(name, num)                                                                 \
        .text;                                                                                                          \
        .globl name;            /* define global symbol for 'name' */   \
@@ -61,7 +61,7 @@
        .long name;                                                     \
        .long num
 
-/* Same as above, but takes a specific function to jump to.  See comments 
+/* Same as above, but takes a specific function to jump to.  See comments
  * below from _allirqs for details.
  */
 #define IRQ_HANDLER_SPEC(name, num, func)                                      \
@@ -141,9 +141,9 @@ IRQ_HANDLER(IRQ13, 45)
 IRQ_HANDLER(IRQ14, 46)
 IRQ_HANDLER(IRQ15, 47)
 /* 25 general purpose vectors, for use by the LAPIC.  Can expand later. */
-IRQ_HANDLER_SPEC(IRQ198, I_DEATH, __death) 
-IRQ_HANDLER_SPEC(IRQ199, I_STARTCORE, __startcore) 
-IRQ_HANDLER_SPEC(IRQ200, I_ACTIVE_MSG, __active_message) 
+IRQ_HANDLER(IRQ198, I_TESTING) # used in testing.c
+IRQ_HANDLER(IRQ199, 231)
+IRQ_HANDLER(IRQ200, 232)
 IRQ_HANDLER(IRQ201, 233)
 IRQ_HANDLER(IRQ202, 234)
 IRQ_HANDLER(IRQ203, 235)
@@ -167,7 +167,7 @@ IRQ_HANDLER(IRQ219, 251)
 IRQ_HANDLER(IRQ220, 252)
 IRQ_HANDLER(IRQ221, 253)
 IRQ_HANDLER(IRQ222, 254)
-IRQ_HANDLER(IRQ223, 255) # I_TESTING in testing.c
+IRQ_HANDLER_SPEC(IRQ223, I_ACTIVE_MSG, __active_message)
 
 /* Technically, these HANDLER entries do not need to be in numeric order */
 TRAPHANDLER_NOEC(ISR_syscall, T_SYSCALL)
@@ -201,7 +201,7 @@ _alltraps:
        iret
 
 /* will need to think about when we reenable interrupts.  right now, iret does it,
- * if the previous EFLAGS had interrupts enabled 
+ * if the previous EFLAGS had interrupts enabled
  */
 _allirqs:
        cld
@@ -227,7 +227,7 @@ sysenter_handler:
        sti                                             # enable interrupts (things are sane here)
        cld
        pushfl                                  # save the eflags
-       pushl $0                                # these zeros keep the trapframe looking the same 
+       pushl $0                                # these zeros keep the trapframe looking the same
        pushl $0                                # as when we receive a trap or interrupt
        pushl $0                                # and CS == 0 lets the kernel know it was a sysenter
        pushl $T_SYSCALL                # helps with print_trapframe
index d3cb93c..d5e1048 100644 (file)
@@ -93,7 +93,7 @@ extern env_t* curenvs[MAX_NUM_CPUS];
 
 void   env_init(void);
 int            env_alloc(env_t *SAFE*SAFE e, envid_t parent_id);
-void   env_init_trapframe(trapframe_t *tf);
+void   env_init_trapframe(trapframe_t *SAFE tf);
 void   env_set_program_counter(env_t* e, uintptr_t pc);
 void   env_push_ancillary_state(env_t* e);
 void   env_pop_ancillary_state(env_t* e);
index ee2bcb7..095e7c7 100644 (file)
@@ -11,6 +11,7 @@
 #define ROS_KERN_PROCESS_H
 
 #include <arch/types.h>
+#include <trap.h>
 #include <atomic.h>
 
 /* Process States.  Not 100% on the names yet. */
@@ -37,12 +38,21 @@ extern spinlock_t runnablelist_lock;
 
 int proc_set_state(struct proc *p, uint32_t state) WRITES(p->state);
 struct proc *get_proc(unsigned pid);
-bool proc_controls(struct proc *actor, struct proc *target);
-void proc_run(struct proc *p);
-void (proc_startcore)(struct proc *p, trapframe_t *tf) __attribute__((noreturn));
+bool proc_controls(struct proc *SAFE actor, struct proc *SAFE target);
+void proc_run(struct proc *SAFE p);
+// TODO: why do we need these parentheses?
+void (proc_startcore)(struct proc *SAFE p, trapframe_t *SAFE tf)
+     __attribute__((noreturn));
 void (proc_destroy)(struct proc *SAFE p);
 
 /* The reference counts are mostly to track how many cores loaded the cr3 */
 error_t proc_incref(struct proc *SAFE p);
 void proc_decref(struct proc *SAFE p);
+
+/* Active message handlers for process management */
+void __startcore(trapframe_t *tf, uint32_t srcid, uint32_t a0, uint32_t a1,
+                 uint32_t a2);
+void __death(trapframe_t *tf, uint32_t srcid, uint32_t a0, uint32_t a1,
+             uint32_t a2);
+
 #endif // !ROS_KERN_PROCESS_H
index 0120de1..1596d20 100644 (file)
@@ -26,16 +26,6 @@ struct per_cpu_info {
        spinlock_t amsg_lock;
        unsigned amsg_current;
        active_message_t active_msgs[NUM_ACTIVE_MESSAGES];
-
-                       /* 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;
 #endif
 };
 extern struct per_cpu_info per_cpu_info[MAX_NUM_CPUS];
index 264f15d..9af6fac 100644 (file)
@@ -6,6 +6,7 @@
 # error "This is an ROS kernel header; user programs should not #include it"
 #endif
 
+#include <arch/arch.h>
 #include <arch/mmu.h>
 #include <arch/trap.h>
 
@@ -16,7 +17,7 @@ typedef struct InterruptHandler {
        poly_isr_t isr;
        TV(t) data;
 } handler_t;
-extern handler_t interrupt_handlers[];
+extern handler_t (COUNT(256) interrupt_handlers)[];
 
 void idt_init(void);
 void register_interrupt_handler(handler_t (COUNT(256)table)[], uint8_t int_num,
@@ -55,4 +56,14 @@ typedef struct
 
 uint32_t send_active_message(uint32_t dst, amr_t pc, uint32_t arg0,
                              uint32_t arg1, uint32_t arg2);
+
+/* Spins til the active message is sent.  Could block in the future. */
+static inline void send_active_msg_sync(uint32_t dst, amr_t pc, uint32_t arg0,
+                                        uint32_t arg1, uint32_t arg2)
+{
+       while (send_active_message(dst, pc, arg0, arg1, arg2))
+               cpu_relax();
+       return;
+}
+
 #endif /* ROS_KERN_TRAP_H */
index e161b06..8df76cc 100644 (file)
@@ -32,8 +32,6 @@ void manager(void)
        static uint8_t progress = 0;
        struct proc *envs[256];
 
-test_active_messages();
-panic("");
 struct proc *p = kfs_proc_create(kfs_lookup_path("roslib_mhello"));
 // being proper and all:
 proc_set_state(p, PROC_RUNNABLE_S);
@@ -46,6 +44,7 @@ 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);
+udelay(5000000);
 printk("Killing p\n");
 proc_destroy(p);
 printk("Killed p\n");
index 9d12868..ad87f7e 100644 (file)
@@ -136,48 +136,33 @@ void proc_run(struct proc *p)
                        break;
                case (PROC_RUNNABLE_M):
                        proc_set_state(p, PROC_RUNNING_M);
-                       /* Prep each core that we are going to run this process on.
-                        * vcoremap[i] holds the coreid of the physical core allocated to
-                        * this process.  It is set outside proc_run */
-                       for (int i = 0; i < p->num_vcores; i++) {
-                               per_cpu_info[p->vcoremap[i]].p_to_run = p;
+                       /* vcoremap[i] holds the coreid of the physical core allocated to
+                        * this process.  It is set outside proc_run.  For the active
+                        * message, a0 = struct proc*, a1 = struct trapframe*.   */
+                       if (p->num_vcores) {
+                               // TODO: handle silly state (HSS)
+                               // set virtual core 0 to run the main context
+                               send_active_msg_sync(p->vcoremap[0], __startcore, p,
+                                                    &p->env_tf, 0);
+                               /* handle the others.  note the sync message will spin until
+                                * there is a free active message slot, which could lock up the
+                                * system.  think about this. (TODO) */
+                               for (int i = 1; i < p->num_vcores; i++)
+                                       send_active_msg_sync(p->vcoremap[i], __startcore, p, 0, 0);
                        }
-                       // set virtual core 0 to run the main context
-                       // TODO: handle silly state (HSS)
-                       per_cpu_info[p->vcoremap[0]].tf_to_pop = &p->env_tf;
-                       /* needed to make sure the writes beat the IPI.  could have done it
-                        * with other mem accesses, like grabbing a lock, and the write
-                        * would have made it */
-                       wmb_f();
                        /* There a subtle (attempted) race avoidance here.  proc_startcore
-                        * can handle a death IPI, but we can't have the startcore come
-                        * 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.
-                        * - 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.
+                        * can handle a death message, but we can't have the startcore come
+                        * after the death message.  Otherwise, it would look like a new
+                        * process.  So we hold the lock to make sure our message went out
+                        * before a possible death message.
                         * - Likewise, we need interrupts to be disabled, in case one of the
-                        *   IPIs was for us, and reenable them after letting go of the
+                        *   messages 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);
-                       }
+                        *   it may not get the message for a while... */
                        spin_unlock_irqsave(&p->proc_lock);
                        break;
                default:
@@ -292,32 +277,22 @@ void proc_destroy(struct proc *p)
                        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 0
+                       // here's how to do it manually
                        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();
+                       #endif
+                       send_active_msg_sync(p->vcoremap[0], __death, 0, 0, 0);
+                       break;
                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 */
+                       /* Send the DEATH message to every core running this process. */
+                       for (int i = 0; i < p->num_vcores; i++)
+                               send_active_msg_sync(p->vcoremap[i], __death, 0, 0, 0);
+                       /* TODO: might need to update the pcoremap that's currently in
+                        * schedule, though probably not. */
                        break;
                default:
                        panic("Weird state in proc_destroy");
index dd3fca1..8c6c924 100644 (file)
@@ -33,7 +33,7 @@ atomic_t outstanding_calls = 0;
  *   queue, then halt again.
  *
  * TODO: think about resetting the stack pointer at the beginning for worker
- * cores.
+ * cores. (keeps the stack from growing if we never go back to userspace).
  * TODO: think about unifying the manager into a workqueue function, so we don't
  * need to check mgmt_core in here.  it gets a little ugly, since there are
  * other places where we check for mgmt and might not smp_idle / call manager.