Fix a MP lock race. The MP locking state can change when lwkt_chktokens()
authorMatthew Dillon <dillon@dragonflybsd.org>
Tue, 19 Jul 2005 19:08:05 +0000 (19:08 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Tue, 19 Jul 2005 19:08:05 +0000 (19:08 +0000)
returns a failure.  The state is not retained by an interrupt because we
are trying to set it for the new thread rather then the current thread, and
the current thread's mpcount could be 0.

Change ASSERT_MP_LOCK_HELD() to supply a thread for reporting purposes in
the KASSERT.

Reported-by: David Rhodus <sdrhodus@gmail.com>
sys/i386/i386/vm86.c
sys/i386/include/lock.h
sys/kern/lwkt_thread.c
sys/platform/pc32/i386/vm86.c
sys/platform/pc32/include/lock.h

index 4efb1d9..c9ca5da 100644 (file)
@@ -25,7 +25,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/vm86.c,v 1.31.2.2 2001/10/05 06:18:55 peter Exp $
- * $DragonFly: src/sys/i386/i386/Attic/vm86.c,v 1.13 2005/01/31 04:35:17 dillon Exp $
+ * $DragonFly: src/sys/i386/i386/Attic/vm86.c,v 1.14 2005/07/19 19:08:03 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -623,7 +623,7 @@ vm86_intcall(int intnum, struct vm86frame *vmf)
                return (EINVAL);
 
        crit_enter();
-       ASSERT_MP_LOCK_HELD();
+       ASSERT_MP_LOCK_HELD(curthread);
 
        vm86_setup_timer_fault();
        vmf->vmf_trapno = intnum;
@@ -662,7 +662,7 @@ vm86_datacall(intnum, vmf, vmc)
        int i, entry, retval;
 
        crit_enter();
-       ASSERT_MP_LOCK_HELD();
+       ASSERT_MP_LOCK_HELD(curthread);
 
        for (i = 0; i < vmc->npages; i++) {
                page = vtophys(vmc->pmap[i].kva & PG_FRAME);
index c98ab02..450b056 100644 (file)
@@ -32,7 +32,7 @@
  * SUCH DAMAGE.
  * 
  * $FreeBSD: src/sys/i386/include/lock.h,v 1.11.2.2 2000/09/30 02:49:34 ps Exp $
- * $DragonFly: src/sys/i386/include/Attic/lock.h,v 1.10 2004/11/20 20:50:36 dillon Exp $
+ * $DragonFly: src/sys/i386/include/Attic/lock.h,v 1.11 2005/07/19 19:08:04 dillon Exp $
  */
 
 #ifndef _MACHINE_LOCK_H_
@@ -196,7 +196,7 @@ void        cpu_get_initial_mplock(void);
 extern u_int   mp_lock;
 
 #define MP_LOCK_HELD()   (mp_lock == mycpu->gd_cpuid)
-#define ASSERT_MP_LOCK_HELD()   KKASSERT(MP_LOCK_HELD())
+#define ASSERT_MP_LOCK_HELD(td)   KASSERT(MP_LOCK_HELD(), ("MP_LOCK_HELD(): not held thread %p", td))
 
 static __inline void
 cpu_rel_mplock(void)
@@ -209,7 +209,7 @@ cpu_rel_mplock(void)
 #define get_mplock()
 #define try_mplock()   1
 #define rel_mplock()
-#define ASSERT_MP_LOCK_HELD()
+#define ASSERT_MP_LOCK_HELD(td)
 
 #endif /* SMP */
 #endif  /* _KERNEL || _UTHREAD */
index c50aac4..2906b8c 100644 (file)
@@ -31,7 +31,7 @@
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  * 
- * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.76 2005/07/07 20:28:26 hmp Exp $
+ * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.77 2005/07/19 19:08:05 dillon Exp $
  */
 
 /*
@@ -576,26 +576,45 @@ again:
             * or if the target is holding tokens and we could not 
             * gain ownership of the tokens, continue looking for a
             * thread to schedule and spin instead of HLT if we can't.
+            *
+            * NOTE: the mpheld variable invalid after this conditional, it
+            * can change due to both cpu_try_mplock() returning success
+            * AND interactions in lwkt_chktokens() due to the fact that
+            * we are trying to check the mpcount of a thread other then
+            * the current thread.  Because of this, if the current thread
+            * is not holding td_mpcount, an IPI indirectly run via
+            * lwkt_chktokens() can obtain and release the MP lock and
+            * cause the core MP lock to be released. 
             */
            if ((ntd->td_mpcount && mpheld == 0 && !cpu_try_mplock()) ||
                (ntd->td_toks && lwkt_chktokens(ntd) == 0)
            ) {
                u_int32_t rqmask = gd->gd_runqmask;
+
+               mpheld = MP_LOCK_HELD();
+               ntd = NULL;
                while (rqmask) {
                    TAILQ_FOREACH(ntd, &gd->gd_tdrunq[nq], td_threadq) {
                        if (ntd->td_mpcount && !mpheld && !cpu_try_mplock()) {
-                               /* spinning due to MP lock being held */
+                           /* spinning due to MP lock being held */
 #ifdef INVARIANTS
-                               ++mplock_contention_count;
+                           ++mplock_contention_count;
 #endif
+                           /* mplock still not held, 'mpheld' still valid */
                            continue;
                        }
-                       mpheld = MP_LOCK_HELD();
+
+                       /*
+                        * mpheld state invalid after chktokens call returns
+                        * failure, but the variable is only needed for
+                        * the loop.
+                        */
                        if (ntd->td_toks && !lwkt_chktokens(ntd)) {
-                               /* spinning due to token contention */
+                           /* spinning due to token contention */
 #ifdef INVARIANTS
-                               ++token_contention_count;
+                           ++token_contention_count;
 #endif
+                           mpheld = MP_LOCK_HELD();
                            continue;
                        }
                        break;
@@ -608,6 +627,7 @@ again:
                if (ntd == NULL) {
                    ntd = &gd->gd_idlethread;
                    ntd->td_flags |= TDF_IDLE_NOHLT;
+                   KASSERT(ntd->td_mpcount == 0, ("Idlex thread %p was holding the BGL!", ntd));
                } else {
                    TAILQ_REMOVE(&gd->gd_tdrunq[nq], ntd, td_threadq);
                    TAILQ_INSERT_TAIL(&gd->gd_tdrunq[nq], ntd, td_threadq);
@@ -628,6 +648,9 @@ again:
            ntd = &gd->gd_idlethread;
            if (gd->gd_reqflags & RQF_IDLECHECK_MASK)
                ntd->td_flags |= TDF_IDLE_NOHLT;
+#ifdef SMP
+           KASSERT(ntd->td_mpcount == 0, ("Idley thread %p was holding the BGL!", ntd));
+#endif
        }
     }
     KASSERT(ntd->td_pri >= TDPRI_CRIT,
@@ -643,7 +666,7 @@ again:
        if (MP_LOCK_HELD())
            cpu_rel_mplock();
     } else {
-       ASSERT_MP_LOCK_HELD();
+       ASSERT_MP_LOCK_HELD(ntd);
     }
 #endif
     if (td != ntd) {
index c7cd104..28242fb 100644 (file)
@@ -25,7 +25,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/vm86.c,v 1.31.2.2 2001/10/05 06:18:55 peter Exp $
- * $DragonFly: src/sys/platform/pc32/i386/vm86.c,v 1.13 2005/01/31 04:35:17 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/i386/vm86.c,v 1.14 2005/07/19 19:08:03 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -623,7 +623,7 @@ vm86_intcall(int intnum, struct vm86frame *vmf)
                return (EINVAL);
 
        crit_enter();
-       ASSERT_MP_LOCK_HELD();
+       ASSERT_MP_LOCK_HELD(curthread);
 
        vm86_setup_timer_fault();
        vmf->vmf_trapno = intnum;
@@ -662,7 +662,7 @@ vm86_datacall(intnum, vmf, vmc)
        int i, entry, retval;
 
        crit_enter();
-       ASSERT_MP_LOCK_HELD();
+       ASSERT_MP_LOCK_HELD(curthread);
 
        for (i = 0; i < vmc->npages; i++) {
                page = vtophys(vmc->pmap[i].kva & PG_FRAME);
index e364be3..dfd932b 100644 (file)
@@ -32,7 +32,7 @@
  * SUCH DAMAGE.
  * 
  * $FreeBSD: src/sys/i386/include/lock.h,v 1.11.2.2 2000/09/30 02:49:34 ps Exp $
- * $DragonFly: src/sys/platform/pc32/include/lock.h,v 1.10 2004/11/20 20:50:36 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/include/lock.h,v 1.11 2005/07/19 19:08:04 dillon Exp $
  */
 
 #ifndef _MACHINE_LOCK_H_
@@ -196,7 +196,7 @@ void        cpu_get_initial_mplock(void);
 extern u_int   mp_lock;
 
 #define MP_LOCK_HELD()   (mp_lock == mycpu->gd_cpuid)
-#define ASSERT_MP_LOCK_HELD()   KKASSERT(MP_LOCK_HELD())
+#define ASSERT_MP_LOCK_HELD(td)   KASSERT(MP_LOCK_HELD(), ("MP_LOCK_HELD(): not held thread %p", td))
 
 static __inline void
 cpu_rel_mplock(void)
@@ -209,7 +209,7 @@ cpu_rel_mplock(void)
 #define get_mplock()
 #define try_mplock()   1
 #define rel_mplock()
-#define ASSERT_MP_LOCK_HELD()
+#define ASSERT_MP_LOCK_HELD(td)
 
 #endif /* SMP */
 #endif  /* _KERNEL || _UTHREAD */