Removes some struct proc* refcnting
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 12 Nov 2010 00:37:45 +0000 (16:37 -0800)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:56 +0000 (17:35 -0700)
We only do it when calling out of syscall.c into something that might
not return and wants a reference it can decref (other than current).

We can just get rid of this completely, making proc_yield() take a
current for a ref, and core_request handling itself.  The only reason we
don't yet is that core_request() could be called remotely (current isn't
loaded), so for now we'll keep having it expect an 'edible' reference.

Documentation/process-internals.txt
kern/src/syscall.c

index 57358c1..87b9c73 100644 (file)
@@ -79,7 +79,11 @@ protected by a refcnt.
 
 +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.
+  also a source of the pointer, so its a bit different.  Note that all kref's
+  are sources of a pointer.  Technically, to even use 'current', we should kref
+  it and pass it around as a proc.  We don't for performance reasons.  When we
+  are running on a core that has current loaded, the ref is both for its usage
+  as well as for being the current process.
 - 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
@@ -88,8 +92,9 @@ protected by a refcnt.
   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.
+  before passing the reference, and decref when it returns.  We used to do this
+  for all syscalls, but now only do it for calls that might not return and
+  expect to receive reference (like proc_yield).
 
 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
@@ -108,6 +113,11 @@ 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.
 
+Arguably, for functions that MAY not return, but will always be called with
+current's reference (proc_yield()), we could get away without giving it an
+edible reference, and then never eating the ref.  Yield needs to be reworked
+anyway, so it's not a bit deal yet.
+
 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
index f4bd2aa..c64ff86 100644 (file)
@@ -325,24 +325,27 @@ static error_t sys_proc_destroy(struct proc *p, pid_t pid, int exitcode)
                return -1;
        }
        if (p_to_die == p) {
-               // syscall code and pid2proc both have edible references, only need 1.
                p->exitcode = exitcode;
-               kref_put(&p_to_die->kref);
                printd("[PID %d] proc exiting gracefully (code %d)\n", p->pid,exitcode);
        } else {
                printd("[%d] destroying proc %d\n", p->pid, p_to_die->pid);
        }
        proc_destroy(p_to_die);
+       /* we only get here if we weren't the one to die */
        kref_put(&p_to_die->kref);
        return ESUCCESS;
 }
 
 static int sys_proc_yield(struct proc *p, bool being_nice)
 {
-       /* proc_yield() doesn't return - we need to set the syscall retval early */
+       /* proc_yield() often doesn't return - we need to set the syscall retval
+        * early.  If it doesn't return, it expects to eat our reference (for now).
+        */
        signal_current_sc(0);
+       kref_get(&p->kref, 1);
        proc_yield(p, being_nice);
-       assert(0);
+       kref_put(&p->kref);
+       return 0;
 }
 
 static ssize_t sys_fork(env_t* e)
@@ -438,7 +441,9 @@ static ssize_t sys_fork(env_t* e)
 
 /* Load the binary "path" into the current process, and start executing it.
  * argv and envp are magically bundled in procinfo for now.  Keep in sync with
- * glibc's sysdeps/ros/execve.c */
+ * glibc's sysdeps/ros/execve.c.  Once past a certain point, this function won't
+ * return.  It assumes (and checks) that it is current.  Don't give it an extra
+ * refcnt'd *p (syscall won't do that). */
 static int sys_exec(struct proc *p, char *path, size_t path_l,
                     struct procinfo *pi)
 {
@@ -453,6 +458,10 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
                set_errno(EINVAL);
                return -1;
        }
+       if (p != current) {
+               set_errno(EINVAL);
+               return -1;
+       }
        /* Can't exec if we don't have a current_tf to restart (if we fail). */
        if (!old_cur_tf) {
                set_errno(EINVAL);
@@ -522,8 +531,7 @@ 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
         * already been written to).*/
-       /* TODO: remove this when we remove the one in local_syscall() */
-       kref_put(&current->kref);
+       abandon_core();
        smp_idle();
        assert(0);
 }
@@ -613,9 +621,13 @@ static int sys_shared_page_free(env_t* p1, void*DANGEROUS addr, pid_t p2)
 static int sys_resource_req(struct proc *p, int type, unsigned int amt_wanted,
                             unsigned int amt_wanted_min, int flags)
 {
+       int retval;
        signal_current_sc(0);
        /* this might not return (if it's a _S -> _M transition) */
-       return resource_req(p, type, amt_wanted, amt_wanted_min, flags);
+       kref_get(&p->kref, 1);
+       retval = resource_req(p, type, amt_wanted, amt_wanted_min, flags);
+       kref_put(&p->kref);
+       return retval;
 }
 
 /* Will notify the target on the given vcore, if the caller controls the target.
@@ -1416,13 +1428,9 @@ void run_local_syscall(void)
        pcpui->nr_calls--;                              /* one less to do */
        (*pcpui->tf_retval_loc)++;              /* one more started */
        pcpui->errno_loc = &sysc->err;
-       // TODO: push this deeper into the syscalls */
-       kref_get(&current->kref, 1);
        /* TODO: arg5 */
        sysc->retval = syscall(pcpui->cur_proc, sysc->num, sysc->arg0, sysc->arg1,
                               sysc->arg2, sysc->arg3, sysc->arg4);
-       /* TODO: when we remove this, remove it from sys_exec() too */
-       kref_put(&current->kref);
        sysc->flags |= SC_DONE;
        /* regardless of whether the call blocked or not, we smp_idle().  If it was
         * the last call, it'll return to the process.  If there are more, it will