proc_run() now returns
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 22 Feb 2012 03:07:18 +0000 (19:07 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 27 Feb 2012 21:27:59 +0000 (13:27 -0800)
Previously, it would not return if you were running an _S.  Now, no code
has to worry about it not returning.

This was an artifact of the old implementation that required proc code
to immediately deal with popping user contexts.  Now that we have
current_tf and owning_proc in pcpui, proc_run() acts like
__proc_give_cores() and friends: it just tells the core (via pcpui) to
run you when the core has nothing else to do (smp_idle).

Documentation/process-internals.txt
kern/src/manager.c
kern/src/monitor.c
kern/src/process.c
kern/src/schedule.c

index e7d6c05..f1f07ce 100644 (file)
@@ -113,8 +113,9 @@ stores or makes a copy of the reference.
 ---------------------------
 Refcnting and especially decreffing gets tricky when there are functions that
 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
+proc_run() used to not return, if the core it was called on would pop into
 userspace (if it was a _S, or if the core is part of the vcoremap for a _M).
+This doesn't happen anymore, since we have cur_tf in the per-cpu info.
 
 Functions that MAY not return will "eat" your reference *IF* they do not return.
 This means that you must have a reference when you call them (like always), and
@@ -136,8 +137,9 @@ screw up, and semantically, if the function returns, then we may still have an
 interest in p and should decref later.
 
 The downside is that functions need to determine if they will return or not,
-which can be a pain (a linear time search when running an _M, for instance,
-which can suck if we are trying to use a broadcast/logical IPI).
+which can be a pain (for an out-of-date example: a linear time search when
+running an _M, for instance, which can suck if we are trying to use a
+broadcast/logical IPI).
 
 As the caller, you usually won't know if the function will return or not, so you
 need to provide a consumable reference.  Current doesn't count.  For example,
@@ -155,18 +157,14 @@ had it implicitly be refcnt'd (the fact that it's on the list is enough, sort of
 as if it was part of the +1 for existing), but that complicates things.  For
 instance, it is a source of a reference (for the scheduler) and you could not
 proc_run() a process from the runnable list without worrying about increfing it
-before hand.  Remember that proc_run() might consume your reference (which
-actually turns into a current reference, which is later destroyed by decref in
-abandon_core()).
+before hand.  This isn't true anymore, but the runnable lists are getting
+overhauled anyway.  We'll see what works nicely.
 
 1.7 Internal Details for Specific Functions:
 ---------------------------
 proc_run()/__proc_give_cores(): makes sure enough refcnts are in place for all
 places that will install owning_proc.  This also makes it easier on the system
 (one big incref(n), instead of n increfs of (1) from multiple cores). 
-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.
 
 __set_proc_current() is a helper that makes sure p is the cur_proc.  It will
 incref if installing a new reference to p.  If it removed an old proc, it will
@@ -202,18 +200,20 @@ list or otherwise in the proc system (but not other subsystems)?  In this case,
 proc_run() won't need to eat a reference at all - it will just incref for every
 current it will set up.
 
-A: No: if you pid2proc(), then proc_run() but never return, you have (and lose)
-an extra reference.  We need proc_run() to eat the reference when it does not
-return.  If you decref between pid2proc() and proc_run(), there's a (rare) race
-where the refcnt hits 0 by someone else trying to kill it.  While proc_run()
-will check to see if someone else is trying to kill it, there's a slight chance
-that the struct will be reused and recreated.  It'll probably never happen, but
-it could, and out of principle we shouldn't be referencing memory after it's
-been deallocated.  Avoiding races like this is one of the reasons for our refcnt
-discipline.
-
-Q: Could proc_run() always eat your reference, which would make it easier for
-its implementation?
+New A: Maybe, now that proc_run() returns.
+
+Old A: No: if you pid2proc(), then proc_run() but never return, you have (and
+lose) an extra reference.  We need proc_run() to eat the reference when it
+does not return.  If you decref between pid2proc() and proc_run(), there's a
+(rare) race where the refcnt hits 0 by someone else trying to kill it.  While
+proc_run() will check to see if someone else is trying to kill it, there's a
+slight chance that the struct will be reused and recreated.  It'll probably
+never happen, but it could, and out of principle we shouldn't be referencing
+memory after it's been deallocated.  Avoiding races like this is one of the
+reasons for our refcnt discipline.
+
+Q: (Moot) Could proc_run() always eat your reference, which would make it
+easier for its implementation?
 
 A: Yeah, technically, but it'd be a pain, as mentioned above.  You'd need to
 reaquire a reference via pid2proc, and is rather easy to mess up.
index bf5e6c2..35bddfc 100644 (file)
@@ -110,10 +110,8 @@ char *p_envp[] = {"LD_LIBRARY_PATH=/lib", 0};
 
 void manager_brho(void)
 {
-       static uint8_t RACY progress = 0;       /* this will wrap around. */
-       static struct proc *p;
-       struct file *temp_f;
        static bool first = TRUE;
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
        if (first) {    
                printk("*** Hit shift-g to get into the monitor. ***\n");
@@ -128,19 +126,26 @@ void manager_brho(void)
                }
                process_routine_kmsg(0);
                schedule();
+               /* would like to idle here, but without reset stacks, people will run
+                * off the kstack.  so just idle if we have an owning proc (which we
+                * should then 'restart'). */
+               if (pcpui->owning_proc)
+                       smp_idle();
        }
        /* whatever we do in the manager, keep in mind that we need to not do
         * anything too soon (like make processes), since we'll drop in here during
         * boot if the boot sequence required any I/O (like EXT2), and we need to
         * PRKM() */
-
        assert(0);
-       /* ancient tests below: */
 
+#if 0 /* ancient tests below: (keeping around til we ditch the manager) */
        // for testing taking cores, check in case 1 for usage
        uint32_t corelist[MAX_NUM_CPUS];
        uint32_t num = 3;
+       struct file *temp_f;
+       static struct proc *p;
 
+       static uint8_t RACY progress = 0;       /* this will wrap around. */
        switch (progress++) {
                case 0:
                        printk("Top of the manager to ya!\n");
@@ -188,6 +193,7 @@ void manager_brho(void)
                        envs[0] = kfs_proc_create(kfs_lookup_path("roslib_hello"));
                        __proc_set_state(envs[0], PROC_RUNNABLE_S);
                        proc_run(envs[0]);
+                       warn("DEPRECATED");
                        break;
                        #endif
                case 2:
@@ -217,6 +223,7 @@ void manager_brho(void)
        }
        */
        return;
+#endif
 }
 
 void manager_klueska()
@@ -230,6 +237,7 @@ void manager_klueska()
                //envs[0] = kfs_proc_create(kfs_lookup_path("fillmeup"));
                __proc_set_state(envs[0], PROC_RUNNABLE_S);
                proc_run(envs[0]);
+               warn("DEPRECATED");
        }
        schedule();
 
@@ -243,25 +251,6 @@ void manager_waterman()
        assert(0);
 }
 
-void manager_pearce()
-{
-       static struct proc *envs[256];
-       static volatile uint8_t progress = 0;
-
-       if (progress == 0) {
-               progress++;
-               panic("what do you want to do?");
-               //envs[0] = kfs_proc_create(kfs_lookup_path("parlib_httpserver_integrated"));
-               //envs[0] = kfs_proc_create(kfs_lookup_path("parlib_lock_test"));
-               __proc_set_state(envs[0], PROC_RUNNABLE_S);
-               proc_run(envs[0]);
-       }
-       schedule();
-
-       panic("DON'T PANIC");
-
-}
-
 void manager_yuzhu()
 {
        
index 9a2f637..b6d4903 100644 (file)
@@ -305,10 +305,13 @@ int mon_bin_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        spin_unlock(&p->proc_lock);
        proc_decref(p); /* let go of the reference created in proc_create() */
        kref_put(&program->f_kref);
-       /* Should never return from schedule (env_pop in there) also note you may
-        * not get the process you created, in the event there are others floating
-        * around that are runnable */
+       /* Make a scheduling decision.  You might not get the process you created,
+        * in the event there are others floating around that are runnable */
        schedule();
+       /* want to idle, so we un the process we just selected.  this is a bit
+        * hackish, but so is the monitor. */
+       smp_idle();
+       assert(0);
        return 0;
 }
 
index 30cd213..e68edb2 100644 (file)
@@ -418,28 +418,22 @@ static void __set_proc_current(struct proc *p)
  * RUNNABLE_S, or on its partition in the case of a RUNNABLE_M.  This should
  * never be called to "restart" a core.  This expects that the "instructions"
  * for which core(s) to run this on will be in the vcoremap, which needs to be
- * set externally.
+ * set externally (give_cores()).
  *
- * When a process goes from RUNNABLE_M to RUNNING_M, its vcoremap will be
- * "packed" (no holes in the vcore->pcore mapping), vcore0 will continue to run
- * it's old core0 context, and the other cores will come in at the entry point.
- * Including in the case of preemption.
+ * This will always return, regardless of whether or not the calling core is
+ * being given to a process. (it used to pop the tf directly, before we had
+ * cur_tf).
  *
- * This won't return if the current core is going to be running the process as a
- * _S.  It will return if the process is an _M.  Regardless, proc_run will eat
- * your reference if it does not return. */
+ * Since it always returns, it will never "eat" your reference (old
+ * documentation talks about this a bit). */
 void proc_run(struct proc *p)
 {
        struct vcore *vc_i;
        spin_lock(&p->proc_lock);
-
        switch (p->state) {
                case (PROC_DYING):
                        spin_unlock(&p->proc_lock);
                        printk("Process %d not starting due to async death\n", p->pid);
-                       // if we're a worker core, smp_idle, o/w return
-                       if (!management_core())
-                               smp_idle(); // this never returns
                        return;
                case (PROC_RUNNABLE_S):
                        assert(current != p);
@@ -455,6 +449,8 @@ void proc_run(struct proc *p)
                         * work. */
                        __map_vcore(p, 0, core_id()); // sort of.  this needs work.
                        __seq_end_write(&p->procinfo->coremap_seqctr);
+                       /* incref, since we're saving a reference in current */
+                       proc_incref(p, 1);
                        __set_proc_current(p);
                        /* We restartcore, instead of startcore, since startcore is a bit
                         * lower level and we want a chance to process kmsgs before starting
@@ -465,8 +461,9 @@ void proc_run(struct proc *p)
                        current_tf = &p->env_tf;        /* no need for irq disable yet */
                        /* storing the passed in ref of p in owning_proc */
                        per_cpu_info[core_id()].owning_proc = p;
-                       proc_restartcore();     /* will reenable interrupts */
-                       break;
+                       /* When the calling core idles, it'll call restartcore and run the
+                        * _S process's context. */
+                       return;
                case (PROC_RUNNABLE_M):
                        /* vcoremap[i] holds the coreid of the physical core allocated to
                         * this process.  It is set outside proc_run.  For the kernel
@@ -486,10 +483,7 @@ void proc_run(struct proc *p)
                        } else {
                                warn("Tried to proc_run() an _M with no vcores!");
                        }
-                       /* 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
+                       /* There a subtle race avoidance here.  __proc_startcore can handle
                         * a 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
@@ -497,7 +491,7 @@ void proc_run(struct proc *p)
                         * - Note there is no guarantee this core's interrupts were on, so
                         *   it may not get the message for a while... */
                        spin_unlock(&p->proc_lock);
-                       break;
+                       return;
                default:
                        spin_unlock(&p->proc_lock);
                        panic("Invalid process state %p in proc_run()!!", p->state);
index eac4b03..d91f2c4 100644 (file)
@@ -142,7 +142,7 @@ void schedule(void)
                        /* _S proc, just run it */
                        proc_run(p);
                }
-               /* proc_run will either eat the ref, or we'll decref manually. */
+               /* decref the ref from the TAILQ */
                proc_decref(p);
        } else {
                spin_unlock_irqsave(&runnablelist_lock);