proc_run() handles kmsgs before running
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 27 Oct 2010 00:32:45 +0000 (17:32 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:56 +0000 (17:35 -0700)
There was a race where we would miss the message (present since we made
the proc_lock a non-irqsave lock), though we never had the messages til
now that could cause the issue.  So proc_run() calls restartcore now.

A necessary part of the change is that process_routine_kmsg() takes a
tf, which is the user TF you want the kernel to pass to the kernel
message (*p->env_tf for proc_run's case).

Also note that the system probably isn't ready yet to overlap proc
management messages (like __death()) with a __launch_kthread() (which
tries to schedule the current process).

kern/arch/i686/trap.c
kern/arch/sparc/trap.c
kern/include/trap.h
kern/src/manager.c
kern/src/process.c
kern/src/smp.c

index 06571af..35438c9 100644 (file)
@@ -500,13 +500,15 @@ void __kernel_message(struct trapframe *tf)
  * make sure immediates still run first (or when they arrive, if processing a
  * bunch of these messages).  This will disable interrupts, and restore them to
  * whatever state you left them. */
-void process_routine_kmsg(void)
+void process_routine_kmsg(struct trapframe *tf)
 {
        per_cpu_info_t *myinfo = &per_cpu_info[core_id()];
        kernel_message_t msg_cp, *k_msg;
        int8_t irq_state = 0;
 
        disable_irqsave(&irq_state);
+       /* If we were told what our TF was, use that.  o/w, go with current_tf. */
+       tf = tf ? tf : current_tf;
        while (1) {
                /* normally, we want ints disabled, so we don't have an empty self-ipi
                 * for every routine message. (imagine a long list of routines).  But we
@@ -535,7 +537,6 @@ void process_routine_kmsg(void)
                        send_self_ipi(I_KERNEL_MSG);
                /* Execute the kernel message */
                assert(msg_cp.pc);
-               msg_cp.pc(current_tf, msg_cp.srcid, msg_cp.arg0, msg_cp.arg1,
-                         msg_cp.arg2);
+               msg_cp.pc(tf, msg_cp.srcid, msg_cp.arg0, msg_cp.arg1, msg_cp.arg2);
        }
 }
index 8e8bb47..e04fb4d 100644 (file)
@@ -254,13 +254,15 @@ void handle_ipi(trapframe_t* tf)
 
 /* Same as in x86.  Might be diff in the future if there is no way to check for
  * immediate messages or there is the ability to selectively mask IPI vectors.*/
-void process_routine_kmsg(void)
+void process_routine_kmsg(struct trapframe *tf)
 {
        per_cpu_info_t *myinfo = &per_cpu_info[core_id()];
        kernel_message_t msg_cp, *k_msg;
        int8_t irq_state = 0;
 
        disable_irqsave(&irq_state);
+       /* If we were told what our TF was, use that.  o/w, go with current_tf. */
+       tf = tf ? tf : current_tf;
        while (1) {
                /* normally, we want ints disabled, so we don't have an empty self-ipi
                 * for every routine message. (imagine a long list of routines).  But we
@@ -287,8 +289,7 @@ void process_routine_kmsg(void)
                        send_ipi(core_id());
                /* Execute the kernel message */
                assert(msg_cp.pc);
-               msg_cp.pc(current_tf, msg_cp.srcid, msg_cp.arg0, msg_cp.arg1,
-                         msg_cp.arg2);
+               msg_cp.pc(tf, msg_cp.srcid, msg_cp.arg0, msg_cp.arg1, msg_cp.arg2);
        }
 }
 
index 0eaf036..c45afce 100644 (file)
@@ -90,6 +90,6 @@ typedef struct kernel_message NTPTV(a0t) NTPTV(a1t) NTPTV(a2t) kernel_message_t;
 
 uint32_t send_kernel_message(uint32_t dst, amr_t pc, TV(a0t) arg0, TV(a1t) arg1,
                              TV(a2t) arg2, int type);
-void process_routine_kmsg(void);
+void process_routine_kmsg(struct trapframe *tf);
 
 #endif /* ROS_KERN_TRAP_H */
index efcca2e..72a858c 100644 (file)
@@ -115,12 +115,12 @@ void manager_brho(void)
 
        /* I usually want this */
        schedule();
-       process_routine_kmsg(); /* maybe do this before schedule() */
+       process_routine_kmsg(0);        /* maybe do this before schedule() */
 
        enable_irq();
        /* this ghetto hack really wants to wait for an interrupt, but time out */
        udelay(60000);  /* wait for IO when there really is nothing to do */
-       process_routine_kmsg();
+       process_routine_kmsg(0);
        printd("No work to do (schedule returned)\n");
        monitor(0);
 
index 692ae16..9909030 100644 (file)
@@ -460,19 +460,16 @@ void proc_run(struct proc *p)
                        p->procinfo->num_vcores = 0;
                        __map_vcore(p, 0, core_id()); // sort of.  this needs work.
                        __seq_end_write(&p->procinfo->coremap_seqctr);
-                       /* __proc_startcore assumes the reference we give it is for current.
-                        * Decref if current is already properly set. */
+                       /* proc_restartcore assumes the reference we give it is for current.
+                        * Decref if current is already properly set, otherwise ensure
+                        * current is set. */
                        if (p == current)
                                kref_put(&p->kref);
-                       /* We don't want to process routine messages here, since it's a bit
-                        * different than when we perform a syscall in this process's
-                        * context.  We want interrupts disabled so that if there was a
-                        * routine message on the way, we'll get the interrupt once we pop
-                        * back to userspace.  */
+                       /* We restartcore, instead of startcore, since startcore is a bit
+                        * lower level and we want a chance to process kmsgs before starting
+                        * the process. */
                        spin_unlock(&p->proc_lock);
-                       disable_irq();
-
-                       __proc_startcore(p, &p->env_tf);
+                       proc_restartcore(p, &p->env_tf);
                        break;
                case (PROC_RUNNABLE_M):
                        /* vcoremap[i] holds the coreid of the physical core allocated to
@@ -512,6 +509,28 @@ void proc_run(struct proc *p)
        }
 }
 
+/* Helper, makes p the 'current' process, dropping the old current/cr3.  Don't
+ * incref - this assumes the passed in reference already counted 'current'. */
+static void __set_proc_current(struct proc *p)
+{
+       /* We use the pcpui to access 'current' to cut down on the core_id() calls,
+        * though who know how expensive/painful they are. */
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       /* If the process wasn't here, then we need to load its address space. */
+       if (p != pcpui->cur_proc) {
+               /* Do not incref here.  We were given the reference to current,
+                * pre-upped. */
+               lcr3(p->env_cr3);
+               /* This is "leaving the process context" of the previous proc.  The
+                * previous lcr3 unloaded the previous proc's context.  This should
+                * rarely happen, since we usually proactively leave process context,
+                * but this is the fallback. */
+               if (pcpui->cur_proc)
+                       kref_put(&pcpui->cur_proc->kref);
+               pcpui->cur_proc = p;
+       }
+}
+
 /* Actually runs the given context (trapframe) of process p on the core this
  * code executes on.  This is called directly by __startcore, which needs to
  * bypass the routine_kmsg check.  Interrupts should be off when you call this.
@@ -528,19 +547,7 @@ void proc_run(struct proc *p)
 static void __proc_startcore(struct proc *p, trapframe_t *tf)
 {
        assert(!irq_is_enabled());
-       /* If the process wasn't here, then we need to load its address space. */
-       if (p != current) {
-               /* Do not incref here.  We were given the reference to current,
-                * pre-upped. */
-               lcr3(p->env_cr3);
-               /* This is "leaving the process context" of the previous proc.  The
-                * previous lcr3 unloaded the previous proc's context.  This should
-                * rarely happen, since we usually proactively leave process context,
-                * but is the fallback. */
-               if (current)
-                       kref_put(&current->kref);
-               set_current_proc(p);
-       }
+       __set_proc_current(p);
        /* need to load our silly state, preferably somewhere other than here so we
         * can avoid the case where the context was just running here.  it's not
         * sufficient to do it in the "new process" if-block above (could be things
@@ -552,12 +559,11 @@ static void __proc_startcore(struct proc *p, trapframe_t *tf)
         * __startcore.  */
        if (p->state == PROC_RUNNING_S)
                env_pop_ancillary_state(p);
-       
        env_pop_tf(tf);
 }
 
-/* Restarts the given context (trapframe) of process p on the core this code
- * executes on.  Calls an internal function to do the work.
+/* Restarts/runs the given context (trapframe) of process p on the core this
+ * code executes on.  Calls an internal function to do the work.
  *
  * In case there are pending routine messages, like __death, __preempt, or
  * __notify, we need to run them.  Alternatively, if there are any, we could
@@ -569,14 +575,17 @@ static void __proc_startcore(struct proc *p, trapframe_t *tf)
  * returning from local traps and such. */
 void proc_restartcore(struct proc *p, trapframe_t *tf)
 {
-       if (current_tf != tf) {
-               printk("Current_tf: %08p, tf: %08p\n", current_tf, tf);
+       if ((tf != &p->env_tf) && (tf != current_tf)) {
+               printk("tf: %08p, Current_tf: %08p, env_tf: %08p\n", tf, current_tf,
+                      &p->env_tf);
                panic("Current TF is jacked...");
        }
-
        /* Need ints disabled when we return from processing (race) */
        disable_irq();
-       process_routine_kmsg();
+       /* Need to be current, in case a kmsg is there that tries to clobber us.
+        * Yes, this gets called again in __proc_startcore(). */
+       __set_proc_current(p);
+       process_routine_kmsg(tf);
        __proc_startcore(p, tf);
 }
 
@@ -1274,7 +1283,7 @@ void __proc_kmsg_pending(struct proc *p, bool ipi_pending)
 {
        if (ipi_pending) {
                kref_put(&p->kref);
-               process_routine_kmsg();
+               process_routine_kmsg(0);
                panic("stack-killing kmsg not found in %s!!!", __FUNCTION__);
        }
 }
index c14771a..e1ab2b1 100644 (file)
@@ -43,10 +43,13 @@ void smp_idle(void)
        int8_t state = 0;
        per_cpu_info_t *myinfo = &per_cpu_info[core_id()];
 
+       /* in the future, we may need to proactively leave process context here.
+        * for now, it is possible to have a current loaded, even if we are idle
+        * (and presumably about to execute a kmsg or fire up a vcore). */
        if (!management_core()) {
                enable_irq();
                while (1) {
-                       process_routine_kmsg();
+                       process_routine_kmsg(0);
                        cpu_halt();
                }
        } else {
@@ -56,7 +59,7 @@ void smp_idle(void)
                 * currently use to do the completion.  Note this also causes us to wait
                 * 10ms regardless of how long the IO takes.  This all needs work. */
                //udelay(10000); /* done in the manager for now */
-               process_routine_kmsg();
+               process_routine_kmsg(0);
                disable_irqsave(&state);
                manager();
        }