Protects cur_tf by disabling interrupts
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 28 Sep 2011 20:58:06 +0000 (13:58 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:07 +0000 (17:36 -0700)
Disable interrupts to keep cur_tf or its contents from changing out from
under you.  This doesn't happen yet, since all kmsgs that change cur_tf
are still routine.

proc_yield() needs to be careful due to concurrent notifications.
proc_yield() is a lot like the other blocks of code that adjust a
process's state on a core (__startcore, __preempt, etc): it should be
run with interrupts disabled to avoid any interleavings.

The changes to _S code (fork/exec) are there just in case we muck with
cur_tf in the future.

kern/src/process.c
kern/src/resource.c
kern/src/smp.c
kern/src/syscall.c

index 44eca02..47a77b5 100644 (file)
@@ -529,7 +529,7 @@ void proc_run(struct proc *p)
                         * lower level and we want a chance to process kmsgs before starting
                         * the process. */
                        spin_unlock(&p->proc_lock);
-                       current_tf = &p->env_tf;
+                       current_tf = &p->env_tf;        /* no need for irq disable yet */
                        proc_restartcore();
                        break;
                case (PROC_RUNNABLE_M):
@@ -603,7 +603,7 @@ static void __proc_startcore(struct proc *p, trapframe_t *tf)
        if (p->state == PROC_RUNNING_S)
                env_pop_ancillary_state(p);
        /* Clear the current_tf, since it is no longer used */
-       current_tf = 0;
+       current_tf = 0; /* TODO: might not need this... */
        env_pop_tf(tf);
 }
 
@@ -622,26 +622,15 @@ void proc_restartcore(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        assert(!pcpui->cur_sysc);
+       /* Need ints disabled when we return from processing (race on missing
+        * messages/IPIs) */
+       disable_irq();
+       process_routine_kmsg(pcpui->cur_tf);
        /* If there is no cur_tf, it is because the old one was already restarted
         * (and we weren't interrupting another one to finish).  In which case, we
         * should just smp_idle() */
        if (!pcpui->cur_tf) {
-               /* It is possible for us to have current loaded if a kthread restarted
-                * after the process yielded the core. */
-               abandon_core();
-               smp_idle();
-       }
-       /* Need ints disabled when we return from processing (race) */
-       disable_irq();
-       /* Need to be current (set by the caller), in case a kmsg is there that
-        * tries to clobber us. */
-       process_routine_kmsg(pcpui->cur_tf);
-       cmb();
-       /* We need to re-evaluate whether or not we should run something, in case a
-        * __death or __preempt came in since we last checked. */
-       if (!pcpui->cur_tf) {
                abandon_core();
-               enable_irq();
                smp_idle();
        }
        __proc_startcore(pcpui->cur_proc, pcpui->cur_tf);
@@ -785,13 +774,19 @@ void __proc_yield_s(struct proc *p, struct trapframe *tf)
  * (which needs to be checked).  If there is no preemption pending, just return.
  * No matter what, don't adjust the number of cores wanted.
  *
- * This usually does not return (abandon_core()), so it will eat your reference.
- * */
+ * This usually does not return (smp_idle()), so it will eat your reference.
+ * Also note that it needs a non-current/edible reference, since it will abandon
+ * and continue to use the *p (current == 0, no cr3, etc).
+ *
+ * We disable interrupts for most of it too, since we need to protect current_tf
+ * and not race with __notify (which doesn't play well with concurrent
+ * yielders). */
 void proc_yield(struct proc *SAFE p, bool being_nice)
 {
        uint32_t vcoreid = get_vcoreid(p, core_id());
        struct vcore *vc = vcoreid2vcore(p, vcoreid);
        struct preempt_data *vcpd = &p->procdata->vcore_preempt_data[vcoreid];
+       int8_t state = 0;
 
        /* no reason to be nice, return */
        if (being_nice && !vc->preempt_pending)
@@ -820,6 +815,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                spin_unlock(&p->proc_lock);
                return;
        }
+       disable_irqsave(&state);
        switch (p->state) {
                case (PROC_RUNNING_S):
                        __proc_yield_s(p, current_tf);  /* current_tf 0'd in abandon core */
@@ -833,10 +829,13 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                        /* Now that we're off the online list, check to see if an alert made
                         * it through (event.c sets this) */
                        wrmb(); /* prev write must hit before reading notif_pending */
+                       /* 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 */
                                TAILQ_INSERT_TAIL(&p->online_vcs, vc, list); /* could go HEAD */
                                spin_unlock(&p->proc_lock);
+                               enable_irqsave(&state);
                                return;
                        }
                        /* We won the race with event sending, we can safely yield */
@@ -862,6 +861,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                case (PROC_DYING):
                        /* just return and take the death message (which should be otw) */
                        spin_unlock(&p->proc_lock);
+                       enable_irqsave(&state);
                        return;
                default:
                        // there are races that can lead to this (async death, preempt, etc)
@@ -873,8 +873,12 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
        /* TODO: (RMS) If there was a change to the idle cores, try and give our
         * core to someone who was preempted. */
        /* Clean up the core and idle.  For mgmt cores, they will ultimately call
-        * manager, which will call schedule() and will repick the yielding proc. */
+        * manager, which will call schedule() and will repick the yielding proc.
+        *
+        * Need to do this before enabling interrupts, since once we put_idle_core()
+        * and unlock, we could get a startcore. */
        abandon_core();
+       enable_irqsave(&state); /* arguably unnecessary, smp_idle() will recheck */
        smp_idle();
 }
 
@@ -1367,18 +1371,21 @@ void __unmap_vcore(struct proc *p, uint32_t vcoreid)
        p->procinfo->vcoremap[vcoreid].valid = FALSE;
 }
 
-/* Stop running whatever context is on this core, load a known-good cr3, and
- * 'idle'.  Note this leaves no trace of what was running. This "leaves the
- * process's context. */
+/* Stop running whatever context is on this core and load a known-good cr3.
+ * Note this leaves no trace of what was running. This "leaves the process's
+ * context. */
 void abandon_core(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       int8_t state = 0;
        /* Syscalls that don't return will ultimately call abadon_core(), so we need
         * to make sure we don't think we are still working on a syscall. */
        pcpui->cur_sysc = 0;
        if (pcpui->cur_proc) {
-               pcpui->cur_tf = 0;
+               disable_irqsave(&state);
+               pcpui->cur_tf = 0;      /* shouldn't have one of these without a proc btw */
                __abandon_core();
+               enable_irqsave(&state);
        }
 }
 
index 4e64983..ffa9cd9 100644 (file)
@@ -42,6 +42,7 @@ ssize_t core_request(struct proc *p)
        uint32_t corelist[MAX_NUM_CPUS];
        bool need_to_idle = FALSE;
        bool self_ipi_pending = FALSE;
+       int8_t state = 0;
 
        spin_lock(&p->proc_lock);
        if (p->state == PROC_DYING) {
@@ -54,9 +55,11 @@ ssize_t core_request(struct proc *p)
                printk("[kernel] trying to transition _M -> _S (deprecated)!\n");
                assert(p->state == PROC_RUNNING_M); // TODO: (ACR) async core req
                /* save the context, to be restarted in _S mode */
+               disable_irqsave(&state);        /* protect cur_tf */
                assert(current_tf);
                p->env_tf = *current_tf;
                current_tf = 0;                 /* Make sure it isn't used in the future */
+               enable_irqsave(&state);
                env_push_ancillary_state(p); // TODO: (HSS)
                /* sending death, since it's not our job to save contexts or anything in
                 * this case.  also, if this returns true, we will not return down
@@ -121,8 +124,10 @@ ssize_t core_request(struct proc *p)
                                 * it in the preempt slot so that we can also save the silly
                                 * state. */
                                struct preempt_data *vcpd = &p->procdata->vcore_preempt_data[0];
+                               disable_irqsave(&state);        /* protect cur_tf */
                                assert(current_tf);
                                vcpd->preempt_tf = *current_tf;
+                               enable_irqsave(&state);
                                save_fp_state(&vcpd->preempt_anc);
                                __seq_start_write(&vcpd->preempt_tf_valid);
                                /* If we remove this, vcore0 will start where the _S left off */
index 5591440..b4f95e7 100644 (file)
@@ -31,13 +31,14 @@ static void try_run_proc(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
+       disable_irq();
        /* There was a process running here, and we should return to it */
        if (pcpui->cur_tf) {                    /* aka, current_tf */
                assert(pcpui->cur_proc);        /* aka, current */
                proc_restartcore();
                assert(0);
-       } else if (pcpui->cur_proc) {
-               abandon_core();
+       } else {
+               assert(!pcpui->cur_proc);
        }
 }
 
@@ -55,6 +56,8 @@ static void __smp_idle(void)
 {
        int8_t state = 0;
 
+       /* TODO: idle, abandon_core(), and proc_restartcore() need cleaned up */
+       enable_irq();   /* get any IRQs before we halt later */
        try_run_proc();
        /* if we made it here, we truly want to idle */
        /* in the future, we may need to proactively leave process context here.
index e135f0d..ed68fe2 100644 (file)
@@ -375,6 +375,7 @@ static int sys_proc_yield(struct proc *p, bool being_nice)
 
 static ssize_t sys_fork(env_t* e)
 {
+       int8_t state = 0;
        // TODO: right now we only support fork for single-core processes
        if (e->state != PROC_RUNNING_S) {
                set_errno(EINVAL);
@@ -386,12 +387,14 @@ static ssize_t sys_fork(env_t* e)
 
        env->heap_top = e->heap_top;
        env->ppid = e->pid;
+       disable_irqsave(&state);        /* protect cur_tf */
        /* Can't really fork if we don't have a current_tf to fork */
        if (!current_tf) {
                set_errno(EINVAL);
                return -1;
        }
        env->env_tf = *current_tf;
+       enable_irqsave(&state);
 
        /* We need to speculatively say the syscall worked before copying the memory
         * out, since the 'forked' process's call never actually goes through the
@@ -482,7 +485,7 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
        char *t_path;
        struct file *program;
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
-       struct trapframe *old_cur_tf = pcpui->cur_tf;
+       int8_t state = 0;
 
        /* We probably want it to never be allowed to exec if it ever was _M */
        if (p->state != PROC_RUNNING_S) {
@@ -493,21 +496,27 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
                set_errno(EINVAL);
                return -1;
        }
+       /* Copy in the path.  Consider putting an upper bound on path_l. */
+       t_path = user_strdup_errno(p, path, path_l);
+       if (!t_path)
+               return -1;
+       disable_irqsave(&state);        /* protect cur_tf */
        /* Can't exec if we don't have a current_tf to restart (if we fail).  This
         * isn't 100% true, but I'm okay with it. */
-       if (!old_cur_tf) {
+       if (!pcpui->cur_tf) {
+               enable_irqsave(&state);
                set_errno(EINVAL);
                return -1;
        }
-       /* Copy in the path.  Consider putting an upper bound on path_l. */
-       t_path = user_strdup_errno(p, path, path_l);
-       if (!t_path)
-               return -1;
+       /* Preemptively copy out the cur_tf, in case we fail later (easier on cur_tf
+        * if we do this now) */
+       p->env_tf = *pcpui->cur_tf;
        /* Clear the current_tf.  We won't be returning the 'normal' way.  Even if
         * we want to return with an error, we need to go back differently in case
         * we succeed.  This needs to be done before we could possibly block, but
         * unfortunately happens before the point of no return. */
        pcpui->cur_tf = 0;
+       enable_irqsave(&state);
        /* This could block: */
        program = do_file_open(t_path, 0, 0);
        user_memdup_free(p, t_path);
@@ -550,7 +559,6 @@ mid_error:
         * error value (errno is already set). */
        kref_put(&program->f_kref);
 early_error:
-       p->env_tf = *old_cur_tf;
        finish_current_sysc(-1);
 success:
        /* Here's how we'll restart the new (or old) process: */
@@ -561,7 +569,7 @@ success:
        spin_unlock(&p->proc_lock);
 all_out:
        /* we can't return, since we'd write retvals to the old location of the
-        * sycall struct (which has been freed and is in the old userspace) (or has
+        * syscall struct (which has been freed and is in the old userspace) (or has
         * already been written to).*/
        abandon_core();
        smp_idle();