Proc code unmaps vcores when taking cores
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 30 Apr 2012 20:48:29 +0000 (13:48 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 5 Sep 2012 21:43:56 +0000 (14:43 -0700)
Instead of leaving the mapping up for the __preempt or __death code to
deal with.  That code now uses the pcpui var.  Check out the doc changes
for more info.

Documentation/process-internals.txt
kern/src/event.c
kern/src/process.c

index 4b77c99..e3301c2 100644 (file)
@@ -387,12 +387,14 @@ something.
 ----------------
 There are two ways to deal with this.  One (and the better one, I think) is to
 check state, and determine if it should proceed or abort.  This requires that
-all local-fate dependent calls always have enough state, meaning that any
-function that results in sending a directive to a vcore store enough info in
-the proc struct that a local call can determine if it should take action or
-abort.  This might be sufficient.  This works for death already, since you
-aren't supposed to do anything other than die (and restore any invariants
-first, handled in Section 3).  We'll go with this way.
+all local-fate dependent calls always have enough state to do its job.  In the
+past, this meant that any function that results in sending a directive to a
+vcore store enough info in the proc struct that a local call can determine if
+it should take action or abort.  In the past, we used the vcore/pcoremap as a
+way to send info to the receiver about what vcore they are (or should be).
+Now, we store that info in pcpui (for '__startcore', we send it as a
+parameter.  Either way, the general idea is still true: local calls can
+proceed when they are called, and not self-ipi'd to a nebulous later time.
 
 The other way is to send the work (including the checks) in a self-ipi kernel
 message.  This will guarantee that the message is executed after any existing
@@ -411,23 +413,29 @@ for a proc until AFTER the preemption is completed.
 4.2: Preempt-Served Flag
 ----------------
 We want to be able to consider a pcore free once its owning proc has dealt
-with removing it (not necessarily taken from the vcoremap, but at least it is
-a done-deal that the core will go away and the messages are sent).  This
-allows a scheduler-like function to easily take a core and then give it to
-someone else, without waiting for each vcore to respond, saying that the pcore
-is free/idle.
-
-Since we want to keep the pcore in the vcoremap, we need another signal to let
-a process know a message is already on its way.  preempt_pending is a signal
-to userspace that the alarm was set, not that an actual message is on its way
-and that a vcore's fate is sealed.  Since we can't use a pcore's presence in
-the vcoremap to determine that the core should be revoked, we have to check
-the "fate sealed"/preempt-served flag. 
-
-It's a bit of a pain to have this flag, just to resolve this race in the
-kernel, though the local call would have to check the vcoremap anyway,
-incurring a cache miss if we go with using the vcoremap to signal the
-impending message.
+with removing it.  This allows a scheduler-like function to easily take a core
+and then give it to someone else, without waiting for each vcore to respond,
+saying that the pcore is free/idle.
+
+We used to not unmap until we were in '__preempt' or '__death', and we needed
+a flag to tell yield-like calls that a message was already on the way and to
+not rely on the vcoremap.  This is pretty fucked up for a number of reasons,
+so we changed that.  But we still wanted to know when a preempt was in
+progress so that the kernel could avoid giving out the vcore until the preempt
+was complete.
+
+Here's the scenario: we send a '__startcore' to core 3 for VC5->PC3.  Then we
+quickly send a '__preempt' to 3, and then a '__startcore' to core 4 (a
+different pcore) for VC5->PC4.  Imagine all of this happens before the first
+'__startcore' gets processed (IRQ delay, fast ksched, whatever).  We need to
+not run the second '__startcore' on pcore 4 before the preemption has saved
+all of the state of the VC5.  So we spin on preempt_served (which may get
+renamed to preempt_in_progress).  We need to do this in the sender, and not
+the receiver (not in the kmsg), because the kmsgs can't tell which one they
+are.  Specifically, the first '__startcore' on core 3 runs the same code as
+the '__startcore' on core 4, working on the same vcore.  Anything we tell VC5
+will be seen by both PC3 and PC4.  We'd end up deadlocking on PC3 while it
+spins waiting for the preempt message that also needs to run on PC3.
 
 The preempt_pending flag is actual a timestamp, with the expiration time of
 the core at which the message will be sent.  We could try to use that, but
@@ -585,14 +593,13 @@ flexibility in schedule()-like functions (no need to wait to give the core
 out), quicker dispatch latencies, less contention on shared structs (like the
 idle-core-map), etc.
 
-Also, we don't remove the pcore from the vcoremap, even if it is being
-allocated to another core (the same pcore can exist in two vcoremaps, contrary
-to older statements).  Taking the pcore from the vcoremap would mean some
-non-fate related local calls (sys_get_vcoreid()) will fail, since the vcoreid
-is gone!  Additionally, we don't need a vcoreid in the k_msg (we would have if
-we could not use the vcore/pcoremappings).  There should not be any issues
-with the new process sending messages to the pcore before the core is sorted,
-since k_msgs are delivered in order.
+This 'freeing' of the pcore is from the perspective of the kernel scheduler
+and the proc struct.  Contrary to all previous announcements, vcores are
+unmapped from pcores when sending k_msgs (technically right after), while
+holding the lock.  The pcore isn't actually not-running-the-proc until the
+kmsg completes and we abandon_core().  Previously, we used the vcoremap to
+communicate to other cores in a lock-free manner, but that was pretty shitty
+and now we just store the vcoreid in pcpu info.
 
 Another tricky part is the seq_ctr used to signal userspace of changes to the
 coremap or num_vcores (coremap_seqctr).  While we may not even need this in the
@@ -677,6 +684,30 @@ We also considered using the transition stack as a signal that a process is in
 a notification handler.  The kernel can inspect the stack pointer to determine
 this.  It's possible, but unnecessary.
 
+Using the pcoremap as a way to pass info with kmsgs: it worked a little, but
+had some serious problems, as well as making life difficult.  It had two
+purposes: help with local fate calls (yield) and allow broadcast messaging.
+The main issue is that it was using a global struct to pass info with
+messages, but it was based on the snapshot of state at the time the message
+was sent.  When you send a bunch of messages, certain state may have changed
+between messages, and the old snapshot isn't there anymore by the time the
+message gets there.  To avoid this, we went through some hoops and had some
+fragile code that would use other signals to avoid those scenarios where the
+global state change would send the wrong message.  It was tough to understand,
+and not clear it was correct (hint: it wasn't).  Here's an example (on one
+pcore): if we send a preempt and we then try to map that pcore to another
+vcore in the same process before the preempt call checks its pcoremap, we'll
+clobber the pcore->vcore mapping (used by startcore) and the preempt will
+think it is the new vcore, not the one it was when the message was sent.
+While this is a bit convoluted, I can imagine a ksched doing this, and
+perhaps with weird IRQ delays, the messages might get delayed enough for it to
+happen.  I'd rather not have to have the ksched worry about this just because
+proc code was old and ghetto.  Another reason we changed all of this was so
+that you could trust the vcoremap while holding the lock.  Otherwise, it's
+actually non-trivial to know the state of a vcore (need to check a combination
+of preempt_served and is_mapped), and even if you do that, there are some
+complications with doing this in the ksched.
+
 5. current_tf and owning_proc
 ===========================
 Originally, current_tf is a per-core macro that returns a struct trapframe *
index ab1d1e4..c89bc2d 100644 (file)
@@ -104,6 +104,7 @@ static void post_vc_msg(struct proc *p, uint32_t vcoreid,
  * sure it is mapped (slight optimization) */
 static void try_notify(struct proc *p, uint32_t vcoreid, int ev_flags)
 {
+       /* Note this is an unlocked-peek at the vcoremap */
        if ((ev_flags & EVENT_IPI) && vcore_is_mapped(p, vcoreid))
                proc_notify(p, vcoreid);
 }
index 4a4366a..b950f41 100644 (file)
@@ -1217,7 +1217,7 @@ uint32_t __proc_preempt_all(struct proc *p, uint32_t *pc_arr)
         * with stale preempt_serveds first.  This might be just as fast anyways. */
        struct vcore *vc_i;
        /* TODO:(BULK) PREEMPT - don't bother with this, set a proc wide flag, or
-        * just make us RUNNABLE_M. */
+        * just make us RUNNABLE_M.  Note this is also used by __map_vcore. */
        TAILQ_FOREACH(vc_i, &p->online_vcs, list)
                vc_i->preempt_served = TRUE;
        return __proc_take_allcores(p, pc_arr, TRUE);
@@ -1235,8 +1235,6 @@ void proc_preempt_core(struct proc *p, uint32_t pcoreid, uint64_t usec)
                return;
        }
        spin_lock(&p->proc_lock);
-       /* TODO: this is racy, could be messages in flight that haven't unmapped
-        * yet, so we need to do something more complicated */
        if (is_mapped_vcore(p, pcoreid)) {
                __proc_preempt_warn(p, get_vcoreid(p, pcoreid), warn_time);
                __proc_preempt_core(p, pcoreid);
@@ -1464,22 +1462,19 @@ void __proc_take_corelist(struct proc *p, uint32_t *pc_arr, uint32_t num,
 {
        struct vcore *vc;
        uint32_t vcoreid;
+       assert(p->state & (PROC_RUNNING_M | PROC_RUNNABLE_M));
        __seq_start_write(&p->procinfo->coremap_seqctr);
        for (int i = 0; i < num; i++) {
                vcoreid = get_vcoreid(p, pc_arr[i]);
                /* Sanity check */
                assert(pc_arr[i] == get_pcoreid(p, vcoreid));
                /* Revoke / unmap core */
-               if (p->state == PROC_RUNNING_M) {
+               if (p->state == PROC_RUNNING_M)
                        __proc_revoke_core(p, vcoreid, preempt);
-               } else {
-                       assert(p->state == PROC_RUNNABLE_M);
-                       __unmap_vcore(p, vcoreid);
-               }
-               /* Change lists for the vcore.  Note, the messages are already in flight
-                * (or the vcore is already unmapped), if applicable.  The only code
-                * that looks at the lists without holding the lock is event code, and
-                * it doesn't care if the vcore was unmapped (it handles that) */
+               __unmap_vcore(p, vcoreid);
+               /* Change lists for the vcore.  Note, the vcore is already unmapped
+                * and/or the messages are already in flight.  The only code that looks
+                * at the lists without holding the lock is event code. */
                vc = vcoreid2vcore(p, vcoreid);
                TAILQ_REMOVE(&p->online_vcs, vc, list);
                /* even for single preempts, we use the inactive list.  bulk preempt is
@@ -1501,18 +1496,16 @@ uint32_t __proc_take_allcores(struct proc *p, uint32_t *pc_arr, bool preempt)
 {
        struct vcore *vc_i, *vc_temp;
        uint32_t num = 0;
+       assert(p->state & (PROC_RUNNING_M | PROC_RUNNABLE_M));
        __seq_start_write(&p->procinfo->coremap_seqctr);
        /* Write out which pcores we're going to take */
        TAILQ_FOREACH(vc_i, &p->online_vcs, list)
                pc_arr[num++] = vc_i->pcoreid;
-       /* Revoke if they are running, o/w unmap.  Both of these need the online
+       /* Revoke if they are running, and unmap.  Both of these need the online
         * list to not be changed yet. */
-       if (p->state == PROC_RUNNING_M) {
+       if (p->state == PROC_RUNNING_M)
                __proc_revoke_allcores(p, preempt);
-       } else {
-               assert(p->state == PROC_RUNNABLE_M);
-               __proc_unmap_allcores(p);
-       }
+       __proc_unmap_allcores(p);
        /* Move the vcores from online to the head of the appropriate list */
        TAILQ_FOREACH_SAFE(vc_i, &p->online_vcs, list, vc_temp) {
                /* TODO: we may want a TAILQ_CONCAT_HEAD, or something that does that */
@@ -1535,13 +1528,16 @@ uint32_t __proc_take_allcores(struct proc *p, uint32_t *pc_arr, bool preempt)
  * calling. */
 void __map_vcore(struct proc *p, uint32_t vcoreid, uint32_t pcoreid)
 {
-       while (p->procinfo->vcoremap[vcoreid].valid)
+       /* Need to spin until __preempt is done saving state and whatnot before we
+        * give the core back out.  Note that __preempt doesn't need the mapping: we
+        * just need to not give out the same vcore (via a __startcore) until the
+        * state is saved so __startcore has something to start. (and spinning in
+        * startcore won't work, since startcore has no versioning). */
+       while (p->procinfo->vcoremap[vcoreid].preempt_served)
                cpu_relax();
        p->procinfo->vcoremap[vcoreid].pcoreid = pcoreid;
-       wmb();
        p->procinfo->vcoremap[vcoreid].valid = TRUE;
        p->procinfo->pcoremap[pcoreid].vcoreid = vcoreid;
-       wmb();
        p->procinfo->pcoremap[pcoreid].valid = TRUE;
 }
 
@@ -1550,7 +1546,6 @@ void __map_vcore(struct proc *p, uint32_t vcoreid, uint32_t pcoreid)
 void __unmap_vcore(struct proc *p, uint32_t vcoreid)
 {
        p->procinfo->pcoremap[p->procinfo->vcoremap[vcoreid].pcoreid].valid = FALSE;
-       wmb();
        p->procinfo->vcoremap[vcoreid].valid = FALSE;
 }
 
@@ -1855,7 +1850,6 @@ void __notify(struct trapframe *tf, uint32_t srcid, long a0, long a1, long a2)
        assert(pcpui->cur_tf);
        assert(!in_kernel(pcpui->cur_tf));
        vcoreid = pcpui->owning_vcoreid;
-       assert(vcoreid == get_vcoreid(p, coreid));
        vcpd = &p->procdata->vcore_preempt_data[vcoreid];
        /* for SCPs that haven't (and might never) call vc_event_init, like rtld.
         * this is harmless for MCPS to check this */
@@ -1893,10 +1887,6 @@ void __preempt(struct trapframe *tf, uint32_t srcid, long a0, long a1, long a2)
        assert(pcpui->cur_tf == &pcpui->actual_tf);
        assert(!in_kernel(pcpui->cur_tf));
        vcoreid = pcpui->owning_vcoreid;
-       assert(vcoreid == get_vcoreid(p, coreid));
-       p->procinfo->vcoremap[vcoreid].preempt_served = FALSE;
-       /* either __preempt or proc_yield() ends the preempt phase. */
-       p->procinfo->vcoremap[vcoreid].preempt_pending = 0;
        vcpd = &p->procdata->vcore_preempt_data[vcoreid];
        printd("[kernel] received __preempt for proc %d's vcore %d on pcore %d\n",
               p->procinfo->pid, vcoreid, coreid);
@@ -1913,8 +1903,10 @@ void __preempt(struct trapframe *tf, uint32_t srcid, long a0, long a1, long a2)
        /* Mark the vcore as preempted and unlock (was locked by the sender). */
        atomic_or(&vcpd->flags, VC_PREEMPTED);
        atomic_and(&vcpd->flags, ~VC_K_LOCK);
-       wmb();  /* make sure everything else hits before we unmap */
-       __unmap_vcore(p, vcoreid);
+       wmb();  /* make sure everything else hits before we finish the preempt */
+       p->procinfo->vcoremap[vcoreid].preempt_served = FALSE;
+       /* either __preempt or proc_yield() ends the preempt phase. */
+       p->procinfo->vcoremap[vcoreid].preempt_pending = 0;
        /* We won't restart the process later.  current gets cleared later when we
         * notice there is no owning_proc and we have nothing to do (smp_idle,
         * restartcore, etc) */
@@ -1932,10 +1924,8 @@ void __death(struct trapframe *tf, uint32_t srcid, long a0, long a1, long a2)
        struct proc *p = pcpui->owning_proc;
        if (p) {
                vcoreid = pcpui->owning_vcoreid;
-               assert(vcoreid == get_vcoreid(p, coreid));
                printd("[kernel] death on physical core %d for process %d's vcore %d\n",
                       coreid, p->pid, vcoreid);
-               __unmap_vcore(p, vcoreid);
                /* We won't restart the process later.  current gets cleared later when
                 * we notice there is no owning_proc and we have nothing to do
                 * (smp_idle, restartcore, etc) */