Fixes race with vcore_yield()
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 29 Apr 2013 22:22:58 +0000 (15:22 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 29 Apr 2013 22:22:58 +0000 (15:22 -0700)
In some scenarios, the kernel would think we wanted <= 0 cores
(negatives triggered the > max_vcores() check), if we were yielding
repeatedly while messages arrived.

kern/include/ros/resource.h
kern/src/process.c
kern/src/schedule.c
user/parlib/uthread.c
user/parlib/vcore.c

index fa5f412..5d1c8e2 100644 (file)
 
 #include <ros/common.h>
 
-/* A request means to set the amt_wanted to X.  Any changes result in prodding
- * the scheduler / whatever.
- *
- * To make these requests, userspace uses SYS_resource_req, which currently is a
- * syscall to make one request.
- *
- * Another way would be to take a ptr to a resource req and length, to batch
- * requests together.  Individual syscalls are simpler than the batch.  For
- * example,  servicing the core request doesn't easily return (which could lead
- * to other requests getting ignored, or us having to worry about the
- * order of processing).  Dealing with more than one request per type could be a
- * pain too.  The batch one is nice, since it amortizes the overhead of the syscall,
- * but it doesn't really matter that much, esp when there are only a few resources.
- *
- * amt_wanted_min is the least amount you are will to run with.
- *
- * A few caveats for cores:
- * - when someone yields (esp if the wish > grant): yielding means take one
- *   away, and set wished = current.  don't yield if you want another core still
- * - if someone requests less cores than they currently have active, we'll set
- *   their wish to their active and return an error code (no core allocation
- *   changes either).
- */
-
 /* Types of resource requests */
 #define RES_CORES                       0
 #define RES_MEMORY                      1
index 04594fb..1983cef 100644 (file)
@@ -1074,7 +1074,8 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
        /* Note we need interrupts disabled, since a __notify can come in
         * and set pending to FALSE */
        if (vcpd->notif_pending) {
-               /* We lost, put it back on the list and abort the yield */
+               /* We lost, put it back on the list and abort the yield.  If we ever
+                * build an myield, we'll need a way to deal with this for all vcores */
                TAILQ_INSERT_TAIL(&p->online_vcs, vc, list); /* could go HEAD */
                goto out_failed;
        }
index 9fd3051..93f715b 100644 (file)
@@ -287,10 +287,7 @@ void __sched_mcp_wakeup(struct proc *p)
                spin_unlock(&sched_lock);
                return;
        }
-       /* could try and prioritize p somehow (move it to the front of the list).
-        * for now, just help them out a bit (mild help here, can remove this) */
-       if (!p->procdata->res_req[RES_CORES].amt_wanted)
-               p->procdata->res_req[RES_CORES].amt_wanted = 1;
+       /* could try and prioritize p somehow (move it to the front of the list). */
        spin_unlock(&sched_lock);
        /* note they could be dying at this point too. */
        poke(&ksched_poker, p);
@@ -405,6 +402,18 @@ static uint32_t get_cores_needed(struct proc *p)
        /* Help them out - if they ask for something impossible, give them 1 so they
         * can make some progress. (this is racy, and unnecessary). */
        if (amt_wanted > p->procinfo->max_vcores) {
+               printk("[kernel] proc %d wanted more than max, wanted %d\n", p->pid,
+                      amt_wanted);
+               p->procdata->res_req[RES_CORES].amt_wanted = 1;
+               amt_wanted = 1;
+       }
+       /* There are a few cases where amt_wanted is 0, but they are still RUNNABLE
+        * (involving yields, events, and preemptions).  In these cases, give them
+        * at least 1, so they can make progress and yield properly.  If they are
+        * not WAITING, they did not yield and may have missed a message. */
+       if (!amt_wanted) {
+               /* could ++, but there could be a race and we don't want to give them
+                * more than they ever asked for (in case they haven't prepped) */
                p->procdata->res_req[RES_CORES].amt_wanted = 1;
                amt_wanted = 1;
        }
index a2d0433..21ac6d0 100644 (file)
@@ -70,7 +70,10 @@ void uthread_lib_init(struct uthread *uthread)
         * send the messages, and does not necessarily provide storage space for the
         * messages.  What we're doing is saying that all PREEMPT and CHECK_MSGS
         * events should be spammed to vcores that are running, preferring whatever
-        * the kernel thinks is appropriate.  And IPI them. */
+        * the kernel thinks is appropriate.  And IPI them.
+        *
+        * It is critical that these are either SPAM_PUB or INDIR|FALLBACK, so that
+        * yielding vcores do not miss the preemption messages. */
        ev_handlers[EV_VCORE_PREEMPT] = handle_vc_preempt;
        ev_handlers[EV_CHECK_MSGS] = handle_vc_indir;
        preempt_ev_q = get_event_q();   /* small ev_q, mostly a vehicle for flags */
index dafec35..3e53ea8 100644 (file)
@@ -283,6 +283,7 @@ try_handle_it:
  * care what other code does - we intend to set those flags no matter what. */
 void vcore_yield(bool preempt_pending)
 {
+       unsigned long old_nr;
        uint32_t vcoreid = vcore_id();
        struct preempt_data *vcpd = vcpd_of(vcoreid);
        __sync_fetch_and_and(&vcpd->flags, ~VC_CAN_RCV_MSG);
@@ -298,14 +299,36 @@ void vcore_yield(bool preempt_pending)
                return;
        }
        /* If we are yielding since we don't want the core, tell the kernel we want
-        * one less vcore.  If yield fails (slight race), we may end up having more
-        * vcores than amt_wanted for a while, and might lose one later on (after a
+        * one less vcore (vc_yield assumes a dumb 2LS).
+        *
+        * If yield fails (slight race), we may end up having more vcores than
+        * amt_wanted for a while, and might lose one later on (after a
         * preempt/timeslicing) - the 2LS will have to notice eventually if it
-        * actually needs more vcores (which it already needs to do).  We need to
-        * atomically decrement, though I don't want the kernel's data type here to
-        * be atomic_t (only userspace cares in this one case). */
-       if (!preempt_pending)
-               __sync_fetch_and_sub(&__procdata.res_req[RES_CORES].amt_wanted, 1);
+        * actually needs more vcores (which it already needs to do).  amt_wanted
+        * could even be 0.
+        *
+        * In general, any time userspace decrements or sets to 0, it could get
+        * preempted, so the kernel will still give us at least one, until the last
+        * vcore properly yields without missing a message (and becomes a WAITING
+        * proc, which the ksched will not give cores to).
+        *
+        * I think it's possible for userspace to do this (lock, read amt_wanted,
+        * check all message queues for all vcores, subtract amt_wanted (not set to
+        * 0), unlock) so long as every event handler +1s the amt wanted, but that's
+        * a huge pain, and we already have event handling code making sure a
+        * process can't sleep (transition to WAITING) if a message arrives (can't
+        * yield if notif_pending, can't go WAITING without yielding, and the event
+        * posting the notif_pending will find the online VC or be delayed by
+        * spinlock til the proc is WAITING). */
+       if (!preempt_pending) {
+               do {
+                       old_nr = __procdata.res_req[RES_CORES].amt_wanted;
+                       if (old_nr == 0)
+                               break;
+               } while (!__sync_bool_compare_and_swap(
+                            &__procdata.res_req[RES_CORES].amt_wanted,
+                            old_nr, old_nr - 1));
+       }
        /* We can probably yield.  This may pop back up if notif_pending became set
         * by the kernel after we cleared it and we lost the race. */
        sys_yield(preempt_pending);