Changes the pidhash to be an internal reference
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 6 Aug 2010 19:20:10 +0000 (12:20 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:50 +0000 (17:35 -0700)
In sync with kref usage patterns, easier to understand, and fixes a bug
with removing from the pidhash before freeing (which messed up
sys_try_wait()).

Side note: this patch changes more documentation than code (which was
really just one change and one move).

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

index 9eba08d..2b61bcb 100644 (file)
@@ -97,11 +97,11 @@ other cleanup without holding a list's lock.  This effectively allows you to
 split the release between what kref_put() does automatically with whatever else
 you want done.
 
-What about an 'internal' kref?  Much like with the pid2proc hash, if we refcnt
-the refs on lists, we still have the same issue of syncing between a list reader
-and a list writer.  The reader needs to atomically read and kref_get (not_zero
-or otherwise).  Otherwise, someone can remove, put, free, and do whatever to the
-item between the read and the kref_get (in theory).
+What about an 'internal' kref?  If we refcnt the refs on lists, we still have
+the same issue of syncing between a list reader and a list writer.  The reader
+needs to atomically read and kref_get (not_zero or otherwise).  Otherwise,
+someone can remove, put, free, and do whatever to the item between the read and
+the kref_get (in theory).
 
 The reason for this is the same reason we have trouble with lists and internal
 references in the first place: both the list reader and the writer are sharing
@@ -213,12 +213,12 @@ TAILQ all the time vs. being in a process's file list).
 Process management is a bit different, since it does not want to destroy or free
 you until there was some explicit action (calling proc_destroy()).  We still use
 krefs, since we don't know who is the "last one out" to do the freeing, so we
-layer proc_destroy() on top of kref/__proc_free().  This makes it easier for us
-to hack something together that works with the pid2proc hash.  Specifically, we
-can kref items in that LLS, as well as the runnable_list.  We know when to take
-them out.  This is similar to the dentry, except that when there is no other
-reference than the LLS, we don't want to do anything in particular (at least not
-yet).  The dentry needs to have a lot of state changed, and maybe freed.
+layer proc_destroy() on top of kref/__proc_free().  This is why we have the "one
+ref to keep the object alive."  For a little while, this ref was stored in the
+pid2proc hash, but that is now holds an internal reference (we have the tech, it
+keeps things in sync with other usage models, and it makes proc_destroy and
+sys_trywait easier).  Note the runnable_list has external references, in part
+because it is a different subsystem (scheduler).
 
 Remember: the reason for why we have trouble with lists and (internal)
 references: both the list reader and the writer are sharing the same
index 3e6bcb8..57358c1 100644 (file)
@@ -26,8 +26,8 @@ to Linux's kref:
   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 from pid2proc (which is a
-  careful way of doing this).  pid2proc kref_get()s on the reference that is
-  stored inside it.  If you incref and there are 0 references, the kernel will
+  careful way of doing this - pid2proc kref_get_not_zero()s on the reference that is
+  stored inside it).  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
@@ -59,11 +59,14 @@ protected by a refcnt.
 +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.  This existence is
-  based on it's presence in the hash table, which is a stored reference.
-- The hash table is a bit tricky.  We need to kref_get() when it is locked, so
-  we know we have a valid kref to get.  We don't need to lock the list to
-  kref_put the process, since once it is removed from the hash, we now own that
-  reference.  
+  explicitly kref_put()d in proc_destroy().
+- The hash table is a bit tricky.  We need to kref_get_not_zero() when it is
+  locked, so we know we aren't racing with proc_free freeing the proc and
+  removing it from the list.  After removing it from the hash, we don't need to
+  kref_put it, since it was an internal ref.  The kref (i.e. external) isn't for
+  being on the hash list, it's for existing.  This separation allows us to
+  remove the proc from the hash list in the "release" function.  See kref.txt
+  for more details.
 
 +1 for someone using it or planning to use it.
 - This includes simply having a pointer to the proc, since presumably you will
index 3ef3272..efdaaaa 100644 (file)
@@ -162,17 +162,18 @@ 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.
- * Note this makes a copy of the reference stored in the hash table (which is
- * the proc existing).  Need to do this while locking the table, in case someone
- * else subsequently removes it from the table, then kref_put()s it to 0 before
- * we can get it.  Don't push the locking into the hashtable without dealing
- * with this. */
+ * This uses get_not_zero, since it is possible the refcnt is 0, which means the
+ * process is dying and we should not have the ref (and thus return 0).  We need
+ * to lock to protect us from getting p, (someone else removes and frees p),
+ * then get_not_zero() on p.
+ * Don't push the locking into the hashtable without dealing with this. */
 struct proc *pid2proc(pid_t pid)
 {
        spin_lock(&pid_hash_lock);
        struct proc *p = hashtable_search(pid_hash, (void*)pid);
        if (p)
-               kref_get(&p->kref, 1);
+               if (!kref_get_not_zero(&p->kref, 1))
+                       p = 0;
        spin_unlock(&pid_hash_lock);
        return p;
 }
@@ -256,28 +257,23 @@ error_t proc_alloc(struct proc **pp, struct proc *parent)
 
        { INITSTRUCT(*p)
 
+       /* one reference for the proc existing, and one for the ref we pass back. */
+       kref_init(&p->kref, __proc_free, 2);
        // Setup the default map of where to get cache colors from
        p->cache_colors_map = global_cache_colors_map;
        p->next_cache_color = 0;
-
        /* Initialize the address space */
        if ((r = env_setup_vm(p)) < 0) {
                kmem_cache_free(proc_cache, p);
                return r;
        }
-
-       /* Get a pid, then store a reference in the pid_hash */
        if (!(p->pid = get_free_pid())) {
                kmem_cache_free(proc_cache, p);
                return -ENOFREEPID;
        }
-       /* one reference for the proc existing (in the hash table),
-        * and one for the ref we pass back */
-       kref_init(&p->kref, __proc_free, 2);
        spin_lock(&pid_hash_lock);
        hashtable_insert(pid_hash, (void*)p->pid, p);
        spin_unlock(&pid_hash_lock);
-
        /* Set the basic status variables. */
        spinlock_init(&p->proc_lock);
        p->exitcode = 0;
@@ -376,7 +372,11 @@ static void __proc_free(struct kref *kref)
                        cache_color_free(llc_cache, p->cache_colors_map);
                cache_colors_map_free(p->cache_colors_map);
        }
-       /* Give our PID back */
+       /* Remove us from the pid_hash and give our PID back (in that order). */
+       spin_lock(&pid_hash_lock);
+       if (!hashtable_remove(pid_hash, (void*)p->pid))
+               panic("Proc not in the pid table in %s", __FUNCTION__);
+       spin_unlock(&pid_hash_lock);
        put_free_pid(p->pid);
        /* Flush all mapped pages in the user portion of the address space */
        env_user_mem_free(p, 0, UVPT);
@@ -634,25 +634,13 @@ void proc_destroy(struct proc *p)
                              __FUNCTION__);
        }
        __proc_set_state(p, PROC_DYING);
-       /* This kref_put() is for the process in general (its existence in the hash
-        * table).  Note we do it after unlocking the hash table, since once it is
-        * gone, no one can get it to kref_get() it.  We also do it after unlocking,
-        * since it is possible that we are the releaser (though not when a
-        * self_ipi is pending, so only when death was remote (we're not current)).
-        *
-        * Also note that we don't give the PID back until __proc_free().  This is
-        * because not everyone is done with the process yet, although you won't
-        * find the proc in any lists, nor will it get reused anytime soon. */
-       spin_lock(&pid_hash_lock);
-       if (!hashtable_remove(pid_hash, (void*)p->pid))
-               panic("Proc not in the pid table in %s", __FUNCTION__);
-       spin_unlock(&pid_hash_lock);
+       /* This kref_put() is for the process's existence. */
+       kref_put(&p->kref);
        /* 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). */
        spin_unlock(&p->proc_lock);
-       kref_put(&p->kref);             /* for the hashtable ref */
        /* at this point, we normally have one ref to be eaten in kmsg_pending and
         * one for every 'current'.  and maybe one for a parent */
        __proc_kmsg_pending(p, self_ipi_pending);