Fixes change_to_vcore failure case
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 Oct 2012 01:00:11 +0000 (18:00 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 Oct 2012 01:02:47 +0000 (18:02 -0700)
sys_change_vcore() could fail for a few reasons.  Previously, userspace
was assuming that if it failed it was because the vcore was already
running.  However, it can fail because the caller is getting preempted.

Changes the syscall to pass back a return value based on the type of
failure.  Userspace now continues to attempt to change until it not
longer is told to try again.

Note that it is not necessary to loop in code like ensure_vcore_runs,
since those functions are already called in busy loops.

kern/include/process.h
kern/src/process.c
kern/src/syscall.c
user/parlib/include/parlib.h
user/parlib/syscall.c
user/parlib/uthread.c

index 85d63e6..adc50e4 100644 (file)
@@ -78,8 +78,8 @@ void proc_yield(struct proc *SAFE p, bool being_nice);
 void proc_notify(struct proc *p, uint32_t vcoreid);
 void proc_wakeup(struct proc *p);
 bool __proc_is_mcp(struct proc *p);
-void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
-                          bool enable_my_notif);
+int proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
+                         bool enable_my_notif);
 
 /* Vcoremap info: */
 uint32_t proc_get_vcoreid(struct proc *p);
index 474b92d..f4d2bfd 100644 (file)
@@ -1715,9 +1715,17 @@ static void __set_curtf_to_vcoreid(struct proc *p, uint32_t vcoreid)
 
 /* Changes calling vcore to be vcoreid.  enable_my_notif tells us about how the
  * state calling vcore wants to be left in.  It will look like caller_vcoreid
- * was preempted.  Note we don't care about notif_pending.  */
-void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
-                          bool enable_my_notif)
+ * was preempted.  Note we don't care about notif_pending.
+ *
+ * Will return:
+ *             0 if we successfully changed to the target vcore.
+ *             -EBUSY if the target vcore is already mapped (a good kind of failure)
+ *             -EAGAIN if we failed for some other reason and need to try again.  For
+ *             example, the caller could be preempted, and we never even attempted to
+ *             change.
+ *             -EINVAL some userspace bug */
+int proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
+                         bool enable_my_notif)
 {
        uint32_t caller_vcoreid, pcoreid = core_id();
        struct per_cpu_info *pcpui = &per_cpu_info[pcoreid];
@@ -1725,6 +1733,11 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        struct vcore *caller_vc, *new_vc;
        struct event_msg preempt_msg = {0};
        int8_t state = 0;
+       int retval = -EAGAIN;   /* by default, try again */
+       /* Need to not reach outside the vcoremap, which might be smaller in the
+        * future, but should always be as big as max_vcores */
+       if (new_vcoreid >= p->procinfo->max_vcores)
+               return -EINVAL;
        /* Need to lock before reading the vcoremap, like in yield.  Need to do this
         * before disabling irqs (deadlock with incoming proc mgmt kmsgs) */
        spin_lock(&p->proc_lock);
@@ -1732,8 +1745,10 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
         * unmapped by a __preempt or __death, like in yield. */
        disable_irqsave(&state);
        /* new_vcoreid is already runing, abort */
-       if (vcore_is_mapped(p, new_vcoreid))
-               goto out_failed;
+       if (vcore_is_mapped(p, new_vcoreid)) {
+               retval = -EBUSY;
+               goto out_locked;
+       }
        /* Need to make sure our vcore is allowed to switch.  We might have a
         * __preempt, __death, etc, coming in.  Similar to yield. */
        switch (p->state) {
@@ -1742,14 +1757,14 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
                case (PROC_RUNNING_S):  /* user bug, just return */
                case (PROC_DYING):              /* incoming __death */
                case (PROC_RUNNABLE_M): /* incoming (bulk) preempt/myield TODO:(BULK) */
-                       goto out_failed;
+                       goto out_locked;
                default:
                        panic("Weird state(%s) in %s()", procstate2str(p->state),
                              __FUNCTION__);
        }
        /* Make sure we're still mapped in the proc. */
        if (!is_mapped_vcore(p, pcoreid))
-               goto out_failed;
+               goto out_locked;
        /* Get all our info */
        caller_vcoreid = get_vcoreid(p, pcoreid);
        assert(caller_vcoreid == pcpui->owning_vcoreid);
@@ -1757,8 +1772,9 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        caller_vc = vcoreid2vcore(p, caller_vcoreid);
        /* Should only call from vcore context */
        if (!caller_vcpd->notif_disabled) {
+               retval = -EINVAL;
                printk("[kernel] You tried to change vcores from uthread ctx\n");
-               goto out_failed;
+               goto out_locked;
        }
        /* Sanity check, can remove after a while (we should have been unmapped) */
        assert(!caller_vc->preempt_served);
@@ -1807,10 +1823,12 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        send_kernel_event(p, &preempt_msg, new_vcoreid);
        /* Change cur_tf so we'll be the new vcoreid */
        __set_curtf_to_vcoreid(p, new_vcoreid);
-       /* Fall through to exit (we didn't fail) */
-out_failed:
+       retval = 0;
+       /* Fall through to exit */
+out_locked:
        spin_unlock(&p->proc_lock);
        enable_irqsave(&state);
+       return retval;
 }
 
 /* Kernel message handler to start a process's context on this core, when the
index 3bf53ef..fd2c382 100644 (file)
@@ -382,21 +382,12 @@ static int sys_proc_yield(struct proc *p, bool being_nice)
        assert(0);
 }
 
-static void sys_change_vcore(struct proc *p, uint32_t vcoreid,
+static int sys_change_vcore(struct proc *p, uint32_t vcoreid,
                              bool enable_my_notif)
 {
-       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       /* Change to vcore may start the vcore up remotely before we can finish the
-        * async syscall, so we need to finish the sysc and not touch the struct.
-        * Note this sysc has no return value. */
-       finish_sysc(pcpui->cur_sysc, pcpui->cur_proc);
-       pcpui->cur_sysc = 0;    /* don't touch sysc again */
-       proc_change_to_vcore(p, vcoreid, enable_my_notif);
-       /* Shouldn't return, to prevent the chance of mucking with cur_sysc.
-        * smp_idle will make sure we run the appropriate cur_tf (which will be the
-        * new vcore for successful calls). */
-       smp_idle();
-       assert(0);
+       /* Note retvals can be negative, but we don't mess with errno in case
+        * callers use this in low-level code and want to extract the 'errno'. */
+       return proc_change_to_vcore(p, vcoreid, enable_my_notif);
 }
 
 static ssize_t sys_fork(env_t* e)
index 28a836d..20c876e 100644 (file)
@@ -50,7 +50,7 @@ int         sys_self_notify(uint32_t vcoreid, unsigned int ev_type,
 int         sys_halt_core(unsigned int usec);
 void*          sys_init_arsc();
 int         sys_block(unsigned int usec);
-void        sys_change_vcore(uint32_t vcoreid, bool enable_my_notif);
+int         sys_change_vcore(uint32_t vcoreid, bool enable_my_notif);
 int         sys_change_to_m(void);
 int         sys_poke_ksched(int res_type);
 
index 12157e6..08bcedb 100644 (file)
@@ -137,10 +137,18 @@ int sys_block(unsigned int usec)
  * get started from vcore_entry() or not, and whether or not remote cores need
  * to sys_change_vcore to preempt-recover the calling vcore.  Only set this to
  * FALSE if you are unable to handle starting fresh at vcore_entry().  One
- * example of this is in mcs_pdr_locks */
-void sys_change_vcore(uint32_t vcoreid, bool enable_my_notif)
-{
-       ros_syscall(SYS_change_vcore, vcoreid, enable_my_notif, 0, 0, 0, 0);
+ * example of this is in mcs_pdr_locks.
+ *
+ * Will return:
+ *             0 if we successfully changed to the target vcore.
+ *             -EBUSY if the target vcore is already mapped (a good kind of failure)
+ *             -EAGAIN if we failed for some other reason and need to try again.  For
+ *             example, the caller could be preempted, and we never even attempted to
+ *             change.
+ *             -EINVAL some userspace bug */
+int sys_change_vcore(uint32_t vcoreid, bool enable_my_notif)
+{
+       return ros_syscall(SYS_change_vcore, vcoreid, enable_my_notif, 0, 0, 0, 0);
 }
 
 int sys_change_to_m(void)
index 77f1576..85be6e7 100644 (file)
@@ -597,6 +597,16 @@ static void stop_uth_stealing(struct preempt_data *vcpd)
                             old_flags & ~VC_UTHREAD_STEALING));
 }
 
+/* Helper.  Will ensure a good attempt at changing vcores, meaning we try again
+ * if we failed for some reason other than the vcore was already running. */
+static void __change_vcore(uint32_t rem_vcoreid, bool enable_my_notif)
+{
+       /* okay to do a normal spin/relax here, even though we are in vcore
+        * context. */
+       while (-EAGAIN == sys_change_vcore(rem_vcoreid, enable_my_notif))
+               cpu_relax();
+}
+
 /* Helper, used in preemption recovery.  When you can freely leave vcore
  * context and need to change to another vcore, call this.  vcpd is the caller,
  * rem_vcoreid is the remote vcore.  This will try to package up your uthread.
@@ -614,7 +624,7 @@ static void change_to_vcore(struct preempt_data *vcpd, uint32_t rem_vcoreid)
                 * once they 0'd it, we should be good to yield.  just a bit dangerous.
                 * */
                were_handling_remotes = ev_might_not_return();
-               sys_change_vcore(rem_vcoreid, TRUE);    /* noreturn on success */
+               __change_vcore(rem_vcoreid, TRUE);      /* noreturn on success */
                goto out_we_returned;
        }
        /* Note that the reason we need to check STEALING is because we can get into
@@ -632,7 +642,7 @@ static void change_to_vcore(struct preempt_data *vcpd, uint32_t rem_vcoreid)
         * progress by doing a sys_change_vcore(). */
        /* Crap, someone is stealing (unlikely).  All we can do is change. */
        if (atomic_read(&vcpd->flags) & VC_UTHREAD_STEALING) {
-               sys_change_vcore(rem_vcoreid, FALSE);   /* returns on success */
+               __change_vcore(rem_vcoreid, FALSE);     /* returns on success */
                return;
        }
        cmb();
@@ -640,7 +650,7 @@ static void change_to_vcore(struct preempt_data *vcpd, uint32_t rem_vcoreid)
         * VC_UTHREAD_STEALING. */
        if (!current_uthread) {
                were_handling_remotes = ev_might_not_return();
-               sys_change_vcore(rem_vcoreid, TRUE);    /* noreturn on success */
+               __change_vcore(rem_vcoreid, TRUE);      /* noreturn on success */
                goto out_we_returned;
        }
        /* Need to make sure we don't have a DONT_MIGRATE (very rare, someone would
@@ -648,7 +658,7 @@ static void change_to_vcore(struct preempt_data *vcpd, uint32_t rem_vcoreid)
         * to finish stealing (and fail) fast enough for us to miss the previous
         * check). */
        if (current_uthread->flags & UTHREAD_DONT_MIGRATE) {
-               sys_change_vcore(rem_vcoreid, FALSE);   /* returns on success */
+               __change_vcore(rem_vcoreid, FALSE);     /* returns on success */
                return;
        }
        /* Now save our uthread and restart them */
@@ -656,7 +666,7 @@ static void change_to_vcore(struct preempt_data *vcpd, uint32_t rem_vcoreid)
        __uthread_pause(vcpd, current_uthread);
        current_uthread = 0;
        were_handling_remotes = ev_might_not_return();
-       sys_change_vcore(rem_vcoreid, TRUE);            /* noreturn on success */
+       __change_vcore(rem_vcoreid, TRUE);              /* noreturn on success */
        /* Fall-through to out_we_returned */
 out_we_returned:
        ev_we_returned(were_handling_remotes);