Windows: osi_mutex / osi_rwlock changes

Reorganize the osi_mutex and osi_rwlock structure so
that all counters are 32-bit and pointers are
aligned.  This requires adding padding fields.

Move lock validation checks within the critical section.

Include additional assertions checking the ownership
state and protecting against under/overflows.

Increase the size of the rwlock tid array to support
a larger number of simultaneous readers.

Change-Id: Ia46684c601a1a589a210a36862ae6ad6448a435e
Reviewed-on: http://gerrit.openafs.org/6130
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>

diff --git a/src/WINNT/client_osi/osibasel.c b/src/WINNT/client_osi/osibasel.c
index d6916e3a6f08dfcd7491e57dcf9857f3a6844c15..351a6f78f115d15a205062710d39b0b37d80d610 100644 (file)
--- a/src/WINNT/client_osi/osibasel.c
+++ b/src/WINNT/client_osi/osibasel.c
@@ -154,6 +154,7 @@ void lock_ObtainWrite(osi_rwlock_t *lockp)
     CRITICAL_SECTION *csp;
     osi_queue_t * lockRefH, *lockRefT;
     osi_lock_ref_t *lockRefp;
+    DWORD tid = thrd_Current();
 
     if ((i=lockp->type) != 0) {
         if (i >= 0 && i < OSI_NLOCKTYPES)
@@ -173,18 +174,29 @@ void lock_ObtainWrite(osi_rwlock_t *lockp)
     csp = &osi_baseAtomicCS[lockp->atomicIndex];
     EnterCriticalSection(csp);
 
+    if (lockp->flags & OSI_LOCKFLAG_EXCL) {
+        osi_assertx(lockp->tid[0] != tid, "OSI_RWLOCK_WRITEHELD");
+    } else {
+        for ( i=0; i < lockp->readers && i < OSI_RWLOCK_THREADS; i++ ) {
+            osi_assertx(lockp->tid[i] != tid, "OSI_RWLOCK_READHELD");
+        }
+    }
+
     /* here we have the fast lock, so see if we can obtain the real lock */
     if (lockp->waiters > 0 || (lockp->flags & OSI_LOCKFLAG_EXCL) ||
         (lockp->readers > 0)) {
         lockp->waiters++;
         osi_TWait(&lockp->d.turn, OSI_SLEEPINFO_W4WRITE, &lockp->flags, lockp->tid, csp);
         lockp->waiters--;
+        osi_assertx(lockp->waiters >= 0, "waiters underflow");
         osi_assert(lockp->readers == 0 && (lockp->flags & OSI_LOCKFLAG_EXCL));
     } else {
         /* if we're here, all clear to set the lock */
         lockp->flags |= OSI_LOCKFLAG_EXCL;
-        lockp->tid[0] = thrd_Current();
+        lockp->tid[0] = tid;
     }
+    osi_assertx(lockp->readers == 0, "write lock readers present");
+
     LeaveCriticalSection(csp);
 
     if (lockOrderValidation) {
@@ -221,8 +233,12 @@ void lock_ObtainRead(osi_rwlock_t *lockp)
     csp = &osi_baseAtomicCS[lockp->atomicIndex];
     EnterCriticalSection(csp);
 
-    for ( i=0; i < lockp->readers; i++ ) {
-        osi_assertx(lockp->tid[i] != tid, "OSI_RWLOCK_READHELD");
+    if (lockp->flags & OSI_LOCKFLAG_EXCL) {
+        osi_assertx(lockp->tid[0] != tid, "OSI_RWLOCK_WRITEHELD");
+    } else {
+        for ( i=0; i < lockp->readers && i < OSI_RWLOCK_THREADS; i++ ) {
+            osi_assertx(lockp->tid[i] != tid, "OSI_RWLOCK_READHELD");
+        }
     }
 
     /* here we have the fast lock, so see if we can obtain the real lock */
@@ -230,6 +246,7 @@ void lock_ObtainRead(osi_rwlock_t *lockp)
         lockp->waiters++;
         osi_TWait(&lockp->d.turn, OSI_SLEEPINFO_W4READ, &lockp->readers, lockp->tid, csp);
         lockp->waiters--;
+        osi_assertx(lockp->waiters >= 0, "waiters underflow");
         osi_assert(!(lockp->flags & OSI_LOCKFLAG_EXCL) && lockp->readers > 0);
     } else {
         /* if we're here, all clear to set the lock */
@@ -260,6 +277,10 @@ void lock_ReleaseRead(osi_rwlock_t *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         int found = 0;
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
@@ -279,10 +300,6 @@ void lock_ReleaseRead(osi_rwlock_t *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->readers > 0, "read lock not held");
 
     for ( i=0; i < lockp->readers; i++) {
@@ -295,10 +312,12 @@ void lock_ReleaseRead(osi_rwlock_t *lockp)
     }
 
     /* releasing a read lock can allow readers or writers */
-    if (--lockp->readers == 0 && !osi_TEmpty(&lockp->d.turn)) {
+    if (--(lockp->readers) == 0 && !osi_TEmpty(&lockp->d.turn)) {
         osi_TSignalForMLs(&lockp->d.turn, 0, csp);
     }
     else {
+        osi_assertx(lockp->readers >= 0, "read lock underflow");
+
         /* and finally release the big lock */
         LeaveCriticalSection(csp);
     }
@@ -317,6 +336,10 @@ void lock_ReleaseWrite(osi_rwlock_t *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         int found = 0;
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
@@ -336,10 +359,6 @@ void lock_ReleaseWrite(osi_rwlock_t *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->flags & OSI_LOCKFLAG_EXCL, "write lock not held");
     osi_assertx(lockp->tid[0] == thrd_Current(), "write lock not held by current thread");
 
@@ -377,6 +396,8 @@ void lock_ConvertWToR(osi_rwlock_t *lockp)
     lockp->flags &= ~OSI_LOCKFLAG_EXCL;
     lockp->readers++;
 
+    osi_assertx(lockp->readers == 1, "read lock not one");
+
     if (!osi_TEmpty(&lockp->d.turn)) {
         osi_TSignalForMLs(&lockp->d.turn, /* still have readers */ 1, csp);
     }
@@ -414,14 +435,17 @@ void lock_ConvertRToW(osi_rwlock_t *lockp)
         }
     }
 
-    if (--lockp->readers == 0) {
+    if (--(lockp->readers) == 0) {
         /* convert read lock to write lock */
         lockp->flags |= OSI_LOCKFLAG_EXCL;
         lockp->tid[0] = tid;
     } else {
+        osi_assertx(lockp->readers > 0, "read lock underflow");
+
         lockp->waiters++;
         osi_TWait(&lockp->d.turn, OSI_SLEEPINFO_W4WRITE, &lockp->flags, lockp->tid, csp);
         lockp->waiters--;
+        osi_assertx(lockp->waiters >= 0, "waiters underflow");
         osi_assert(lockp->readers == 0 && (lockp->flags & OSI_LOCKFLAG_EXCL));
     }
 
@@ -441,6 +465,10 @@ void lock_ObtainMutex(struct osi_mutex *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -449,21 +477,19 @@ void lock_ObtainMutex(struct osi_mutex *lockp)
             lock_VerifyOrderMX(lockRefH, lockRefT, lockp);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     /* here we have the fast lock, so see if we can obtain the real lock */
     if (lockp->waiters > 0 || (lockp->flags & OSI_LOCKFLAG_EXCL)) {
         lockp->waiters++;
         osi_TWait(&lockp->d.turn, OSI_SLEEPINFO_W4WRITE, &lockp->flags, &lockp->tid, csp);
         lockp->waiters--;
+        osi_assertx(lockp->waiters >= 0, "waiters underflow");
         osi_assert(lockp->flags & OSI_LOCKFLAG_EXCL);
     } else {
         /* if we're here, all clear to set the lock */
         lockp->flags |= OSI_LOCKFLAG_EXCL;
         lockp->tid = thrd_Current();
     }
+
     LeaveCriticalSection(csp);
 
     if (lockOrderValidation) {
@@ -487,6 +513,10 @@ void lock_ReleaseMutex(struct osi_mutex *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         int found = 0;
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
@@ -506,10 +536,6 @@ void lock_ReleaseMutex(struct osi_mutex *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->flags & OSI_LOCKFLAG_EXCL, "mutex not held");
     osi_assertx(lockp->tid == thrd_Current(), "mutex not held by current thread");
 
@@ -535,6 +561,10 @@ int lock_TryRead(struct osi_rwlock *lockp)
         if (i >= 0 && i < OSI_NLOCKTYPES)
             return (osi_lockOps[i]->TryReadProc)(lockp);
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -548,17 +578,13 @@ int lock_TryRead(struct osi_rwlock *lockp)
         }
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     /* here we have the fast lock, so see if we can obtain the real lock */
     if (lockp->waiters > 0 || (lockp->flags & OSI_LOCKFLAG_EXCL)) {
         i = 0;
     }
     else {
         /* if we're here, all clear to set the lock */
-        if (++lockp->readers < OSI_RWLOCK_THREADS)
+        if (++(lockp->readers) < OSI_RWLOCK_THREADS)
             lockp->tid[lockp->readers-1] = thrd_Current();
         i = 1;
     }
@@ -587,6 +613,10 @@ int lock_TryWrite(struct osi_rwlock *lockp)
         if (i >= 0 && i < OSI_NLOCKTYPES)
             return (osi_lockOps[i]->TryWriteProc)(lockp);
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -600,10 +630,6 @@ int lock_TryWrite(struct osi_rwlock *lockp)
         }
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     /* here we have the fast lock, so see if we can obtain the real lock */
     if (lockp->waiters > 0 || (lockp->flags & OSI_LOCKFLAG_EXCL)
          || (lockp->readers > 0)) {
@@ -639,6 +665,10 @@ int lock_TryMutex(struct osi_mutex *lockp) {
         if (i >= 0 && i < OSI_NLOCKTYPES)
             return (osi_lockOps[i]->TryMutexProc)(lockp);
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -652,10 +682,6 @@ int lock_TryMutex(struct osi_mutex *lockp) {
         }
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     /* here we have the fast lock, so see if we can obtain the real lock */
     if (lockp->waiters > 0 || (lockp->flags & OSI_LOCKFLAG_EXCL)) {
         i = 0;
@@ -675,6 +701,7 @@ int lock_TryMutex(struct osi_mutex *lockp) {
         TlsSetValue(tls_LockRefH, lockRefH);
         TlsSetValue(tls_LockRefT, lockRefT);
     }
+
     return i;
 }
 
@@ -692,6 +719,10 @@ void osi_SleepR(LONG_PTR sleepVal, struct osi_rwlock *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -708,10 +739,6 @@ void osi_SleepR(LONG_PTR sleepVal, struct osi_rwlock *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->readers > 0, "osi_SleepR: not held");
 
     for ( i=0; i < lockp->readers; i++) {
@@ -726,7 +753,7 @@ void osi_SleepR(LONG_PTR sleepVal, struct osi_rwlock *lockp)
     /* XXX better to get the list of things to wakeup from TSignalForMLs, and
      * then do the wakeup after SleepSpin releases the low-level mutex.
      */
-    if (--lockp->readers == 0 && !osi_TEmpty(&lockp->d.turn)) {
+    if (--(lockp->readers) == 0 && !osi_TEmpty(&lockp->d.turn)) {
         osi_TSignalForMLs(&lockp->d.turn, 0, NULL);
     }
 
@@ -748,6 +775,10 @@ void osi_SleepW(LONG_PTR sleepVal, struct osi_rwlock *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -764,10 +795,6 @@ void osi_SleepW(LONG_PTR sleepVal, struct osi_rwlock *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->flags & OSI_LOCKFLAG_EXCL, "osi_SleepW: not held");
 
     lockp->flags &= ~OSI_LOCKFLAG_EXCL;
@@ -793,6 +820,10 @@ void osi_SleepM(LONG_PTR sleepVal, struct osi_mutex *lockp)
         return;
     }
 
+    /* otherwise we're the fast base type */
+    csp = &osi_baseAtomicCS[lockp->atomicIndex];
+    EnterCriticalSection(csp);
+
     if (lockOrderValidation && lockp->level != 0) {
         lockRefH = (osi_queue_t *)TlsGetValue(tls_LockRefH);
         lockRefT = (osi_queue_t *)TlsGetValue(tls_LockRefT);
@@ -809,10 +840,6 @@ void osi_SleepM(LONG_PTR sleepVal, struct osi_mutex *lockp)
         TlsSetValue(tls_LockRefT, lockRefT);
     }
 
-    /* otherwise we're the fast base type */
-    csp = &osi_baseAtomicCS[lockp->atomicIndex];
-    EnterCriticalSection(csp);
-
     osi_assertx(lockp->flags & OSI_LOCKFLAG_EXCL, "osi_SleepM not held");
 
     lockp->flags &= ~OSI_LOCKFLAG_EXCL;
@@ -853,12 +880,11 @@ void lock_InitializeMutex(osi_mutex_t *mp, char *namep, unsigned short level)
         return;
     }
 
-    /* otherwise we have the base case, which requires no special
+    /*
+     * otherwise we have the base case, which requires no special
      * initialization.
      */
-    mp->type = 0;
-    mp->flags = 0;
-    mp->tid = 0;
+    memset(mp, 0, sizeof(osi_mutex_t));
     mp->atomicIndex = (unsigned short)(InterlockedIncrement(&atomicIndexCounter) % OSI_MUTEXHASHSIZE);
     mp->level = level;
     osi_TInit(&mp->d.turn);

diff --git a/src/WINNT/client_osi/osibasel.h b/src/WINNT/client_osi/osibasel.h
index 9ef6b90519240226a45fe556fc2e89d5a1ae9d02..410668d3cd8359f85eaf24d56f331f7742e5163f 100644 (file)
--- a/src/WINNT/client_osi/osibasel.h
+++ b/src/WINNT/client_osi/osibasel.h
@@ -28,17 +28,18 @@
  * lock using an atomic increment operation.
  */
 typedef struct osi_mutex {
-    char type;                 /* for all types; type 0 uses atomic count */
-    char flags;                        /* flags for base type */
+    short type;                        /* for all types; type 0 uses atomic count */
     unsigned short atomicIndex;        /* index of lock for low-level sync */
+    int flags;                 /* flags for base type */
     DWORD tid;                 /* tid of thread that owns the lock */
-    unsigned short waiters;    /* waiters */
-    unsigned short pad;
+    int waiters;               /* waiters */
+    unsigned short level;       /* locking hierarchy level */
+    short pad1;
+    int pad2;
     union {
         void *privateDatap;    /* data pointer for non-zero types */
         osi_turnstile_t turn;  /* turnstile */
     } d;
-    unsigned short level;       /* locking hierarchy level */
 } osi_mutex_t;
 
 /* a read/write lock.  This structure has two forms.  In the
@@ -54,20 +55,21 @@ typedef struct osi_mutex {
  * This type of lock has N readers or one writer.
  */
 
-#define OSI_RWLOCK_THREADS 32
+#define OSI_RWLOCK_THREADS 64
 
 typedef struct osi_rwlock {
-    char type;                 /* for all types; type 0 uses atomic count */
-    char flags;                        /* flags for base type */
+    short type;                        /* for all types; type 0 uses atomic count */
     unsigned short atomicIndex;        /* index into hash table for low-level sync */
-    DWORD tid[OSI_RWLOCK_THREADS];                     /* writer's tid */
-    unsigned short waiters;    /* waiters */
-    unsigned short readers;    /* readers */
+    int flags;                  /* flags */
+    int waiters;               /* waiters */
+    int readers;               /* readers */
+    DWORD tid[OSI_RWLOCK_THREADS];     /* writer's tid */
+    short pad2;
+    unsigned short level;       /* locking hierarchy level */
     union {
         void *privateDatap;    /* data pointer for non-zero types */
         osi_turnstile_t turn;  /* turnstile */
     } d;
-    unsigned short level;       /* locking hierarchy level */
 } osi_rwlock_t;