proc_destroy() no longer requires edible refs
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 29 Sep 2011 21:30:38 +0000 (14:30 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:08 +0000 (17:36 -0700)
proc_destroy() will return, though it does require handling an IMMEDIATE
kmsg (which PRKM currently does, or you can enable interrupts at some
point) before the kernel returns to userspace (in the event of
proc_destroy() being called on the current_tf).

Note that all return paths need to check interrupts (and RKMs) at some
point.

Documentation/process-internals.txt
kern/arch/i686/trap.c
kern/arch/riscv/trap.c
kern/arch/sparc/trap.c
kern/src/mm.c
kern/src/smp.c
kern/src/syscall.c
kern/src/umem.c

index 981f4c5..8ace357 100644 (file)
@@ -172,13 +172,12 @@ __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_destroy(): it used to not return, and back then if your reference was
+from 'current', you needed to incref.  Now that proc_destroy() returns, it
+isn't a big deal.  Just keep in mind that if you have a function that doesn't
+return, there's no way for the function to know if it's passed reference is
+edible.  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.
@@ -235,15 +234,16 @@ 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?
+Q: (Moot) 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?
+Q: (Moot) 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
index 94e77ca..545d5dd 100644 (file)
@@ -275,10 +275,7 @@ static void trap_dispatch(struct trapframe *tf)
                                panic("Damn Damn!  Unhandled trap in the kernel!");
                        else {
                                warn("Unexpected trap from userspace");
-                               proc_incref(current, 1);
                                proc_destroy(current);
-                               assert(0);
-                               return;
                        }
        }
        return;
@@ -417,9 +414,7 @@ void page_fault_handler(struct trapframe *tf)
                       current->pid, prot & PROT_READ ? "READ" : "WRITE", fault_va,
                       tf->tf_eip, core_id(), err);
                print_trapframe(tf);
-               proc_incref(current, 1);
                proc_destroy(current);
-               assert(0);
        }
 }
 
index 1caf969..506fffc 100644 (file)
@@ -272,10 +272,10 @@ unhandled_trap(trapframe_t* state, const char* name)
                spin_unlock(&screwup_lock);
 
                assert(current);
-               proc_incref(current, 1);
                proc_destroy(current);
-
-               panic("I shouldn't have gotten here!");
+               /* Not sure if RISCV has a central point that would run proc_restartcore
+                */
+               proc_restartcore();
        }
 }
 
index aae53da..c14dbde 100644 (file)
@@ -337,10 +337,10 @@ unhandled_trap(trapframe_t* state)
                spin_unlock(&screwup_lock);
 
                assert(current);
-               proc_incref(current, 1);
                proc_destroy(current);
-
-               panic("I shouldn't have gotten here!");
+               /* Not sure if SPARC has a central point that would run proc_restartcore
+                */
+               proc_restartcore();
        }
 }
 
@@ -448,9 +448,8 @@ handle_pop_tf(trapframe_t* state)
 
        trapframe_t tf, *tf_p = &tf;
        if (memcpy_from_user(current,&tf,(void*)state->gpr[8],sizeof(tf))) {
-               proc_incref(current, 1);
                proc_destroy(current);
-               assert(0);
+               proc_restartcore();
        }
 
        proc_secure_trapframe(&tf);
@@ -465,7 +464,7 @@ handle_set_tf(trapframe_t* state)
        if (memcpy_to_user(current,(void*)state->gpr[8],state,sizeof(*state))) {
                proc_incref(current, 1);
                proc_destroy(current);
-               assert(0);
+               proc_restartcore();
        }
 }
 
index 6d35f16..e0a2461 100644 (file)
@@ -393,9 +393,7 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
         *
         * If HPF errors out, we'll warn for now, since it is likely a bug in
         * userspace, though since POPULATE is an opportunistic thing, we don't need
-        * to actually kill the process.  If we do kill them, make sure to
-        * incref/decref around proc_destroy, since we likely don't have an edible
-        * reference to p. */
+        * to actually kill the process. */
        if (flags & MAP_POPULATE)
                for (int i = 0; i < num_pages; i++) {
                        retval = __handle_page_fault(p, addr + i * PGSIZE, vmr->vm_prot);
index b4f95e7..e4ac29b 100644 (file)
@@ -37,8 +37,6 @@ static void try_run_proc(void)
                assert(pcpui->cur_proc);        /* aka, current */
                proc_restartcore();
                assert(0);
-       } else {
-               assert(!pcpui->cur_proc);
        }
 }
 
index ed68fe2..303e584 100644 (file)
@@ -299,6 +299,7 @@ static int sys_proc_create(struct proc *p, char *path, size_t path_l,
        return pid;
 late_error:
        proc_destroy(new_p);
+       proc_decref(new_p);     /* give up the reference created in proc_create() */
 mid_error:
        kref_put(&program->f_kref);
        return -1;
@@ -454,6 +455,7 @@ static ssize_t sys_fork(env_t* e)
         * address space. */
        if (env_user_mem_walk(e, 0, UMAPTOP, &copy_page, env)) {
                proc_destroy(env);      /* this is prob what you want, not decref by 2 */
+               proc_decref(env);
                set_errno(ENOMEM);
                return -1;
        }
@@ -539,11 +541,8 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
        env_user_mem_free(p, 0, UMAPTOP);
        if (load_elf(p, program)) {
                kref_put(&program->f_kref);
-               /* Need an edible reference for proc_destroy in case it doesn't return.
-                * sys_exec was given current's ref (counted once just for current) */
-               proc_incref(p, 1);
+               /* Note this is an inedible reference, but proc_destroy now returns */
                proc_destroy(p);
-               proc_decref(p);
                /* We don't want to do anything else - we just need to not accidentally
                 * return to the user (hence the all_out) */
                goto all_out;
index 4728460..e2a776d 100644 (file)
@@ -120,10 +120,8 @@ void *user_mem_assert(struct proc *p, const void *DANGEROUS va, size_t len,
        if (!res) {
                cprintf("[%08x] user_mem_check assertion failure for "
                        "va %08x\n", p->pid, user_mem_check_addr);
-               /* assuming this is used with an inedible reference */
-               proc_incref(p, 1);
-               proc_destroy(p);        // may not return
-               assert(0);
+               /* won't be freed til the caller decrefs */
+               proc_destroy(p);
         return NULL;
        }
     return res;