Process reference counting
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 1 Dec 2009 07:28:11 +0000 (23:28 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 1 Dec 2009 07:28:11 +0000 (23:28 -0800)
This sorts out reference counting of processes.  This will keep
processes from dying while pointers are still floating around, among
other things.  Read the section in Documentation/process-internals.txt
for specifics and before you do anything that fundamentally messes with
processes.

There are still many outstanding issues: using atomics to properly
handle the reference counting (currently using the proc_lock, which has
issues), splitting core_request() up, and stacks getting annihilated
while holding references.

13 files changed:
Documentation/process-internals.txt [new file with mode: 0644]
kern/arch/i386/trap.c
kern/arch/sparc/trap.c
kern/include/process.h
kern/src/env.c
kern/src/manager.c
kern/src/monitor.c
kern/src/process.c
kern/src/resource.c
kern/src/schedule.c
kern/src/syscall.c
kern/src/testing.c
user/apps/roslib/mproctests.c

diff --git a/Documentation/process-internals.txt b/Documentation/process-internals.txt
new file mode 100644 (file)
index 0000000..cec4b5f
--- /dev/null
@@ -0,0 +1,314 @@
+This discusses core issues with process design and implementation.  Most of this
+info is available in the source in the comments (but may not be in the future).
+For now, it's a dumping ground for topics that people ought to understand before
+they muck with how processes work.
+
+1. Reference Counting
+===========================
+1.1 Basics:
+---------------------------
+Reference counts (proc_refcnt) exist to keep a process alive.  Eventually, this
+will probably turn into a regular "kernel design pattern", like it is in Linux
+(http://lwn.net/Articles/336224/).  The style of reference counting we use for
+processes is similar to a kref:
+- Can only incref if the current value is greater than 0, meaning there is
+  already a reference to it.  It is a bug to try to incref on something that has
+  no references, so always make sure you incref something that you know has a
+  reference.  If you don't know, you need to get it manually (CAREFULLY!) or use
+  pid2proc (which is a careful way of doing this).  If you incref and there are
+  0 references, the kernel will panic.  Fix your bug / don't incref random
+  pointers.
+- Can always decref.
+- When the decref returns 0, perform some operation.  This does some final
+  cleanup on the object.
+
+For a process, proc_destroy() decrefs, and other codes using the proc also
+decref.  The last one to decref calls proc_free to do the final cleanup.
+
+1.2 Brief History of the Refcnt:
+---------------------------
+Originally, the refcnt was created to keep page tables from being destroyed (in
+proc_free()) while cores were still using them, which is what was happens during
+an ARSC (async remote syscall).  It was then defined to be a count of places in
+the kernel that had an interest in the process staying alive, practically just
+to protect current/cr3.  This 'interest' actually extends to any code holding a
+pointer to the proc, such as one acquired via pid2proc(), which is its current
+meaning.
+
+1.3 Quick Aside: The current Macro:
+---------------------------
+current is a pointer to the proc that is currently loaded/running on any given
+core.  It is stored in the per_cpu_info struct, and set/managed by low-level
+process code.  It is necessary for the kernel to quickly figure out who is
+running on its code, especially when servicing interrupts and traps.  current is
+protected by a refcnt.
+
+1.4 Reference Counting Rules:
+---------------------------
++1 for existing.
+- The fact that the process is supposed to exist is worth +1.  When it is time
+  to die, we decref, and it will eventually be cleaned up.
+
++1 for someone using it or planning to use it.
+- This includes simply having a pointer to the proc, since presumably you will
+  use it.  pid2proc() will incref for you.  When you are done, decref.
+- Functions that create a process and return a pointer (like proc_create() or
+  kfs_proc_create()) will also up the refcnt for you.  Decref when you're done.
+- If the *proc is stored somewhere where it will be used again, such as in an IO
+  continuation, it needs to be refcnt'd.  Note that if you already had a
+  reference from pid2proc(), simply don't decref after you store the pointer.
+
++1 for current.
+- current counts as someone using it (expressing interest in the core), but is
+  also a source of the pointer, so its a bit different.
+- You have a reference from current and can use it without refcnting, but
+  anything that needs to eat a reference or store/use it needs an incref first.
+  To be clear, your reference is *NOT* edible.  It protects the cr3, guarantees
+  the process won't die, and serves as a bootstrappable reference.
+- Specifically, if you get a ref from current, but then save it somewhere (like
+  an IO continuation request), then clearly you must incref, since it's both
+  current and stored/used.
+- If you don't know what might be downstream from your function, then incref
+  before passing the reference, and decref when it returns.  Like when handling
+  syscalls, for example.
+
+All functions that take a *proc have a refcnt'd reference, though it may not be
+edible (it could be current).  It is the callers responsibility to make sure
+it'd edible if it necessary.  It is the callees responsibility to incref if it
+stores or makes a copy of the reference.
+
+1.5 Functions That Don't or Might Not Return:
+---------------------------
+Refcnting and especially decreffing gets tricky when there are functions that
+MAY not return.  proc_startcore() does not return (it pops into userspace).
+proc_run() might not return, if the core it was called on will pop into
+userspace (if it was a _S, or if the core is part of the vcoremap for a _M).
+
+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
+that reference will be consumed / decref'd for you if the function doesn't
+return.  Or something similarly appropriate.
+
+We do this because when the function does not return, you will not have the
+chance to decref (your decref code will never run).  We need the reference when
+going in to keep the object alive (like with any other refcnt).  We can't have
+the function always eat the reference, since you cannot simply re-incref the
+pointer (not allowed to incref unless you know you had a good reference).  You'd
+have to do something like p = pid2proc(p_pid);  It's clunky to do that, easy to
+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).
+
+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,
+proc_run() requires a reference.  You can proc_run(p), and use p afterwards, and
+later decref.  You need to make sure you have a reference, so things like
+proc_run(pid2proc(55)) works, since pid2proc() increfs for you.  But you cannot
+proc_run(current), unless you incref current in advance.  Incidentally,
+proc_running current doesn't make a lot of sense.
+
+As another example, proc_startcore() will take your reference and store it
+in current.  Since it is used by both the __startcore and the interrupt return
+paths, we're currently going with the model of "caller makes sure there is a ref
+for current".  Check its comments for details.
+
+1.6 Runnable List:
+---------------------------
+Procs on the runnable list need to have a refcnt (other than the +1 for
+existing).  It's something that cares that the process exists.  We could have
+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()).
+
+1.7 Internal Details for Specific Functions:
+---------------------------
+proc_run(): makes sure enough refcnts are in place for all places that will
+install current.  This also makes it easier on the system (one big incref(n),
+instead of n increfs of (1) from multiple cores).  In the off chance current was
+already set for a core receiving the active message, __startcore will decref.
+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.
+
+proc_startcore(): assumes all references to p are sorted.  *p is already
+accounted for as if it was current on the core startcore runs on. (there is only
+one refcnt for both *p and current, not 2 separate ones).
+
+proc_destroy(): it might not return (if the calling core belongs to the
+process), so it may eat your reference and you must have an edible reference.
+It is possible you called proc_destroy(current).  The cleanup of the current
+will be its own decref, so you need to have a usable/real reference (current
+doesn't count as an edible reference).  So incref before doing that.  Even if p
+== current, proc_destroy() can't tell if you sent it p (and had a reference) or
+current and didn't.
+
+proc_yield(): this never returns, so it eats your reference.  It will also
+decref when it abandon_core()s.
+
+__proc_give_cores() and friends: you call this while holding the lock, but it is
+possible that your core is in the corelist you gave it.  In this case, it will
+detect it, and return a bool signalling if an IPI is pending.  It will not
+consume your reference.  The reasoning behind this is that it is an internal
+function, and you may want to do other things before decreffing.  There is also
+a helper function that will unlock and possibly decref/wait for the IPI, called
+__proc_unlock_ipi_pending().  Use this when it is time to unlock.  It's just a
+helper, which may go away.
+
+abandon_core(): it was not given a reference, so it doesn't eat one.  It will
+decref when it unloads the cr3.  Note that this is a potential performance
+issue.  When preempting or killing, there are n cores that are fighting for the
+cacheline to decref.  An alternative would be to have one core decref for all n
+cores, after it knows all cores unloaded the cr3.  This would be a good use of
+the checklist (possibly with one cacheline per core).  It would take a large
+amount of memory for better scalability.
+
+1.8 Core Request:
+---------------------------
+core_request() is run outside of the process code (for now), though it is fairly
+intricate.  It's another function that might not return, but the reasons for
+this vary:
+       1: The process is moving from _S to _M so the return path to userspace won't
+       happen (and sort of will on the new core / the other side), but that will
+       happen when popping into userspace.
+       2: The scheduler is giving the current core to the process, which can kick
+       in via either proc_run() or __proc_give_cores().
+       3: It was a request to give up all cores, which means the current core will
+       receive an IPI (if it wasn't an async call, which isn't handled yet).
+
+For these reasons, core_request() needs to have an edible reference.
+
+Also, since core_request calls functions that might not return, there are cases
+where it will not be able to call abandon_core() and leave process context.
+This is an example of why we have the fallback case of leaving process context
+in proc_startcore().  See the section below about process context for more
+information.
+
+Eventually, core_request() will be split better, probably with the brutal logic
+in process.c that would call out some functions in resource.c that actually make
+choices.
+
+1.9 Things I Could Have Done But Didn't And Why:
+---------------------------
+Q: Could we have the first reference (existence) mean it could be on the runnable
+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?
+
+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.
+
+Q: Could we have made proc_destroy() take a flag, saying whether or not it was
+called on current and needed a decref instead of wasting an incref?
+
+A: We could, but won't.  This is one case where the external caller is the one
+that knows the function needs to decref or not.  But it breaks the convention a
+bit, doesn't mirror proc_create() as well, and we need to pull in the cacheline
+with the refcnt anyways.  So for now, no.
+
+Q: Could we make __proc_give_cores() simply not return if an IPI is coming?
+
+A: I did this originally, and manually unlocked and __wait_for_ipi()d.  Though
+we'd then need to deal with it like that for all of the related functions, which
+doesn't work if you wanted to do something afterwards (like schedule(p)).  Also
+these functions are meant to be internal helpers, so returning the bool makes
+more sense.  It eventually led to having __proc_unlock_ipi_pending(), which made
+proc_destroy() much cleaner and helped with a general model of dealing with
+these issues.  Win-win.
+
+2 When Do We Really Leave "Process Context"?
+===========================
+2.1 Overview
+---------------------------
+First off, it's not really "process context" in the way Linux deals with it.  We
+aren't operating in kernel mode on behalf of the process (always).  We are
+specifically talking about when a process's cr3 is loaded on a core.  Usually,
+current is also set (the exception for now is when processing ARSCs).
+
+There are a couple different ways to do this.  One is to never unload a context
+until something new is being run there (handled solely in proc_startcore()).
+Another way is to always explicitly leave the core, like by abandon_core()ing.
+
+The issue with the former is that you could have contexts sitting around for a
+while, and also would have a bit of extra latency when __proc_free()ing during
+someone *else's* proc_startcore() (though that could be avoided if it becomes a
+real issue, via some form of reaping).  You'll also probably have excessive
+decrefs (based on the interactions between proc_run() and __startcore()).
+
+The issue with the latter is excessive TLB shootdowns and corner cases.  There
+could be some weird cases (in core_request() for example) where the core you are
+running on has the context loaded for proc A on a mgmt core, but decides to give
+it to proc B.
+
+If no process is running there, current == 0 and boot_cr3 is loaded, meaning no
+process's context is loaded.
+
+2.2 Here's how it is done now:
+---------------------------
+We try to proactively leave, but have the ability to stay in context til
+proc_startcore() to handle the corner cases (and to maybe cut down the TLB
+flushes later).  To stop proactively leaving, just change abandon_core() to not
+do anything with current/cr3.  You'll see weird things like processes that won't
+die until their old cores are reused.  The reason we proactively leave context
+is to help with sanity for these issues, and also to avoid decref's in
+__startcore().
+
+A couple other details: __startcore() sorts the extra increfs, and
+proc_startcore() sorts leaving the old context.  Anytime a __startcore active
+message is sent, the sender increfs in advance for the current refcnt.  If that
+was in error, __startcore decrefs.  proc_startcore(), which the last moment
+before we *must* have the cr3/current issues sorted, does the actual check if
+there was an old process there or not, while it handles the lcr3 (if necessary).
+In general, lcr3's ought to have refcnts near them, or else comments explaining
+why not.
+
+So we leave process context when told to do so (__death/abandon_core()) or if
+another process is run there.  The _M code is such that a proc will stay on its
+core until it receives a message, and that message would cleanup/restore a
+generic context (boot_cr3).  A _S could stay on its core until another _S came
+in.  This is much simpler for cases when a timer interrupt goes off to force a
+schedule() decision.  It also avoids a TLB flush in case the scheduler picked
+that same proc to run again.  This could also happen to an _M, if for some
+reason it was given a management core (!!!) or some other event happened that
+caused some management/scheduling function to run on one of it's cores (perhaps
+it asked).
+
+proc_yield() abandons the core / leaves context.
+
+2.3 Other issues:
+---------------------------
+Note that it is not clear exactly how we want to deal with interrupting
+processes that are in the kernel.  There is no true process context, so we can't
+leave a core until the kernel is in a "safe place", i.e. it's state is bundled
+enough that it can be recontinued later.  We might end up letting the call
+proceed, but not return to userspace (since that's a good save point).  There's
+some rough comments about this in proc_startcore() (check for a pending
+preempt).
+
+This same thing applies to __death messages.  Even though a process is dying, it
+doesn't mean we can just drop whatever the kernel was doing on its behalf.  For
+instance, it might be holding a reference that will never get decreffed if its
+stack gets dropped.  This is a big TODO.
+
+3. Leaving the Kernel Stack:
+===========================
+Next painful commit will deal with this a bit more...
index b865fe7..a48988a 100644 (file)
@@ -184,10 +184,13 @@ trap_dispatch(trapframe_t *tf)
                case T_SYSCALL:
                        // check for userspace, for now
                        assert(tf->tf_cs != GD_KT);
+                       // syscall code wants an edible reference for current
+                       proc_incref(current, 1);
                        tf->tf_regs.reg_eax =
                                syscall(current, tf->tf_regs.reg_eax, tf->tf_regs.reg_edx,
                                        tf->tf_regs.reg_ecx, tf->tf_regs.reg_ebx,
                                        tf->tf_regs.reg_edi, tf->tf_regs.reg_esi);
+                       proc_decref(current, 1);
                        proc_startcore(current, tf); // Note the comment in syscall.c
                        break;
                default:
@@ -197,6 +200,7 @@ trap_dispatch(trapframe_t *tf)
                                panic("Damn Damn!  Unhandled trap in the kernel!");
                        else {
                                warn("Unexpected trap from userspace");
+                               proc_incref(current, 1);
                                proc_destroy(current);
                                return;
                        }
@@ -343,6 +347,7 @@ page_fault_handler(trapframe_t *tf)
        cprintf("[%08x] user fault va %08x ip %08x from core %d\n",
                current->pid, fault_va, tf->tf_eip, core_id());
        print_trapframe(tf);
+       proc_incref(current, 1);
        proc_destroy(current);
 }
 
@@ -359,6 +364,8 @@ void sysenter_callwrapper(struct Trapframe *tf)
        // save a per-core reference to the tf
        set_current_tf(tf);
 
+       // syscall code wants an edible reference for current
+       proc_incref(current, 1);
        tf->tf_regs.reg_eax = (intreg_t) syscall(current,
                                                 tf->tf_regs.reg_eax,
                                                 tf->tf_regs.reg_edx,
@@ -366,6 +373,7 @@ void sysenter_callwrapper(struct Trapframe *tf)
                                                 tf->tf_regs.reg_ebx,
                                                 tf->tf_regs.reg_edi,
                                                 0);
+       proc_decref(current, 1);
        /*
         * careful here - we need to make sure that this current is the right
         * process, which could be weird if the syscall blocked.  it would need to
index 373fe5d..2676412 100644 (file)
@@ -148,6 +148,7 @@ unhandled_trap(trapframe_t* state)
        {
                warn("Unhandled trap in user!\nTrap type: %s",buf);
                assert(current);
+               proc_incref(current, 1);
                proc_destroy(current);
                panic("I shouldn't have gotten here!");
        }
@@ -237,7 +238,10 @@ handle_syscall(trapframe_t* state)
 
        //env_push_ancillary_state(current); // remove this if you don't need it
 
+       // syscall code wants an edible reference for current
+       proc_incref(current, 1);
        state->gpr[8] = syscall(current,num,a1,a2,a3,a4,a5);
+       proc_decref(current, 1);
 
        trap_handled();
 }
index 4c6a421..cbd0e64 100644 (file)
@@ -77,26 +77,34 @@ void proc_yield(struct proc *SAFE p);
  * These all adjust the vcoremap and take appropriate actions (like __startcore
  * if you were already RUNNING_M.  You could be RUNNABLE_M with no vcores when
  * these are done (basically preempted, and waiting to get run again).
- * All of these could modify corelist and *num to communicate info back out,
- * which would be the list of cores that are known to be free.
+ *
+ * These are internal functions.  Error checking is to catch bugs, and you
+ * shouldn't call these functions with parameters you are not sure about (like
+ * an invalid corelist).  
+ *
+ * They also may cause an IPI to be sent to core it is called on.  If so, the
+ * return value will be true.  Once you unlock (and enable interrupts) you will
+ * be preempted, and usually lose your stack.  There is a helper to unlock and
+ * handle the refcnt.
  *
  * WARNING: YOU MUST HOLD THE PROC_LOCK BEFORE CALLING THESE! */
 /* Gives process p the additional num cores listed in corelist */
-error_t __proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num);
-/* Makes process p's coremap look like corelist (add, remove, etc) */
-error_t __proc_set_allcores(struct proc *SAFE p, uint32_t corelist[],
-                            size_t *num, amr_t message, TV(a0t) arg0,
-                            TV(a1t) arg1, TV(a2t) arg2);
+bool __proc_give_cores(struct proc *SAFE p, int32_t *corelist, size_t num);
+/* Makes process p's coremap look like corelist (add, remove, etc). Not used */
+bool __proc_set_allcores(struct proc *SAFE p, int32_t *corelist,
+                         size_t *num, amr_t message, TV(a0t) arg0,
+                         TV(a1t) arg1, TV(a2t) arg2);
 /* Takes from process p the num cores listed in corelist */
-error_t __proc_take_cores(struct proc *SAFE p, uint32_t corelist[],
-                          size_t *num, amr_t message, TV(a0t) arg0,
+bool __proc_take_cores(struct proc *SAFE p, int32_t *corelist,
+                       size_t num, amr_t message, TV(a0t) arg0,
+                       TV(a1t) arg1, TV(a2t) arg2);
+bool __proc_take_allcores(struct proc *SAFE p, amr_t message, TV(a0t) arg0,
                           TV(a1t) arg1, TV(a2t) arg2);
-error_t __proc_take_allcores(struct proc *SAFE p, amr_t message, TV(a0t) arg0,
-                             TV(a1t) arg1, TV(a2t) arg2);
+void __proc_unlock_ipi_pending(struct proc *p, bool ipi_pending);
 
-/* The reference counts are mostly to track how many cores loaded the cr3 */
-error_t proc_incref(struct proc *SAFE p);
-void proc_decref(struct proc *SAFE p);
+/* Will probably have generic versions of these later. */
+void proc_incref(struct proc *SAFE p, size_t count);
+void proc_decref(struct proc *SAFE p, size_t count);
 
 /* Allows the kernel to figure out what process is running on this core.  Can be
  * used just like a pointer to a struct proc.  Need these to be macros due to
index 5e232b5..5fe0a15 100644 (file)
@@ -229,7 +229,7 @@ void load_icode(env_t *SAFE e, uint8_t *COUNT(size) binary, size_t size)
         * This can get a bit tricky if this code blocks (will need to think about a
         * decref then), if we try to change states, etc.
         */
-       proc_incref(e);
+       proc_incref(e, 1);
        lcr3(e->env_cr3);
 
        // TODO: how do we do a runtime COUNT?
@@ -255,7 +255,7 @@ void load_icode(env_t *SAFE e, uint8_t *COUNT(size) binary, size_t size)
 
        // reload the original address space
        lcr3(old_cr3);
-       proc_decref(e);
+       proc_decref(e, 1);
 }
 
 #define PER_CPU_THING(type,name)\
index bd091e0..01553d2 100644 (file)
@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <timing.h>
 #include <resource.h>
+#include <monitor.h>
 
 /*
  * Currently, if you leave this function by way of proc_run (process_workqueue
@@ -33,7 +34,7 @@ void manager(void)
        static uint8_t RACY progress = 0;
 
        struct proc *envs[256];
-       static struct proc *p ;
+       static struct proc *p;
 
        // for testing taking cores, check in case 1 for usage
        uint32_t corelist[MAX_NUM_CPUS];
@@ -53,6 +54,7 @@ void manager(void)
 
        switch (progress++) {
                case 0:
+                       // TODO: need to store the pid for future manager runs, not the *p
                        //p = kfs_proc_create(kfs_lookup_path("roslib_mhello"));
                        p = kfs_proc_create(kfs_lookup_path("roslib_mproctests"));
                        //p = kfs_proc_create(kfs_lookup_path("roslib_spawn"));
@@ -62,6 +64,7 @@ void manager(void)
                        // normal single-cored way
                        spin_unlock_irqsave(&p->proc_lock);
                        proc_run(p);
+                       proc_decref(p, 1);
                        #if 0
                        // this is how you can transition to a parallel process manually
                        // make sure you don't proc run first
@@ -74,6 +77,7 @@ void manager(void)
                        #endif
                        break;
                case 1:
+                       monitor(0);
                        #if 0
                        udelay(10000000);
                        // this is a ghetto way to test restarting an _M
index 71855a7..92247bd 100644 (file)
@@ -261,6 +261,7 @@ int mon_kfs_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        __proc_set_state(p, PROC_RUNNABLE_S);
        schedule_proc(p);
        spin_unlock_irqsave(&p->proc_lock);
+       proc_decref(p, 1); // let go of the reference created in proc_create()
        // 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
@@ -306,6 +307,7 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        return 1;
                }
                spin_unlock_irqsave(&p->proc_lock);
+               proc_decref(p, 1);
        } else if (!strcmp(argv[1], "kill")) {
                if (argc != 3) {
                        printk("Give me a pid number.\n");
@@ -317,6 +319,7 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        return 1;
                }
                proc_destroy(p);
+               proc_decref(p, 1);
        } else {
                printk("Bad option\n");
                return 1;
index 7d792a7..5df1034 100644 (file)
@@ -45,6 +45,13 @@ static void put_idle_core(uint32_t coreid)
        spin_unlock(&idle_lock);
 }
 
+/* Other helpers, implemented later. */
+static uint32_t get_free_vcoreid(struct proc *SAFE p, uint32_t prev);
+static uint32_t get_busy_vcoreid(struct proc *SAFE p, uint32_t prev);
+static int32_t __get_vcoreid(int32_t *corelist, size_t num, int32_t pcoreid);
+static int32_t get_vcoreid(struct proc *SAFE p, int32_t pcoreid);
+static inline void __wait_for_ipi(const char *fnname);
+
 /* PID management. */
 #define PID_MAX 32767 // goes from 0 to 32767, with 0 reserved
 static DECL_BITMASK(pid_bmask, PID_MAX + 1);
@@ -149,12 +156,14 @@ int __proc_set_state(struct proc *p, uint32_t state)
 }
 
 /* Returns a pointer to the proc with the given pid, or 0 if there is none */
-/* TODO: handle refcnting */
 struct proc *pid2proc(pid_t pid)
 {
        spin_lock(&pid_hash_lock);
        struct proc *p = hashtable_search(pid_hash, (void*)pid);
        spin_unlock(&pid_hash_lock);
+       /* if the refcnt was 0, decref and return 0 (we failed). (TODO) */
+       if (p)
+               proc_incref(p, 1); // TODO:(REF) to do this all atomically and not panic
        return p;
 }
 
@@ -215,7 +224,7 @@ static error_t proc_alloc(struct proc *SAFE*SAFE pp, pid_t parent_id)
     spinlock_init(&p->proc_lock);
        p->ppid = parent_id;
        __proc_set_state(p, PROC_CREATED);
-       p->env_refcnt = 1;
+       p->env_refcnt = 2; // one for the object, one for the ref we pass back
        p->env_flags = 0;
        p->env_entry = 0; // cheating.  this really gets set in load_icode
        p->num_vcores = 0;
@@ -286,32 +295,6 @@ static void __proc_free(struct proc *p)
        p->env_cr3 = 0;
        page_decref(pa2page(pa));
 
-       /* between when someone grabs the *proc (from pid), til they use it, it
-        * could be killed and/or deallocated.  lots of our protections come from
-        * the state, which needs to be dereferenced, and isn't protected from this.
-        *
-        * (TODO) rule: when you get a *proc, you up the refcnt.  need to down it
-        * when you are done.  if you fail, someone else is deallocating it.  you
-        * shouldn't have dereferenced it (it may be 'gone' already), so there is a
-        * slight race, but if you do this check quickly, it's *probably* not an
-        * issue.  so do it with interrupts off.  pid2proc will handle that, but you
-        * need to decref still
-        * - getting the pointer from current is a refcnt'd source.  when current is
-        *   set, the refcnt is up anyway (for protecting the page tables), so just
-        *   use it normally.
-        * - if you pass the pointer (via amsg, etc), the code that uses it should
-        *   up its refcnt or otherwise deal with the issue (which startcore should
-        *   do), though this might need to change. (freed and realloc before the
-        *   message arrives).  perhaps up the refcnt in advance, and down it if it
-        *   was unnecessary (TODO).
-        * - if the pointer is stored somewhere (like in an IO continuation), the
-        *   refcnt needs to stay up.
-        * - can check for DYING too, though some situations may want to grab a
-        *   dying proc's *
-        * - i am not concerned with pointing to a new proc with that same PID, the
-        *   concern is with referencing kernel memory.
-        * Might move this to proc_destroy.  depends if we want parents to wait().
-        */
        /* Remove self from the pid hash, return PID.  Note the reversed order. */
        spin_lock(&pid_hash_lock);
        if (!hashtable_remove(pid_hash, (void*)p->pid))
@@ -340,16 +323,19 @@ bool proc_controls(struct proc *actor, struct proc *target)
  * 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.  */
+ * Including in the case of preemption.
+ *
+ * This won't return if the current core is going to be one of the processes
+ * cores (either for _S mode or for _M if it's in the vcoremap).  proc_run will
+ * eat your reference if it does not return. */
 void proc_run(struct proc *p)
 {
+       bool self_ipi_pending = FALSE;
        spin_lock_irqsave(&p->proc_lock);
        switch (p->state) {
                case (PROC_DYING):
                        spin_unlock_irqsave(&p->proc_lock);
                        printk("Process %d not starting due to async death\n", p->pid);
-                       // There should be no core cleanup to do (like decref).
-                       assert(current != p);
                        // if we're a worker core, smp_idle, o/w return
                        if (!management_core())
                                smp_idle(); // this never returns
@@ -365,16 +351,27 @@ void proc_run(struct proc *p)
                        p->num_vcores = 0;
                        p->vcoremap[0] = core_id();
                        spin_unlock_irqsave(&p->proc_lock);
-                       // This normally doesn't return, but might error out in the future.
+                       /* Transferring our reference to startcore, where p will become
+                        * current.  If it already is, decref in advance.  This is similar
+                        * to __startcore(), in that it sorts out the refcnt accounting.  */
+                       if (current == p)
+                               proc_decref(p, 1);
                        proc_startcore(p, &p->env_tf);
                        break;
                case (PROC_RUNNABLE_M):
-                       __proc_set_state(p, PROC_RUNNING_M);
                        /* vcoremap[i] holds the coreid of the physical core allocated to
                         * this process.  It is set outside proc_run.  For the active
                         * message, a0 = struct proc*, a1 = struct trapframe*.   */
                        if (p->num_vcores) {
+                               __proc_set_state(p, PROC_RUNNING_M);
                                int i = 0;
+                               /* Up the refcnt, since num_vcores are going to start using this
+                                * process and have it loaded in their 'current'. */
+                               p->env_refcnt += p->num_vcores; // TODO: (REF) use incref
+                               /* If the core we are running on is in the vcoremap, we will get
+                                * an IPI (once we reenable interrupts) and never return. */
+                               if (__get_vcoreid(p->vcoremap, p->num_vcores, core_id()) != -1)
+                                       self_ipi_pending = TRUE;
                                // TODO: handle silly state (HSS)
                                // set virtual core 0 to run the main context on transition
                                if (p->env_flags & PROC_TRANSITION_TO_M) {
@@ -397,21 +394,24 @@ void proc_run(struct proc *p)
                                        send_active_message(p->vcoremap[i], (void *)__startcore,
                                                            (void *)p, (void *)0, (void *)i);
 #endif
+                       } else {
+                               warn("Tried to proc_run() an _M with no vcores!");
                        }
-                       /* There a subtle (attempted) 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 to make sure our message went out
-                        * before a possible death message.
+                       /* 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 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
+                        * possible death message.
                         * - Likewise, we need interrupts to be disabled, in case one of the
                         *   messages was for us, and reenable them after letting go of the
                         *   lock.  This is done by spin_lock_irqsave, so be careful if you
                         *   change this.
-                        * - This can also be done far more intelligently / efficiently,
-                        *   like skipping in case it's busy and coming back later.
                         * - Note there is no guarantee this core's interrupts were on, so
                         *   it may not get the message for a while... */
-                       spin_unlock_irqsave(&p->proc_lock);
+                       __proc_unlock_ipi_pending(p, self_ipi_pending);
                        break;
                default:
                        spin_unlock_irqsave(&p->proc_lock);
@@ -419,11 +419,8 @@ void proc_run(struct proc *p)
        }
 }
 
-/*
- * Runs the given context (trapframe) of process p on the core this code
- * executes on.  The refcnt tracks how many cores have "an interest" in this
- * process, which so far just means it uses the process's page table.  See the
- * massive comments around the incref function
+/* Runs the given context (trapframe) of process p on the core this code
+ * executes on.
  *
  * Given we are RUNNING_*, an IPI for death or preemption could come in:
  * 1. death attempt (IPI to kill whatever is on your core):
@@ -450,7 +447,16 @@ void proc_run(struct proc *p)
  * I think we need to make it such that the kernel in "process context" never
  * gets removed from the core (displaced from its stack) without going through
  * some "bundling" code.
- */
+ *
+ * A note on refcnting: this function will not return, and your proc reference
+ * will end up stored in current.  This will make no changes to p's refcnt, so
+ * do your accounting such that there is only the +1 for current.  This means if
+ * it is already in current (like in the trap return path), don't up it.  If
+ * it's already in current and you have another reference (like pid2proc or from
+ * an IPI), then down it (which is what happens in __startcore()).  If it's not
+ * in current and you have one reference, like proc_run(non_current_p), then
+ * also do nothing.  The refcnt for your *p will count for the reference stored
+ * in current. */
 void proc_startcore(struct proc *p, trapframe_t *tf) {
        // it's possible to be DYING, but it's a rare race.
        //if (p->state & (PROC_RUNNING_S | PROC_RUNNING_M))
@@ -464,20 +470,15 @@ void proc_startcore(struct proc *p, trapframe_t *tf) {
        }
        /* If the process wasn't here, then we need to load its address space. */
        if (p != current) {
-               if (proc_incref(p)) {
-                       // getting here would mean someone tried killing this while we tried
-                       // to start one of it's contexts (from scratch, o/w we had it's CR3
-                       // loaded already)
-                       // if this happens, a no-op death-IPI ought to be on its way...  we can
-                       // just smp_idle()
-                       smp_idle();
-               }
+               /* Do not incref here.  We were given the reference to current,
+                * pre-upped. */
                lcr3(p->env_cr3);
-               // we unloaded the old cr3, so decref it (if it exists)
-               // TODO: Consider moving this to wherever we really "mean to leave the
-               // process's context".  abandon_core() does this.
+               /* This is "leaving the process context" of the previous proc.  The
+                * previous lcr3 unloaded the previous proc's context.  This should
+                * rarely happen, since we usually proactively leave process context,
+                * but is the fallback. */
                if (current)
-                       proc_decref(current);
+                       proc_decref(current, 1);
                set_current_proc(p);
        }
        /* need to load our silly state, preferably somewhere other than here so we
@@ -492,28 +493,6 @@ void proc_startcore(struct proc *p, trapframe_t *tf) {
        env_pop_tf(tf);
 }
 
-/* Helper function, helps with receiving local death IPIs, for the cases when
- * this core is running the process.  We should received an IPI shortly.  If
- * not, odds are interrupts are disabled, which shouldn't happen while servicing
- * syscalls. */
-static void check_for_local_death(struct proc *p)
-{
-       if (current == p) {
-               /* a death IPI should be on its way, either from the RUNNING_S one, or
-                * from proc_take_cores with a __death.  in general, interrupts should
-                * be on when you call proc_destroy locally, but currently aren't for
-                * all things (like traphandlers).  since we're dying anyway, it seems
-                * reasonable to turn on interrupts.  note this means all non-proc
-                * management interrupt handlers must return (which they need to do
-                * anyway), so that we get back to this point.  Eventually, we can
-                * remove the enable_irq.  think about this (TODO) */
-               enable_irq();
-               udelay(1000000);
-               panic("Waiting too long on core %d for an IPI in proc_destroy()!",
-                     core_id());
-       }
-}
-
 /*
  * Destroys the given process.  This may be called from another process, a light
  * kernel thread (no real process context), asynchronously/cross-core, or from
@@ -530,19 +509,25 @@ static void check_for_local_death(struct proc *p)
  * (Last core/kernel thread to decref cleans up and deallocates resources.)
  *
  * Note that some cores can be processing async calls, but will eventually
- * decref.  Should think about this more.
- */
+ * decref.  Should think about this more, like some sort of callback/revocation.
+ *
+ * This will eat your reference if it won't return.  Note that this function
+ * needs to change anyways when we make __death more like __preempt.  (TODO) */
 void proc_destroy(struct proc *p)
 {
-       // Note this code relies on this lock disabling interrupts, similar to
-       // proc_run.
+       /* TODO: this corelist is taking up a lot of space on the stack */
        uint32_t corelist[MAX_NUM_CPUS];
        size_t num_cores_freed;
+       bool self_ipi_pending = FALSE;
        spin_lock_irqsave(&p->proc_lock);
+
+       /* TODO: (DEATH) look at this again when we sort the __death IPI */
+       if (current == p)
+               self_ipi_pending = TRUE;
+
        switch (p->state) {
                case PROC_DYING: // someone else killed this already.
-                       spin_unlock_irqsave(&p->proc_lock);
-                       check_for_local_death(p); // IPI may be on it's way.
+                       __proc_unlock_ipi_pending(p, self_ipi_pending);
                        return;
                case PROC_RUNNABLE_M:
                        /* Need to reclaim any cores this proc might have, even though it's
@@ -558,7 +543,7 @@ void proc_destroy(struct proc *p)
                        // here's how to do it manually
                        if (current == p) {
                                lcr3(boot_cr3);
-                               proc_decref(p); // this decref is for the cr3
+                               proc_decref(p, 1); // this decref is for the cr3
                                current = NULL;
                        }
                        #endif
@@ -582,24 +567,15 @@ void proc_destroy(struct proc *p)
                        panic("Weird state(0x%08x) in proc_destroy", p->state);
        }
        __proc_set_state(p, PROC_DYING);
+       /* this decref is for the process in general */
+       p->env_refcnt--; // TODO (REF)
+       //proc_decref(p, 1);
 
-       /* Leave interrupts turned off, but unlock.  Store whether or not we should
-        * reenable interrupts down below.  We're manually doing the unlock_irqsave,
-        * since we need to make sure we finish this function before turning
-        * interrupts back on (in case a death/abandon_core comes in). */
-       bool irq_en = spin_lock_irq_enabled(&p->proc_lock);
-       spin_unlock(&p->proc_lock);
-
-       proc_decref(p); // this decref is for the process in general
-       
-       /* Reenable interrupts if we should have from the spin_unlock_irqsave.  We
-        * should almost always be reenabling.  Once we reenable, we can receive a
-        * death IPI (if applicable) and never return. */
-       if (irq_en)
-               enable_irq();
-
-       /* if our core is part of process p, then check/wait for the death IPI. */
-       check_for_local_death(p);
+       /* Unlock and possible decref and wait.  A death IPI should be on its way,
+        * either from the RUNNING_S one, or from proc_take_cores with a __death.
+        * in general, interrupts should be on when you call proc_destroy locally,
+        * but currently aren't for all things (like traphandlers). */
+       __proc_unlock_ipi_pending(p, self_ipi_pending);
        return;
 }
 
@@ -631,18 +607,45 @@ static uint32_t get_busy_vcoreid(struct proc *SAFE p, uint32_t prev)
        return i;
 }
 
-/* Helper function.  Find the vcoreid for a given physical core id.
- * You better hold the lock before calling this.  If we use some sort of
- * pcoremap, we can avoid this linear search. */
-static uint32_t get_vcoreid(struct proc *SAFE p, int32_t pcoreid)
+/* Helper function.  Find the vcoreid for a given physical core id.  If we use
+ * some sort of pcoremap, we can avoid this linear search.  You better hold the
+ * lock before calling this.  Returns -1 on failure. */
+static int32_t __get_vcoreid(int32_t *corelist, size_t num, int32_t pcoreid)
 {
-       uint32_t i;
-       for (i = 0; i < MAX_NUM_CPUS; i++)
-               if (p->vcoremap[i] == pcoreid)
+       int32_t i;
+       bool found = FALSE;
+       for (i = 0; i < num; i++)
+               if (corelist[i] == pcoreid) {
+                       found = TRUE;
                        break;
-       if (i + 1 >= MAX_NUM_CPUS)
-               warn("At the end of the vcorelist.  Might want to check that out.");
-       return i;
+               }
+       if (found)
+               return i;
+       else
+               return -1;
+}
+
+/* Helper function.  Just like the one above, but this one panics on failure.
+ * You better hold the lock before calling this.  */
+static int32_t get_vcoreid(struct proc *SAFE p, int32_t pcoreid)
+{
+       int32_t vcoreid = __get_vcoreid(p->vcoremap, p->num_vcores, pcoreid);
+       assert(vcoreid != -1);
+       return vcoreid;
+}
+
+/* Use this when you are waiting for an IPI that you sent yourself.  In most
+ * cases, interrupts should already be on (like after a spin_unlock_irqsave from
+ * process context), but aren't always, like in proc_destroy().  We might be
+ * able to remove the enable_irq in the future.  Think about this (TODO).
+ *
+ * Note this means all non-proc management interrupt handlers must return (which
+ * they need to do anyway), so that we get back to this point.  */
+static inline void __wait_for_ipi(const char *fnname)
+{
+       enable_irq();
+       udelay(1000000);
+       panic("Waiting too long on core %d for an IPI in %s()!", core_id(), fnname);
 }
 
 /* Yields the calling core.  Must be called locally (not async) for now.
@@ -654,7 +657,8 @@ static uint32_t get_vcoreid(struct proc *SAFE p, int32_t pcoreid)
  *
  * - RES_CORES amt_wanted will be the amount running after taking away the
  *   yielder, unless there are none left, in which case it will be 1.
- */
+ *
+ * This does not return (abandon_core()), so it will eat your reference.  */
 void proc_yield(struct proc *SAFE p)
 {
        spin_lock_irqsave(&p->proc_lock);
@@ -685,8 +689,9 @@ void proc_yield(struct proc *SAFE p)
                        panic("Weird state(0x%08x) in proc_yield", p->state);
        }
        spin_unlock_irqsave(&p->proc_lock);
-       // clean up the core and idle.  for mgmt cores, they will ultimately call
-       // manager, which will call schedule(), which will repick the yielding proc.
+       proc_decref(p, 1);
+       /* Clean up the core and idle.  For mgmt cores, they will ultimately call
+        * manager, which will call schedule() and will repick the yielding proc. */
        abandon_core();
 }
 
@@ -706,29 +711,21 @@ void proc_yield(struct proc *SAFE p)
  * The other way would be to have this function have the side effect of changing
  * state, and finding another way to do the need_to_idle.
  *
- * In the event of an error, corelist will include all the cores that were *NOT*
- * given to the process (cores that are still free).  Practically, this will be
- * all of them, since it seems like an all or nothing deal right now.
+ * The returned bool signals whether or not a stack-crushing IPI will come in
+ * once you unlock after this function.
  *
- * WARNING: You must hold the proc_lock before calling this!*/
-
-// zra: If corelist has *num elements, then it would be best if the value is
-// passed instead of a pointer to it. If in the future some way will be needed
-// to return the number of cores, then it might be best to use a separate
-// parameter for that.
-error_t __proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num)
+ * WARNING: You must hold the proc_lock before calling this! */
+bool __proc_give_cores(struct proc *SAFE p, int32_t *corelist, size_t num)
 { TRUSTEDBLOCK
+       bool self_ipi_pending = FALSE;
        uint32_t free_vcoreid = 0;
        switch (p->state) {
                case (PROC_RUNNABLE_S):
                case (PROC_RUNNING_S):
                        panic("Don't give cores to a process in a *_S state!\n");
-                       return -EINVAL;
                        break;
                case (PROC_DYING):
-                       // just FYI, for debugging
-                       printk("[kernel] attempted to give cores to a DYING process.\n");
-                       return -EFAIL;
+                       panic("Attempted to give cores to a DYING process.\n");
                        break;
                case (PROC_RUNNABLE_M):
                        // set up vcoremap.  list should be empty, but could be called
@@ -742,7 +739,7 @@ error_t __proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num)
                                        assert(p->vcoremap[i]);
                        }
                        // add new items to the vcoremap
-                       for (int i = 0; i < *num; i++) {
+                       for (int i = 0; i < num; i++) {
                                // find the next free slot, which should be the next one
                                free_vcoreid = get_free_vcoreid(p, free_vcoreid);
                                printd("setting vcore %d to pcore %d\n", free_vcoreid, corelist[i]);
@@ -751,21 +748,26 @@ error_t __proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num)
                        }
                        break;
                case (PROC_RUNNING_M):
-                       for (int i = 0; i < *num; i++) {
+                       /* Up the refcnt, since num cores are going to start using this
+                        * process and have it loaded in their 'current'. */
+                       // TODO: (REF) use proc_incref once we have atomics
+                       p->env_refcnt += num;
+                       if (__get_vcoreid(corelist, num, core_id()) != -1)
+                               self_ipi_pending = TRUE;
+                       for (int i = 0; i < num; i++) {
                                free_vcoreid = get_free_vcoreid(p, free_vcoreid);
                                printd("setting vcore %d to pcore %d\n", free_vcoreid, corelist[i]);
                                p->vcoremap[free_vcoreid] = corelist[i];
                                p->num_vcores++;
-                               assert(corelist[i] != core_id()); // sanity
                                send_active_message(corelist[i], __startcore, p,
-                                                    (struct Trapframe *)0,
-                                                    (void*SNT)free_vcoreid);
+                                                   (struct Trapframe *)0,
+                                                   (void*SNT)free_vcoreid);
                        }
                        break;
                default:
                        panic("Weird proc state %d in proc_give_cores()!\n", p->state);
        }
-       return ESUCCESS;
+       return self_ipi_pending;
 }
 
 /* Makes process p's coremap look like corelist (add, remove, etc).  Caller
@@ -774,27 +776,29 @@ error_t __proc_give_cores(struct proc *SAFE p, uint32_t corelist[], size_t *num)
  * any cores that are getting removed.
  *
  * Before implementing this, we should probably think about when this will be
- * used.  Implies preempting for the message.
+ * used.  Implies preempting for the message.  The more that I think about this,
+ * the less I like it.  For now, don't use this, and think hard before
+ * implementing it.
  *
- * WARNING: You must hold the proc_lock before calling this!*/
-error_t __proc_set_allcores(struct proc *SAFE p, uint32_t corelist[],
-                            size_t *num, amr_t message,TV(a0t) arg0,
-                            TV(a1t) arg1, TV(a2t) arg2)
+ * WARNING: You must hold the proc_lock before calling this! */
+bool __proc_set_allcores(struct proc *SAFE p, int32_t *corelist,
+                         size_t *num, amr_t message,TV(a0t) arg0,
+                         TV(a1t) arg1, TV(a2t) arg2)
 {
        panic("Set all cores not implemented.\n");
 }
 
-/* Takes from process p the num cores listed in corelist.  In the event of an
- * error, corelist will contain the list of cores that are free, and num will
- * contain how many items are in corelist.  This isn't implemented yet, but
- * might be necessary later.  Or not, and we'll never do it.
+/* Takes from process p the num cores listed in corelist, using the given
+ * message for the active message (__death, __preempt, etc).  Like the others
+ * in this function group, bool signals whether or not an IPI is pending.
  *
- * WARNING: You must hold the proc_lock before calling this!*/
-error_t __proc_take_cores(struct proc *SAFE p, uint32_t corelist[],
-                          size_t *num, amr_t message, TV(a0t) arg0,
-                          TV(a1t) arg1, TV(a2t) arg2)
+ * WARNING: You must hold the proc_lock before calling this! */
+bool __proc_take_cores(struct proc *SAFE p, int32_t *corelist,
+                       size_t num, amr_t message, TV(a0t) arg0,
+                       TV(a1t) arg1, TV(a2t) arg2)
 { TRUSTEDBLOCK
        uint32_t vcoreid;
+       bool self_ipi_pending = FALSE;
        switch (p->state) {
                case (PROC_RUNNABLE_M):
                        assert(!message);
@@ -806,31 +810,36 @@ error_t __proc_take_cores(struct proc *SAFE p, uint32_t corelist[],
                        panic("Weird state %d in proc_take_cores()!\n", p->state);
        }
        spin_lock(&idle_lock);
-       assert((*num <= p->num_vcores) && (num_idlecores + *num <= num_cpus));
+       assert((num <= p->num_vcores) && (num_idlecores + num <= num_cpus));
        spin_unlock(&idle_lock);
-       for (int i = 0; i < *num; i++) {
+       for (int i = 0; i < num; i++) {
                vcoreid = get_vcoreid(p, corelist[i]);
                assert(p->vcoremap[vcoreid] == corelist[i]);
-               if (message)
+               if (message) {
+                       if (p->vcoremap[vcoreid] == core_id())
+                               self_ipi_pending = TRUE;
                        send_active_message(corelist[i], message, arg0, arg1, arg2);
+               }
                // give the pcore back to the idlecoremap
                put_idle_core(corelist[i]);
                p->vcoremap[vcoreid] = -1;
        }
-       p->num_vcores -= *num;
-       p->resources[RES_CORES].amt_granted -= *num;
-       return 0;
+       p->num_vcores -= num;
+       p->resources[RES_CORES].amt_granted -= num;
+       return self_ipi_pending;
 }
 
 /* Takes all cores from a process, which must be in an _M state.  Cores are
  * placed back in the idlecoremap.  If there's a message, such as __death or
- * __preempt, it will be sent to the cores.
+ * __preempt, it will be sent to the cores.  The bool signals whether or not an
+ * IPI is coming in once you unlock.
  *
  * WARNING: You must hold the proc_lock before calling this! */
-error_t __proc_take_allcores(struct proc *SAFE p, amr_t message,
-                             TV(a0t) arg0, TV(a1t) arg1, TV(a2t) arg2)
+bool __proc_take_allcores(struct proc *SAFE p, amr_t message,
+                          TV(a0t) arg0, TV(a1t) arg1, TV(a2t) arg2)
 {
        uint32_t active_vcoreid = 0;
+       bool self_ipi_pending = FALSE;
        switch (p->state) {
                case (PROC_RUNNABLE_M):
                        assert(!message);
@@ -847,76 +856,72 @@ error_t __proc_take_allcores(struct proc *SAFE p, amr_t message,
        for (int i = 0; i < p->num_vcores; i++) {
                // find next active vcore
                active_vcoreid = get_busy_vcoreid(p, active_vcoreid);
-               if (message)
+               if (message) {
+                       if (p->vcoremap[active_vcoreid] == core_id())
+                               self_ipi_pending = TRUE;
                        send_active_message(p->vcoremap[active_vcoreid], message,
                                             arg0, arg1, arg2);
+               }
                // give the pcore back to the idlecoremap
                put_idle_core(p->vcoremap[active_vcoreid]);
                p->vcoremap[active_vcoreid] = -1;
        }
        p->num_vcores = 0;
        p->resources[RES_CORES].amt_granted = 0;
-       return 0;
+       return self_ipi_pending;
 }
 
-/*
- * The process refcnt is the number of places the process 'exists' in the
- * system.  Creation counts as 1.  Having your page tables loaded somewhere
- * (lcr3) counts as another 1.  A non-RUNNING_* process should have refcnt at
- * least 1.  If the kernel is on another core and in a processes address space
- * (like processing its backring), that counts as another 1.
- *
- * Note that the actual loading and unloading of cr3 is up to the caller, since
- * that's not the only use for this (and decoupling is more flexible).
- *
- * The refcnt should always be greater than 0 for processes that aren't dying.
- * When refcnt is 0, the process is dying and should not allow any more increfs.
- * A process can be dying with a refcnt greater than 0, since it could be
- * waiting for other cores to "get the message" to die, or a kernel core can be
- * finishing work in the processes's address space.
+/* Helper, to be used when unlocking after calling the above functions that
+ * might cause an IPI to be sent.  TODO inline this, so the __FUNCTION__ works.
+ * Will require an overhaul of core_request (break it up, etc) */
+void __proc_unlock_ipi_pending(struct proc *p, bool ipi_pending)
+{
+       if (ipi_pending) {
+               p->env_refcnt--; // TODO: (REF) (atomics)
+               spin_unlock_irqsave(&p->proc_lock);
+               __wait_for_ipi(__FUNCTION__);
+       } else {
+               spin_unlock_irqsave(&p->proc_lock);
+       }
+}
+
+
+/* This takes a referenced process and ups the refcnt by count.  If the refcnt
+ * was already 0, then someone has a bug, so panic.  Check out the Documentation
+ * for brutal details about refcnting.
  *
  * Implementation aside, the important thing is that we atomically increment
- * only if it wasn't already 0.  If it was 0, then we shouldn't be attaching to
- * the process, so we return an error, which should be handled however is
- * appropriate.  We currently use spinlocks, but some sort of clever atomics
- * would work too.
- *
- * Also, no one should ever update the refcnt outside of these functions.
- * Eventually, we'll have Ivy support for this. (TODO)
+ * only if it wasn't already 0.  If it was 0, panic.
  *
- * TODO: (REF) change to use CAS.
- */
-error_t proc_incref(struct proc *p)
+ * TODO: (REF) change to use CAS / atomics. */
+void proc_incref(struct proc *p, size_t count)
 {
-       error_t retval = 0;
        spin_lock_irqsave(&p->proc_lock);
        if (p->env_refcnt)
-               p->env_refcnt++;
+               p->env_refcnt += count;
        else
-               retval = -EBADPROC;
+               panic("Tried to incref a proc with no existing refernces!");
        spin_unlock_irqsave(&p->proc_lock);
-       return retval;
 }
 
-/*
- * When the kernel is done with a process, it decrements its reference count.
- * When the count hits 0, no one is using it and it should be freed.
- * "Last one out" actually finalizes the death of the process.  This is tightly
- * coupled with the previous function (incref)
- * Be sure to load a different cr3 before calling this!
+/* When the kernel is done with a process, it decrements its reference count.
+ * When the count hits 0, no one is using it and it should be freed.  "Last one
+ * out" actually finalizes the death of the process.  This is tightly coupled
+ * with the previous function (incref)
  *
  * TODO: (REF) change to use CAS.  Note that when we do so, we may be holding
- * the process lock when calling __proc_free().
- */
-void proc_decref(struct proc *p)
+ * the process lock when calling __proc_free(). */
+void proc_decref(struct proc *p, size_t count)
 {
        spin_lock_irqsave(&p->proc_lock);
-       p->env_refcnt--;
+       p->env_refcnt -= count;
        size_t refcnt = p->env_refcnt; // need to copy this in so it's not reloaded
        spin_unlock_irqsave(&p->proc_lock);
        // if we hit 0, no one else will increment and we can check outside the lock
        if (!refcnt)
                __proc_free(p);
+       if (refcnt < 0)
+               panic("Too many decrefs!");
 }
 
 /* Active message handler to start a process's context on this core.  Tightly
@@ -945,21 +950,23 @@ void __startcore(trapframe_t *tf, uint32_t srcid, void * a0, void * a1,
                proc_set_tfcoreid(tf_to_pop, (uint32_t)a2);
                proc_set_program_counter(tf_to_pop, p_to_run->env_entry);
        }
+       /* the sender of the amsg increfed, thinking we weren't running current. */
+       if (p_to_run == current)
+               proc_decref(p_to_run, 1);
        proc_startcore(p_to_run, tf_to_pop);
 }
 
-/* Stop running whatever context is on this core and to 'idle'.  Note this
- * leaves no trace of what was running. This "leaves the process's context. */
+/* 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. */
 void abandon_core(void)
 {
        /* If we are currently running an address space on our core, we need a known
-        * good pgdir before releasing the old one.  This is currently the major
-        * practical implication of the kernel caring about a processes existence
-        * (the inc and decref).  This decref corresponds to the incref in
-        * proc_startcore (though it's not the only one). */
+        * good pgdir before releasing the old one.  We decref, since current no
+        * longer tracks the proc (and current no longer protects the cr3). */
        if (current) {
                lcr3(boot_cr3);
-               proc_decref(current);
+               proc_decref(current, 1);
                set_current_proc(NULL);
        }
        smp_idle();
@@ -1011,7 +1018,7 @@ void print_proc_info(pid_t pid)
        printk("PID: %d\n", p->pid);
        printk("PPID: %d\n", p->ppid);
        printk("State: 0x%08x\n", p->state);
-       printk("Refcnt: %d\n", p->env_refcnt);
+       printk("Refcnt: %d\n", p->env_refcnt - 1); // don't report our ref
        printk("Flags: 0x%08x\n", p->env_flags);
        printk("CR3(phys): 0x%08x\n", p->env_cr3);
        printk("Num Vcores: %d\n", p->num_vcores);
@@ -1028,4 +1035,5 @@ void print_proc_info(pid_t pid)
        printk("Vcore 0's Last Trapframe:\n");
        print_trapframe(&p->env_tf);
        spin_unlock_irqsave(&p->proc_lock);
+       proc_decref(p, 1); /* decref for the pid2proc reference */
 }
index 1f18c5e..cf5d40d 100644 (file)
  * Will return either the number actually granted or an error code.  This will
  * not decrease the actual amount of cores (e.g. from 5 to 2), but it will
  * transition a process from _M to _S (amt_wanted == 0).
+ *
+ * This needs a consumable/edible reference of p, in case it doesn't return.
+ *
+ * TODO: this is a giant function.  need to split it up a bit, probably move the
+ * guts to process.c and have functions to call for the brains.
  */
 ssize_t core_request(struct proc *p)
 {
        size_t num_granted;
        ssize_t amt_new;
-       uint32_t corelist[MAX_NUM_CPUS];
+       int32_t corelist[MAX_NUM_CPUS];
        bool need_to_idle = FALSE;
+       bool self_ipi_pending = FALSE;
 
        spin_lock_irqsave(&p->proc_lock);
        /* check to see if this is a full deallocation.  for cores, it's a
-        * transition from _M to _S */
+        * transition from _M to _S.  Will be issues with handling this async. */
        if (!p->resources[RES_CORES].amt_wanted) {
-               assert(p->state == PROC_RUNNING_M);
+               assert(p->state == PROC_RUNNING_M); // TODO: (ACR) async core req
                // save the context, to be restarted in _S mode
                p->env_tf = *current_tf;
                env_push_ancillary_state(p);
                proc_set_syscall_retval(&p->env_tf, ESUCCESS);
-               // in this case, it's not our job to save contexts or anything
-               __proc_take_allcores(p, __death, 0, 0, 0);
+               /* 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
+                * below, and need to eat the reference to p */
+               self_ipi_pending = __proc_take_allcores(p, __death, 0, 0, 0);
                __proc_set_state(p, PROC_RUNNABLE_S);
                schedule_proc(p);
+               __proc_unlock_ipi_pending(p, self_ipi_pending);
+               return 0;
        }
        /* otherwise, see how many new cores are wanted */
        amt_new = p->resources[RES_CORES].amt_wanted -
@@ -86,7 +96,7 @@ ssize_t core_request(struct proc *p)
                switch (p->state) {
                        case (PROC_RUNNING_S):
                                // issue with if we're async or not (need to preempt it)
-                               // either of these should trip it.
+                               // either of these should trip it. TODO: (ACR) async core req
                                if ((current != p) || (p->vcoremap[0] != core_id()))
                                        panic("We don't handle async RUNNING_S core requests yet.");
                                /* save the tf to be restarted on another core (in proc_run) */
@@ -119,18 +129,21 @@ ssize_t core_request(struct proc *p)
                        default:
                                break;
                }
-               /* give them the cores.  this will start up the extras if RUNNING_M */
-               __proc_give_cores(p, corelist, &num_granted);
-               spin_unlock_irqsave(&p->proc_lock);
+               /* give them the cores.  this will start up the extras if RUNNING_M. */
+               self_ipi_pending = __proc_give_cores(p, corelist, num_granted);
+               __proc_unlock_ipi_pending(p, self_ipi_pending);
                /* if there's a race on state (like DEATH), it'll get handled by
                 * proc_run or proc_destroy */
                if (p->state == PROC_RUNNABLE_M)
                        proc_run(p);
                /* if we are moving to a partitionable core from a RUNNING_S on a
                 * management core, the kernel needs to do something else on this core
-                * (just like in proc_destroy).  this cleans up the core and idles. */
-               if (need_to_idle)
+                * (just like in proc_destroy).  it also needs to decref, to consume the
+                * reference that came into this function (since we don't return).  */
+               if (need_to_idle) {
+                       proc_decref(p, 1);
                        abandon_core();
+               }
        } else { // nothing granted, just return
                spin_unlock_irqsave(&p->proc_lock);
        }
index 00b44df..b6420b8 100644 (file)
@@ -32,6 +32,8 @@ void schedule_init(void)
 
 void schedule_proc(struct proc *p)
 {
+       p->env_refcnt++; // TODO (REF) (usually is called while locked)
+       //proc_incref(p, 1); /* up the refcnt since we are storing the reference */
        spin_lock_irqsave(&runnablelist_lock);
        printd("Scheduling PID: %d\n", p->pid);
        TAILQ_INSERT_TAIL(&proc_runnablelist, p, proc_link);
@@ -49,6 +51,8 @@ void deschedule_proc(struct proc *p)
        printd("Descheduling PID: %d\n", p->pid);
        TAILQ_REMOVE(&proc_runnablelist, p, proc_link);
        spin_unlock_irqsave(&runnablelist_lock);
+       p->env_refcnt--; // TODO (REF) (usually is called while locked)
+       //proc_decref(p, 1); /* down the refcnt, since its no longer stored */
        return;
 }
 
@@ -67,7 +71,9 @@ void schedule(void)
                TAILQ_REMOVE(&proc_runnablelist, p, proc_link);
                spin_unlock_irqsave(&runnablelist_lock);
                printd("PID of proc i'm running: %d\n", p->pid);
+               /* proc_run will either eat the ref, or we'll decref manually. */
                proc_run(p);
+               proc_decref(p, 1);
        } else {
                spin_unlock_irqsave(&runnablelist_lock);
                printk("No processes to schedule, enjoy the Monitor!\n");
index 84ee748..cb86c8f 100644 (file)
@@ -94,6 +94,7 @@ static ssize_t sys_run_binary(env_t* e, void *DANGEROUS binary_buf,
        kfree(new_binary);
        __proc_set_state(env, PROC_RUNNABLE_S);
        schedule_proc(env);
+       proc_decref(env, 1);
        return 0;
 }
 
@@ -185,24 +186,35 @@ static ssize_t sys_shared_page_alloc(env_t* p1,
 
        void * COUNT(1) * COUNT(1) addr = user_mem_assert(p1, _addr, sizeof(void *),
                                                       PTE_USER_RW);
-       page_t* page;
        env_t* p2 = pid2proc(p2_id);
-       error_t e = page_alloc(&page);
+       if (!p2)
+               return -EBADPROC;
 
-       if(e < 0) return e;
+       page_t* page;
+       error_t e = page_alloc(&page);
+       if (e < 0) {
+               proc_decref(p2, 1);
+               return e;
+       }
 
        void* p2_addr = page_insert_in_range(p2->env_pgdir, page,
                                             (void*SNT)UTEXT, (void*SNT)UTOP, p2_flags);
-       if(p2_addr == NULL)
+       if (p2_addr == NULL) {
+               page_free(page);
+               proc_decref(p2, 1);
                return -EFAIL;
+       }
 
        void* p1_addr = page_insert_in_range(p1->env_pgdir, page,
                                            (void*SNT)UTEXT, (void*SNT)UTOP, p1_flags);
        if(p1_addr == NULL) {
                page_remove(p2->env_pgdir, p2_addr);
+               page_free(page);
+               proc_decref(p2, 1);
                return -EFAIL;
        }
        *addr = p1_addr;
+       proc_decref(p2, 1);
        return ESUCCESS;
 }
 
@@ -344,13 +356,18 @@ static error_t sys_proc_destroy(struct proc *p, pid_t pid)
 
        if (!p_to_die)
                return -EBADPROC;
-       if (!proc_controls(p, p_to_die))
+       if (!proc_controls(p, p_to_die)) {
+               proc_decref(p_to_die, 1);
                return -EPERM;
-       if (p_to_die == p)
+       }
+       if (p_to_die == p) {
+               // syscall code and pid2proc both have edible references, only need 1.
+               proc_decref(p, 1);
                printk("[PID %d] proc exiting gracefully\n", p->pid);
-       else
+       } else {
                panic("Destroying other processes is not supported yet.");
                //printk("[%d] destroying proc %d\n", p->pid, p_to_die->pid);
+       }
        proc_destroy(p_to_die);
        return ESUCCESS;
 }
@@ -385,7 +402,9 @@ static int sys_proc_create(struct proc *p, const char *DANGEROUS path)
        if (kfs_inode < 0)
                return -EINVAL;
        struct proc *new_p = kfs_proc_create(kfs_inode);
-       return new_p->pid;
+       pid = new_p->pid;
+       proc_decref(new_p, 1); // let go of the reference created in proc_create()
+       return pid;
 }
 
 /* Makes process PID runnable.  Consider moving the functionality to process.c */
@@ -393,17 +412,24 @@ static error_t sys_proc_run(struct proc *p, unsigned pid)
 {
        struct proc *target = pid2proc(pid);
        error_t retval = 0;
-       spin_lock_irqsave(&p->proc_lock); // note we can get interrupted here. it's not bad.
+
+       if (!target)
+               return -EBADPROC;
+       // note we can get interrupted here. it's not bad.
+       spin_lock_irqsave(&p->proc_lock);
        // make sure we have access and it's in the right state to be activated
        if (!proc_controls(p, target)) {
+               proc_decref(target, 1);
                retval = -EPERM;
        } else if (target->state != PROC_CREATED) {
+               proc_decref(target, 1);
                retval = -EINVAL;
        } else {
                __proc_set_state(target, PROC_RUNNABLE_S);
                schedule_proc(target);
        }
        spin_unlock_irqsave(&p->proc_lock);
+       proc_decref(target, 1);
        return retval;
 }
 
@@ -515,6 +541,7 @@ intreg_t syscall_async(struct proc *p, syscall_req_t *call)
                       call->args[2], call->args[3], call->args[4]);
 }
 
+/* You should already have a refcnt'd ref to p before calling this */
 intreg_t process_generic_syscalls(struct proc *p, size_t max)
 {
        size_t count = 0;
@@ -524,8 +551,9 @@ intreg_t process_generic_syscalls(struct proc *p, size_t max)
         * incref will return ESUCCESS on success.  This might need some thought
         * regarding when the incref should have happened (like by whoever passed us
         * the *p). */
-       if (proc_incref(p))
-               return -EFAIL;
+       // TODO: ought to be unnecessary, if you called this right, kept here for
+       // now in case anyone actually uses the ARSCs.
+       proc_incref(p, 1);
 
        // max is the most we'll process.  max = 0 means do as many as possible
        while (RING_HAS_UNCONSUMED_REQUESTS(sysbr) && ((!max)||(count < max)) ) {
@@ -555,6 +583,6 @@ intreg_t process_generic_syscalls(struct proc *p, size_t max)
        }
        // load sane page tables (and don't rely on decref to do it for you).
        lcr3(boot_cr3);
-       proc_decref(p);
+       proc_decref(p, 1);
        return (intreg_t)count;
 }
index 579d484..5126074 100644 (file)
@@ -543,6 +543,7 @@ static void sync_tests(int start_core, int num_threads, int job_num)
        // we want to fake a run, to reenter manager for the next case
        env_t *env = kfs_proc_create(kfs_lookup_path("roslib_null"));
        smp_call_function_single(0, run_env_handler, env, 0);
+       // Note we are still holding references to all the processes
        process_workqueue();
        panic("whoops!\n");
 }
@@ -573,6 +574,7 @@ static void async_tests(int start_core, int num_threads, int job_num)
        // we want to fake a run, to reenter manager for the next case
        env_t *env = kfs_proc_create(kfs_lookup_path("roslib_null"));
        smp_call_function_single(0, run_env_handler, env, 0);
+       // Note we are still holding references to all the processes
        process_workqueue();
        // this all never returns
        panic("whoops!\n");
index 1b70abb..6d947bd 100644 (file)
@@ -154,6 +154,7 @@ static void global_tests(uint32_t vcoreid)
                case TEST_SWITCH_TO_RUNNABLE_S:
                        if (vcoreid == 2) {
                                cprintf("Core %d trying to request 0/ switch to _S\n", vcoreid);
+                               udelay(3000000, 1995014570);
                                retval = sys_resource_req(RES_CORES, 0, 0, 0);
                                // will only see this if we are scheduled()
                                cprintf("Core %d back up! (retval:%d)\n", vcoreid, retval);