kernel - Fix race in multi-LWP exit
authorMatthew Dillon <dillon@apollo.backplane.com>
Tue, 29 Nov 2011 06:13:38 +0000 (22:13 -0800)
committerMatthew Dillon <dillon@apollo.backplane.com>
Tue, 29 Nov 2011 06:13:38 +0000 (22:13 -0800)
* Fix a very small race condition after TDF_EXITING could get set where a
  LWPs thread could get destroyed by another cpu (reaping it) before
  the thread actually finished exiting.

* Clean up a case where we were improperly testing TDF_RUNQ which could
  result in unnecessary 1-tick delays in exit reaping.

* Beef up the assertion when TDF_EXITING is found to be set that
  both TDF_RUNQ and TDF_TSLEEPQ are not set (before it was just checking
  TDF_TSLEEPQ).

* Clean up reaplwp().

* Remove lwkt_wait_free()

* lwkt_free_thread() now waits for td_refs to drop to 0 before freeing
  the thread.

sys/kern/kern_exit.c
sys/kern/lwkt_thread.c

index d43daee..bcf7f64 100644 (file)
@@ -291,6 +291,7 @@ exit1(int rv)
        }
        varsymset_clean(&p->p_varsymset);
        lockuninit(&p->p_varsymset.vx_lock);
+
        /*
         * Kill all lwps associated with the current process, return an
         * error if we race another thread trying to do the same thing
@@ -652,6 +653,9 @@ lwp_exit(int masterexit)
         * synchronously, which is much faster.
         *
         * Wakeup anyone waiting on p_nthreads to drop to 1 or 0.
+        *
+        * The process is left held until the reaper calls lwp_dispose() on
+        * the lp (after calling lwp_wait()).
         */
        if (masterexit == 0) {
                lwp_rb_tree_RB_REMOVE(&p->p_lwp_tree, lp);
@@ -681,15 +685,15 @@ lwp_exit(int masterexit)
 }
 
 /*
- * Wait until a lwp is completely dead.
+ * Wait until a lwp is completely dead.  The final interlock in this drama
+ * is when TDF_EXITING is set in cpu_thread_exit() just before the final
+ * switchout.
  *
- * If the thread is still executing, which can't be waited upon,
- * return failure.  The caller is responsible of waiting a little
- * bit and checking again.
+ * At the point TDF_EXITING is set a complete exit is accomplished when
+ * TDF_RUNNING and TDF_PREEMPT_LOCK are both clear.
  *
- * Suggested use:
- * while (!lwp_wait(lp))
- *     tsleep(lp, 0, "lwpwait", 1);
+ * Returns non-zero on success, and zero if the caller needs to retry
+ * the lwp_wait().
  */
 static int
 lwp_wait(struct lwp *lp)
@@ -698,10 +702,24 @@ lwp_wait(struct lwp *lp)
 
        KKASSERT(lwkt_preempted_proc() != lp);
 
-       while (lp->lwp_lock > 0)
+       /*
+        * Wait until the lp has entered its low level exit and wait
+        * until other cores with refs on the lp (e.g. for ps or signaling)
+        * release them.
+        */
+       if (lp->lwp_lock > 0) {
                tsleep(lp, 0, "lwpwait1", 1);
+               return(0);
+       }
 
-       lwkt_wait_free(td);
+       /*
+        * Wait until the thread is no longer references and no longer
+        * runnable or preempted (i.e. finishes its low level exit).
+        */
+       if (td->td_refs) {
+               tsleep(td, 0, "lwpwait2", 1);
+               return(0);
+       }
 
        /*
         * The lwp's thread may still be in the middle
@@ -716,12 +734,15 @@ lwp_wait(struct lwp *lp)
         * and let the caller deal with sleeping and calling
         * us again.
         */
-       if ((td->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK|
-                            TDF_EXITING|TDF_RUNQ)) != TDF_EXITING) {
+       if ((td->td_flags & (TDF_RUNNING |
+                            TDF_PREEMPT_LOCK |
+                            TDF_EXITING)) != TDF_EXITING) {
+               tsleep(lp, 0, "lwpwait2", 1);
                return (0);
        }
-       KASSERT((td->td_flags & TDF_TSLEEPQ) == 0,
-               ("lwp_wait: td %p (%s) still on sleep queue", td, td->td_comm));
+       KASSERT((td->td_flags & (TDF_RUNQ|TDF_TSLEEPQ)) == 0,
+               ("lwp_wait: td %p (%s) still on run or sleep queue",
+               td, td->td_comm));
        return (1);
 }
 
@@ -736,8 +757,9 @@ lwp_dispose(struct lwp *lp)
 
        KKASSERT(lwkt_preempted_proc() != lp);
        KKASSERT(td->td_refs == 0);
-       KKASSERT((td->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK|TDF_EXITING)) ==
-                TDF_EXITING);
+       KKASSERT((td->td_flags & (TDF_RUNNING |
+                                 TDF_PREEMPT_LOCK |
+                                 TDF_EXITING)) == TDF_EXITING);
 
        PRELE(lp->lwp_proc);
        lp->lwp_proc = NULL;
@@ -1116,11 +1138,8 @@ reaplwps(void *context, int dummy)
 static void
 reaplwp(struct lwp *lp)
 {
-       if (lwp_wait(lp) == 0) {
-               tsleep_interlock(lp, 0);
-               while (lwp_wait(lp) == 0)
-                       tsleep(lp, PINTERLOCKED, "lwpreap", 1);
-       }
+       while (lwp_wait(lp) == 0)
+               ;
        lwp_dispose(lp);
 }
 
@@ -1131,7 +1150,8 @@ deadlwp_init(void)
 
        for (cpu = 0; cpu < ncpus; cpu++) {
                LIST_INIT(&deadlwp_list[cpu]);
-               deadlwp_task[cpu] = kmalloc(sizeof(*deadlwp_task[cpu]), M_DEVBUF, M_WAITOK);
+               deadlwp_task[cpu] = kmalloc(sizeof(*deadlwp_task[cpu]),
+                                           M_DEVBUF, M_WAITOK);
                TASK_INIT(deadlwp_task[cpu], 0, reaplwps, &deadlwp_list[cpu]);
        }
 }
index 456323f..ddc782c 100644 (file)
@@ -401,7 +401,8 @@ lwkt_alloc_thread(struct thread *td, int stksize, int cpu, int flags)
        }
        crit_exit_gd(gd);
        KASSERT((td->td_flags &
-                (TDF_ALLOCATED_THREAD|TDF_RUNNING)) == TDF_ALLOCATED_THREAD,
+                (TDF_ALLOCATED_THREAD|TDF_RUNNING|TDF_PREEMPT_LOCK)) ==
+                TDF_ALLOCATED_THREAD,
                ("lwkt_alloc_thread: corrupted td flags 0x%X", td->td_flags));
        flags |= td->td_flags & (TDF_ALLOCATED_THREAD|TDF_ALLOCATED_STACK);
     }
@@ -536,13 +537,6 @@ lwkt_rele(thread_t td)
 }
 
 void
-lwkt_wait_free(thread_t td)
-{
-    while (td->td_refs)
-       tsleep(td, 0, "tdreap", hz);
-}
-
-void
 lwkt_free_thread(thread_t td)
 {
     KKASSERT(td->td_refs == 0);
@@ -1694,15 +1688,24 @@ lwkt_exit(void)
      *
      * We have to cache the current td in gd_freetd because objcache_put()ing
      * it would rip it out from under us while our thread is still active.
+     *
+     * We are the current thread so of course our own TDF_RUNNING bit will
+     * be set, so unlike the lwp reap code we don't wait for it to clear.
      */
     gd = mycpu;
     crit_enter_quick(td);
-    lwkt_wait_free(td);
-    while ((std = gd->gd_freetd) != NULL) {
-       KKASSERT((std->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK)) == 0);
-       gd->gd_freetd = NULL;
-       objcache_put(thread_cache, std);
-       lwkt_wait_free(td);
+    for (;;) {
+       if (td->td_refs) {
+           tsleep(td, 0, "tdreap", 1);
+           continue;
+       }
+       if ((std = gd->gd_freetd) != NULL) {
+           KKASSERT((std->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK)) == 0);
+           gd->gd_freetd = NULL;
+           objcache_put(thread_cache, std);
+           continue;
+       }
+       break;
     }
 
     /*