Handles routine kmsgs before returning to userspace
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 23 Mar 2010 05:45:40 +0000 (22:45 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:40 +0000 (17:35 -0700)
Renames and splits proc_startcore() to proc_restartcore() and
__proc_startcore(), and changes proc_run() to use a kmsg to start a _S.
Check the Doc and comments for specific reasons.

Documentation/process-internals.txt
kern/arch/i686/pmap.c
kern/arch/i686/trap.c
kern/arch/sparc/trap.c
kern/include/process.h
kern/include/smp.h
kern/src/process.c

index 2175032..f293d63 100644 (file)
@@ -90,7 +90,7 @@ stores or makes a copy of the reference.
 1.5 Functions That Don't or Might Not Return:
 ---------------------------
 Refcnting and especially decreffing gets tricky when there are functions that
-MAY not return.  proc_startcore() does not return (it pops into userspace).
+MAY not return.  proc_restartcore() does not return (it pops into userspace).
 proc_run() might not return, if the core it was called on will pop into
 userspace (if it was a _S, or if the core is part of the vcoremap for a _M).
 
@@ -120,10 +120,11 @@ proc_run(pid2proc(55)) works, since pid2proc() increfs for you.  But you cannot
 proc_run(current), unless you incref current in advance.  Incidentally,
 proc_running current doesn't make a lot of sense.
 
-As another example, proc_startcore() will take your reference and store it
+As another example, __proc_startcore() will take your reference and store it
 in current.  Since it is used by both the __startcore and the interrupt return
-paths, we're currently going with the model of "caller makes sure there is a ref
-for current".  Check its comments for details.
+paths (proc_restartcore() now, formerly called proc_startcore()), we're
+currently going with the model of "caller makes sure there is a ref for
+current".  Check its comments for details.
 
 1.6 Runnable List:
 ---------------------------
@@ -147,7 +148,7 @@ Also note that while proc_run() consumes your reference, it's not actually
 decreffing, so there's no danger within proc_run() of the process dying /
 __proc_free()ing.
 
-proc_startcore(): assumes all references to p are sorted.  *p is already
+__proc_startcore(): assumes all references to p are sorted.  *p is already
 accounted for as if it was current on the core startcore runs on. (there is only
 one refcnt for both *p and current, not 2 separate ones).
 
@@ -255,13 +256,13 @@ specifically talking about when a process's cr3 is loaded on a core.  Usually,
 current is also set (the exception for now is when processing ARSCs).
 
 There are a couple different ways to do this.  One is to never unload a context
-until something new is being run there (handled solely in proc_startcore()).
+until something new is being run there (handled solely in __proc_startcore()).
 Another way is to always explicitly leave the core, like by abandon_core()ing.
 
 The issue with the former is that you could have contexts sitting around for a
 while, and also would have a bit of extra latency when __proc_free()ing during
-someone *else's* proc_startcore() (though that could be avoided if it becomes a
-real issue, via some form of reaping).  You'll also probably have excessive
+someone *else's* __proc_startcore() (though that could be avoided if it becomes
+real issue, via some form of reaping).  You'll also probably have excessive
 decrefs (based on the interactions between proc_run() and __startcore()).
 
 The issue with the latter is excessive TLB shootdowns and corner cases.  There
@@ -275,7 +276,7 @@ process's context is loaded.
 2.2 Here's how it is done now:
 ---------------------------
 We try to proactively leave, but have the ability to stay in context til
-proc_startcore() to handle the corner cases (and to maybe cut down the TLB
+__proc_startcore() to handle the corner cases (and to maybe cut down the TLB
 flushes later).  To stop proactively leaving, just change abandon_core() to not
 do anything with current/cr3.  You'll see weird things like processes that won't
 die until their old cores are reused.  The reason we proactively leave context
@@ -283,9 +284,9 @@ is to help with sanity for these issues, and also to avoid decref's in
 __startcore().
 
 A couple other details: __startcore() sorts the extra increfs, and
-proc_startcore() sorts leaving the old context.  Anytime a __startcore kernel
+__proc_startcore() sorts leaving the old context.  Anytime a __startcore kernel
 message is sent, the sender increfs in advance for the current refcnt.  If that
-was in error, __startcore decrefs.  proc_startcore(), which the last moment
+was in error, __startcore decrefs.  __proc_startcore(), which the last moment
 before we *must* have the cr3/current issues sorted, does the actual check if
 there was an old process there or not, while it handles the lcr3 (if necessary).
 In general, lcr3's ought to have refcnts near them, or else comments explaining
@@ -310,7 +311,7 @@ Note that dealing with interrupting processes that are in the kernel is tricky.
 There is no true process context, so we can't leave a core until the kernel is
 in a "safe place", i.e. it's state is bundled enough that it can be recontinued
 later.  Calls of this type are routine kernel messages, executed at a convenient
-time (specifically, before we return to userspace in proc_startcore().
+time (specifically, before we return to userspace in proc_restartcore().
 
 This same thing applies to __death messages.  Even though a process is dying, it
 doesn't mean we can just drop whatever the kernel was doing on its behalf.  For
@@ -331,10 +332,32 @@ the old "active messages" into a generic work execution message (a kernel
 message) that can be delayed or shipped to another core.  These types of
 messages will not be executed immediately on the receiving pcore - instead they
 are on the queue for "when there's nothing else to do in the kernel", which is
-checked in smp_idle() and before returning to userspace in proc_startcore().
+checked in smp_idle() and before returning to userspace in proc_restartcore().
 Additionally, these kernel messages can also be queued on an alarm queue,
 delaying their activation as part of a generic kernel alarm facility.
 
+One subtlety is that __proc_startcore() shouldn't check for messages, since it
+is called by __startcore (a message).  Checking there would run the messages out
+of order, which is exactly what we are trying to avoid (total chaos).  No one
+should call __proc_startcore, other than proc_restartcore() or __startcore().
+If we ever have functions that do so, if they are not called from a message,
+they must check for outstanding messages.
+
+This last subtlety is why we needed to change proc_run()'s _S case to use a
+local message instead of calling proc_starcore (and why no one should ever call
+proc_startcore()).  We could unlock, thereby freeing another core to change the
+proc state and send a message to us, then try to proc_startcore, and then
+reading the message before we had installed current or had a userspace TF to
+preempt, and probably a few other things.  Treating _S as a local message is
+cleaner, begs to be merged in the code with _M's code, and uses the messaging
+infrastructure to avoid all the races that it was created to handle.
+
+Incidentally, we don't need to worry about missing messages while trying to pop
+back to userspace from __proc_startcore, since an IPI will be on the way
+(possibly a self-ipi caused by the __kernel_message() handler).  This is also
+why we needed to make process_routine_kmsg() keep interrupts disabled when it
+stops (there's a race between checking the queue and disabling ints).
+
 4. Preemption and Notification Issues:
 ===========================
 4.1: Message Ordering and Local Calls:
index 89eaded..018cc73 100644 (file)
@@ -55,7 +55,7 @@ segdesc_t gdt[] =
        // 0x28 - tss, initialized in idt_init()
        [GD_TSS >> 3] = SEG_NULL,
 
-       // 0x30 - LDT, set per-process in proc_startcore()
+       // 0x30 - LDT, set per-process
        [GD_LDT >> 3] = SEG_NULL
 };
 
index d12a173..515eaf5 100644 (file)
@@ -246,7 +246,7 @@ trap(trapframe_t *tf)
        trap_dispatch(tf);
 
        // Return to the current process, which should be runnable.
-       proc_startcore(current, tf); // Note the comment in syscall.c
+       proc_restartcore(current, tf); // Note the comment in syscall.c
 }
 
 void
@@ -383,7 +383,7 @@ void sysenter_callwrapper(struct trapframe *tf)
         * restore the proper value in current before returning to here.
         * likewise, tf could be pointing to random gibberish.
         */
-       proc_startcore(current, tf);
+       proc_restartcore(current, tf);
 }
 
 struct kmem_cache *kernel_msg_cache;
index 750521f..a8222d5 100644 (file)
@@ -386,7 +386,7 @@ handle_syscall(trapframe_t* state)
        state->gpr[8] = syscall(current,num,a1,a2,a3,a4,a5);
        proc_decref(current, 1);
 
-       env_pop_tf(state);
+       proc_restartcore(current,state);
 }
 
 void
index e704127..3b77871 100644 (file)
@@ -78,8 +78,7 @@ int __proc_set_state(struct proc *p, uint32_t state) WRITES(p->state);
 struct proc *pid2proc(pid_t pid);
 bool proc_controls(struct proc *SAFE actor, struct proc *SAFE target);
 void proc_run(struct proc *SAFE p);
-void proc_startcore(struct proc *SAFE p, trapframe_t *SAFE tf)
-     __attribute__((noreturn));
+void proc_restartcore(struct proc *SAFE p, trapframe_t *SAFE tf);
 void proc_destroy(struct proc *SAFE p);
 void proc_yield(struct proc *SAFE p);
 /* Exposed for sys_getvcoreid(), til it's unnecessary */
index a05062b..63ebf1b 100644 (file)
@@ -24,7 +24,6 @@ struct per_cpu_info {
        spinlock_t lock;
        struct proc *cur_proc;
        trapframe_t *cur_tf;
-       bool preempt_pending;
 
 #ifdef __SHARC__
        // held spin-locks. this will have to go elsewhere if multiple kernel
index f46d1cc..b4c146a 100644 (file)
@@ -381,6 +381,7 @@ void proc_run(struct proc *p)
                                smp_idle(); // this never returns
                        return;
                case (PROC_RUNNABLE_S):
+                       assert(current != p);
                        __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.
@@ -390,13 +391,10 @@ 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);
-                       spin_unlock_irqsave(&p->proc_lock);
-                       /* Transferring our reference to startcore, where p will become
-                        * current.  If it already is, decref in advance.  This is similar
-                        * to __startcore(), in that it sorts out the refcnt accounting.  */
-                       if (current == p)
-                               proc_decref(p, 1);
-                       proc_startcore(p, &p->env_tf);
+                       p->env_refcnt++; // TODO: (REF) use incref
+                       p->procinfo->vcoremap[0].tf_to_run = &p->env_tf;
+                       send_kernel_message(core_id(), __startcore, p, 0, 0, AMSG_IMMEDIATE);
+                       __proc_unlock_ipi_pending(p, TRUE);
                        break;
                case (PROC_RUNNABLE_M):
                        /* vcoremap[i] holds the coreid of the physical core allocated to
@@ -432,8 +430,8 @@ void proc_run(struct proc *p)
                        /* Unlock and decref/wait for the IPI if one is pending.  This will
                         * eat the reference if we aren't returning. 
                         *
-                        * There a subtle race avoidance here.  proc_startcore can handle a
-                        * death message, but we can't have the startcore come after the
+                        * There a subtle race avoidance here.  __proc_startcore can handle
+                        * 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 til after we send our message, which prevents a
                         * possible death message.
@@ -451,35 +449,10 @@ void proc_run(struct proc *p)
        }
 }
 
-/* Runs the given context (trapframe) of process p on the core this code
- * executes on.
- *
- * Given we are RUNNING_*, an IPI for death or preemption could come in:
- * 1. death attempt (IPI to kill whatever is on your core):
- *             we don't need to worry about protecting the stack, since we're
- *             abandoning ship - just need to get a good cr3 and decref current, which
- *             the death handler will do.
- *             If a death IPI comes in, we immediately stop this function and will
- *             never come back.
- * 2. preempt attempt (IPI to package state and maybe run something else):
- *             - if a preempt attempt comes in while we're in the kernel, it'll
- *             just set a flag.  we could attempt to bundle the kernel state
- *             and rerun it later, but it's really messy (and possibly given
- *             back to userspace).  we'll disable ints, check this flag, and if
- *             so, handle the preemption using the same funcs as the normal
- *             preemption handler.  nonblocking kernel calls will just slow
- *             down the preemption while they work.  blocking kernel calls will
- *             need to package their state properly anyway.
- *
- * TODO: in general, think about when we no longer need the stack, in case we
- * are preempted and expected to run again from somewhere else.  we can't
- * expect to have the kernel stack around anymore.  the nice thing about being
- * at this point is that we are just about ready to give up the stack anyways.
- *
- * I think we need to make it such that the kernel in "process context" never
- * gets removed from the core (displaced from its stack) without going through
- * some "bundling" code.
- *
+/* 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.
+ * 
  * A note on refcnting: this function will not return, and your proc reference
  * will end up stored in current.  This will make no changes to p's refcnt, so
  * do your accounting such that there is only the +1 for current.  This means if
@@ -489,17 +462,9 @@ void proc_run(struct proc *p)
  * in current and you have one reference, like proc_run(non_current_p), then
  * also do nothing.  The refcnt for your *p will count for the reference stored
  * in current. */
-void proc_startcore(struct proc *p, trapframe_t *tf) {
-       // 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) {
-               // TODO: handle preemption
-               // the functions will need to consider deal with current like down below
-               panic("Preemption not supported!");
-       }
+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,
@@ -525,6 +490,25 @@ void proc_startcore(struct proc *p, trapframe_t *tf) {
        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.
+ * 
+ * In case there are pending routine messages, like __death, __preempt, or
+ * __notify, we need to run them.  Alternatively, if there are any, we could
+ * self_ipi, and run the messages immediately after popping back to userspace,
+ * but that would have crappy overhead.
+ *
+ * Refcnting: this will not return, and it assumes that you've accounted for
+ * your reference as if it was the ref for "current" (which is what happens when
+ * returning from local traps and such. */
+void proc_restartcore(struct proc *p, trapframe_t *tf)
+{
+       /* Need ints disabled when we return from processing (race) */
+       disable_irq();
+       process_routine_kmsg();
+       __proc_startcore(p, tf);
+}
+
 /*
  * Destroys the given process.  This may be called from another process, a light
  * kernel thread (no real process context), asynchronously/cross-core, or from
@@ -1016,7 +1000,7 @@ void proc_decref(struct proc *p, size_t count)
 }
 
 /* Kernel message handler to start a process's context on this core.  Tightly
- * coupled with proc_run() */
+ * coupled with proc_run().  Interrupts are disabled. */
 void __startcore(trapframe_t *tf, uint32_t srcid, void *a0, void *a1, void *a2)
 {
        uint32_t pcoreid = core_id(), vcoreid;
@@ -1042,7 +1026,7 @@ void __startcore(trapframe_t *tf, uint32_t srcid, void *a0, void *a1, void *a2)
        /* the sender of the amsg increfed, thinking we weren't running current. */
        if (p_to_run == current)
                proc_decref(p_to_run, 1);
-       proc_startcore(p_to_run, tf_to_pop);
+       __proc_startcore(p_to_run, tf_to_pop);
 }
 
 /* Stop running whatever context is on this core, load a known-good cr3, and
@@ -1058,7 +1042,7 @@ void abandon_core(void)
 /* Kernel message handler to clean up the core when a process is dying.
  * 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. */
+ * It could happen if a process decref'd before __proc_startcore could incref. */
 void __death(trapframe_t *tf, uint32_t srcid, void *SNT a0, void *SNT a1,
              void *SNT a2)
 {