Fix a serious SMP bug. The RWLOCK support used by dev/raid/aac,
authorMatthew Dillon <dillon@dragonflybsd.org>
Mon, 20 Jun 2005 07:31:05 +0000 (07:31 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Mon, 20 Jun 2005 07:31:05 +0000 (07:31 +0000)
ACPI, and dev/drm improperly tries to use a token in lwkt_schedule().
Because lwkt_schedule() can be called from an IPI in the context of
the current thread it cannot safely manipulate the current thread,
much less the td_toks field.

To fix this problem the RW lock functions in lwkt_rwlock.c have to be
covered by a critical section as well as the token, and lwkt_schedule()
then simply checks to see if the token is owned by the current cpu.  RWLocks
may require some rethinking.

The token code is designed to operate mostly without having to use a
critical section, but in order for this to work it depends on the td_toks
list not being ripped out from under it.  The nested trytoken/release
call in lwkt_schedule() should not have caused mainline token code to
fail as it restored td_toks to the same state it was in on entry, but
we can't take that chance.

sys/kern/lwkt_rwlock.c
sys/kern/lwkt_thread.c

index 28047cf..260bf46 100644 (file)
@@ -35,7 +35,7 @@
  * 
  * Implements simple shared/exclusive locks using LWKT. 
  *
- * $DragonFly: src/sys/kern/Attic/lwkt_rwlock.c,v 1.7 2005/04/20 17:03:35 dillon Exp $
+ * $DragonFly: src/sys/kern/Attic/lwkt_rwlock.c,v 1.8 2005/06/20 07:31:05 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -44,6 +44,7 @@
 #include <sys/proc.h>
 #include <sys/rtprio.h>
 #include <sys/queue.h>
+#include <sys/thread2.h>
 
 /*
  * lwkt_rwlock_init() (MP SAFE)
@@ -70,6 +71,13 @@ lwkt_rwlock_uninit(lwkt_rwlock_t lock)
 
 /*
  * lwkt_exlock() (MP SAFE)
+ *
+ * NOTE: We need to use a critical section in addition to the token to
+ * interlock against IPI lwkt_schedule calls which may manipulate the
+ * rw_wait structure's list.  This is because the IPI runs in the context of
+ * the current thread and thus cannot use any token calls (if it did the
+ * token would just share with the thread's token and not provide any 
+ * protection).  This needs a rewrite.
  */
 void
 lwkt_exlock(lwkt_rwlock_t lock, const char *wmesg)
@@ -78,6 +86,7 @@ lwkt_exlock(lwkt_rwlock_t lock, const char *wmesg)
     int gen;
 
     lwkt_gettoken(&ilock, &lock->rw_token);
+    crit_enter();
     gen = lock->rw_wait.wa_gen;
     while (lock->rw_owner != curthread) {
        if (lock->rw_owner == NULL && lock->rw_count == 0) {
@@ -90,6 +99,7 @@ lwkt_exlock(lwkt_rwlock_t lock, const char *wmesg)
     }
     ++lock->rw_count;
     lwkt_reltoken(&ilock);
+    crit_exit();
 }
 
 /*
@@ -102,6 +112,7 @@ lwkt_shlock(lwkt_rwlock_t lock, const char *wmesg)
     int gen;
 
     lwkt_gettoken(&ilock, &lock->rw_token);
+    crit_enter();
     gen = lock->rw_wait.wa_gen;
     while (lock->rw_owner != NULL) {
        ++lock->rw_requests;
@@ -110,6 +121,7 @@ lwkt_shlock(lwkt_rwlock_t lock, const char *wmesg)
     }
     ++lock->rw_count;
     lwkt_reltoken(&ilock);
+    crit_exit();
 }
 
 /*
@@ -121,6 +133,7 @@ lwkt_exunlock(lwkt_rwlock_t lock)
     lwkt_tokref ilock;
 
     lwkt_gettoken(&ilock, &lock->rw_token);
+    crit_enter();
     KASSERT(lock->rw_owner != NULL, ("lwkt_exunlock: shared lock"));
     KASSERT(lock->rw_owner == curthread, ("lwkt_exunlock: not owner"));
     if (--lock->rw_count == 0) {
@@ -129,6 +142,7 @@ lwkt_exunlock(lwkt_rwlock_t lock)
            lwkt_signal(&lock->rw_wait, 1);
     }
     lwkt_reltoken(&ilock);
+    crit_exit();
 }
 
 /*
@@ -140,11 +154,13 @@ lwkt_shunlock(lwkt_rwlock_t lock)
     lwkt_tokref ilock;
 
     lwkt_gettoken(&ilock, &lock->rw_token);
+    crit_enter();
     KASSERT(lock->rw_owner == NULL, ("lwkt_shunlock: exclusive lock"));
     if (--lock->rw_count == 0) {
        if (lock->rw_requests)
            lwkt_signal(&lock->rw_wait, 1);
     }
     lwkt_reltoken(&ilock);
+    crit_exit();
 }
 
index 9a58911..e20f8c4 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.74 2005/06/03 23:57:32 dillon Exp $
+ * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.75 2005/06/20 07:31:05 dillon Exp $
  */
 
 /*
@@ -911,14 +911,22 @@ lwkt_schedule(thread_t td)
         *
         * (remember, wait structures use stable storage)
         *
-        * NOTE: tokens no longer enter a critical section, so we only need
-        * to account for the crit_enter() above when calling
-        * _lwkt_schedule_post().
+        * NOTE: we have to account for the number of critical sections
+        * under our control when calling _lwkt_schedule_post() so it
+        * can figure out whether preemption is allowed.
+        *
+        * NOTE: The wait structure algorithms are a mess and need to be
+        * rewritten.
+        *
+        * NOTE: We cannot safely acquire or release a token, even 
+        * non-blocking, because this routine may be called in the context
+        * of a thread already holding the token and thus not provide any
+        * interlock protection.  We cannot safely manipulate the td_toks
+        * list for the same reason.  Instead we depend on our critical
+        * section if the token is owned by our cpu.
         */
        if ((w = td->td_wait) != NULL) {
-           lwkt_tokref wref;
-
-           if (lwkt_trytoken(&wref, &w->wa_token)) {
+           if (w->wa_token.t_cpu == mygd) {
                TAILQ_REMOVE(&w->wa_waitq, td, td_threadq);
                --w->wa_count;
                td->td_wait = NULL;
@@ -933,7 +941,6 @@ lwkt_schedule(thread_t td)
                _lwkt_enqueue(td);
                _lwkt_schedule_post(mygd, td, TDPRI_CRIT);
 #endif
-               lwkt_reltoken(&wref);
            } else {
                lwkt_send_ipiq(w->wa_token.t_cpu, (ipifunc_t)lwkt_schedule, td);
            }