Untested revamp of barrier.c.

        * barrier.c: Still more revamping. The exclusive access
        mutex isn't really needed so it has been removed and replaced
        by an InterlockedDecrement(). nSerial has been removed.
        iStep is now dual-purpose. The process shared attribute
        is now stored in the barrier struct.
        * implement.h (pthread_barrier_t_): Lost some/gained one
        elements.
        * private.c (ptw32_threadStart): Removed some comments.

5 files changed, 90 insertions, 107 deletions

diff --git a/ChangeLog b/ChangeLog
index 97ec503..5061a51 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2001-07-10  Ross Johnson  <rpj@setup1.ise.canberra.edu.au>

 

+	* barrier.c: Still more revamping. The exclusive access

+	mutex isn't really needed so it has been removed and replaced

+	by an InterlockedDecrement(). nSerial has been removed.

+	iStep is now dual-purpose. The process shared attribute

+	is now stored in the barrier struct.

+	* implement.h (pthread_barrier_t_): Lost some/gained one

+	elements.

+	* private.c (ptw32_threadStart): Removed some comments.

+

+2001-07-10  Ross Johnson  <rpj@setup1.ise.canberra.edu.au>

+

 	* barrier.c: Revamped to fix the race condition. Two alternating

 	semaphores are used instead of the PulseEvent. Also improved

 	overall throughput by returning PTHREAD_BARRIER_SERIAL_THREAD

diff --git a/barrier.c b/barrier.c
index e9312c9..3077e9b 100644
--- a/barrier.c
+++ b/barrier.c
@@ -2,7 +2,7 @@
  * barrier.c
  *
  * Description:
- * This translation unit implements spin locks primitives.
+ * This translation unit implements barrier primitives.
  *
  * Pthreads-win32 - POSIX Threads Library for Win32
  * Copyright (C) 1998
@@ -40,9 +40,7 @@ pthread_barrier_init(pthread_barrier_t * barrier,
                      const pthread_barrierattr_t * attr,
                      unsigned int count)
 {
-  int pshared = PTHREAD_PROCESS_PRIVATE;
   pthread_barrier_t b;
-  pthread_mutexattr_t ma;
 
   if (barrier == NULL || count == 0)
     {
@@ -51,33 +49,29 @@ pthread_barrier_init(pthread_barrier_t * barrier,
 
   if (NULL != (b = (pthread_barrier_t) calloc(1, sizeof(*b))))
     {
-      if (attr != NULL && *attr != NULL)
-        {
-          pshared = (*attr)->pshared;
-        }
+      b->pshared = (attr != NULL && *attr != NULL
+                    ? (*attr)->pshared
+                    : PTHREAD_PROCESS_PRIVATE);
 
       b->nCurrentBarrierHeight = b->nInitialBarrierHeight = count;
       b->iStep = 0;
-	b->nSerial = 0;
 
-      if (0 == pthread_mutexattr_init(&ma) &&
-          0 == pthread_mutexattr_setpshared(&ma, pshared))
+      /*
+       * Two semaphores are used in the same way as two stepping
+       * stones might be used in crossing a stream. Once all
+       * threads are safely on one stone, the other stone can
+       * be moved ahead, and the threads can start moving to it.
+       * If some threads decide to eat their lunch before moving
+       * then the other threads have to wait.
+       */
+      if (0 == sem_init(&(b->semBarrierBreeched[0]), b->pshared, 0))
         {
-          if (0 == pthread_mutex_init(&(b->mtxExclusiveAccess), &ma))
+          if (0 == sem_init(&(b->semBarrierBreeched[1]), b->pshared, 0))
             {
-              pthread_mutexattr_destroy(&ma);
-              if (0 == sem_init(&(b->semBarrierBreeched[0]), pshared, 0))
-                {
-                  if (0 == sem_init(&(b->semBarrierBreeched[1]), pshared, 0))
-                    {
-                      *barrier = b;
-                      return 0;
-                    }
-                  (void) sem_destroy(&(b->semBarrierBreeched[0]));
-                }
-              (void) pthread_mutex_destroy(&(b->mtxExclusiveAccess));
+              *barrier = b;
+              return 0;
             }
-          pthread_mutexattr_destroy(&ma);
+          (void) sem_destroy(&(b->semBarrierBreeched[0]));
         }
       (void) free(b);
     }
@@ -97,34 +91,30 @@ pthread_barrier_destroy(pthread_barrier_t *barrier)
     }
 
   b = *barrier;
-  
-  result = pthread_mutex_trylock(&(b->mtxExclusiveAccess));
+  *barrier = NULL;
 
-  if (0 == result)
+  if (0 == (result = sem_destroy(&(b->semBarrierBreeched[0]))))
     {
-      /*
-       * FIXME!!!
-       * The mutex isn't held by another thread but we could still
-       * be too late invalidating the barrier below since another thread
-       * may alredy have entered barrier_wait and the check for a valid
-       * *barrier != NULL.
-       */
-      *barrier = NULL;
-      (void) pthread_mutex_unlock(&(b->mtxExclusiveAccess));
-
-      (void) sem_destroy(&(b->semBarrierBreeched[1]));
-      (void) sem_destroy(&(b->semBarrierBreeched[0]));
-      (void) pthread_mutex_destroy(&(b->mtxExclusiveAccess));
-      (void) free(b);
+      if (0 == (result = sem_destroy(&(b->semBarrierBreeched[1]))))
+        {
+          (void) free(b);
+          return 0;
+        }
+      (void) sem_init(&(b->semBarrierBreeched[0]),
+                        b->pshared,
+                        0);
     }
 
+  *barrier = b;
   return(result);
 }
 
+
 int
 pthread_barrier_wait(pthread_barrier_t *barrier)
 {
   int result;
+  int step;
   pthread_barrier_t b;
 
   if (barrier == NULL || *barrier == (pthread_barrier_t) PTW32_OBJECT_INVALID)
@@ -133,69 +123,61 @@ pthread_barrier_wait(pthread_barrier_t *barrier)
     }
 
   b = *barrier;
+  step = b->iStep;
 
-  result = pthread_mutex_lock(&(b->mtxExclusiveAccess));
-
-  if (0 == result)
+  if (0 == InterlockedDecrement((_LPLONG) &(b->nCurrentBarrierHeight)))
     {
-      int step = b->iStep;
-      unsigned int serial = b->nSerial;
+      /* Must be done before posting the semaphore. */
+      b->nCurrentBarrierHeight = b->nInitialBarrierHeight;
 
-      if (0 == --(b->nCurrentBarrierHeight))
+      /*
+       * There is no race condition between the semaphore wait and post
+       * because we are using two alternating semas and all threads have
+       * entered barrier_wait and checked nCurrentBarrierHeight before this
+       * barrier's sema can be posted. Any threads that have not quite
+       * entered sem_wait below when the multiple_post has completed
+       * will nevertheless continue through the sema (barrier)
+       * and will not be left stranded.
+       */
+      if (b->nInitialBarrierHeight > 1)
         {
-          (void) pthread_mutex_unlock(&(b->mtxExclusiveAccess));
-          /*
-           * All other threads in this barrier set have at least passed
-           * through the decrement and test above, so its safe to
-           * raise the barrier to its full height.
-           */
-          b->iStep = 1 - step;
-          b->nCurrentBarrierHeight = b->nInitialBarrierHeight;
-          (void) sem_post_multiple(&(b->semBarrierBreeched[step]), b->nInitialBarrierHeight - 1);
+          result = sem_post_multiple(&(b->semBarrierBreeched[step]),
+                                     b->nInitialBarrierHeight - 1);
         }
-      else
-        {
-          pthread_t self;
-          int oldCancelState;
-
-          (void) pthread_mutex_unlock(&(b->mtxExclusiveAccess));
+    }
+  else
+    {
+      BOOL switchCancelState;
+      int oldCancelState;
+      pthread_t self = pthread_self();
 
-          self = pthread_self();
- 
-          /*
-           * pthread_barrier_wait() is not a cancelation point
-           * so temporarily prevent pthreadCancelableWait() from being one.
-           */
-          if (self->cancelType == PTHREAD_CANCEL_DEFERRED)
-            {
-              pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldCancelState);
-            }
+      /*
+       * This routine is not a cancelation point, so temporarily
+       * prevent sem_wait() from being one.
+       * PTHREAD_CANCEL_ASYNCHRONOUS threads can still be canceled.
+       */
+      switchCancelState = (self->cancelType == PTHREAD_CANCEL_DEFERRED &&
+                           0 == pthread_setcancelstate(PTHREAD_CANCEL_DISABLE,
+                                                       &oldCancelState));
 
-          /*
-           * There is no race condition between the semaphore wait and post
-           * because we are using two alternating semas and all threads have
-           * entered barrier_wait and checked nCurrentBarrierHeight before this
-           * barrier's sema can be posted. Any threads that have not quite
-           * entered sem_wait below when the multiple_post above is called
-           * will nevertheless continue through the sema (and the barrier).
-           * We have fulfilled our synchronisation function.
-           */
-          result = sem_wait(&(b->semBarrierBreeched[step]));
+      result = sem_wait(&(b->semBarrierBreeched[step]));
 
-          if (self->cancelType == PTHREAD_CANCEL_DEFERRED)
-            {
-              pthread_setcancelstate(oldCancelState, NULL);
-            }
+      if (switchCancelState)
+        {
+          (void) pthread_setcancelstate(oldCancelState, NULL);
         }
+    }
 
-      /*
-       * The first thread to return from the wait will be the
-       * PTHREAD_BARRIER_SERIAL_THREAD.
-       */
-      result = ((_LONG) serial == InterlockedCompareExchange((_LPLONG) &(b->nSerial),
-                                                             (_LONG) ((serial + 1)
-                                                                      & 0x7FFFFFFF),
-                                                             (_LONG) serial)
+  /*
+   * The first thread across will be the PTHREAD_BARRIER_SERIAL_THREAD.
+   * It also sets up the alternate semaphore as the next barrier.
+   */
+  if (0 == result)
+    {
+      result = ((_LONG) step ==
+                InterlockedCompareExchange((_LPLONG) &(b->iStep),
+                                           (_LONG) (1L - step),
+                                           (_LONG) step)
                 ? PTHREAD_BARRIER_SERIAL_THREAD
                 : 0);
     }
diff --git a/implement.h b/implement.h
index bc72b3f..080d9a1 100644
--- a/implement.h
+++ b/implement.h
@@ -189,9 +189,8 @@ struct pthread_spinlock_t_ {
 struct pthread_barrier_t_ {
   unsigned int nCurrentBarrierHeight;
   unsigned int nInitialBarrierHeight;
-  unsigned int nSerial;
   int iStep;
-  pthread_mutex_t mtxExclusiveAccess;
+  int pshared;
   sem_t semBarrierBreeched[2];
 };
 
diff --git a/private.c b/private.c
index de560c3..a332385 100644
--- a/private.c
+++ b/private.c
@@ -369,15 +369,10 @@ ptw32_threadStart (void * vthreadParms)
         * That function may call pthread_exit() or be canceled, which will
         * be handled by the outer try block.
         * 
-        * ptw32_terminate() will be called if there is no user supplied function.
+        * ptw32_terminate() will be called if there is no user
+        * supplied function.
         */
 
-       //Original invocation:
-       //(void) terminate();
-
-
-       //New invocation:
-       //  a) get pointer to the termination function
 #if defined(_MSC_VER)
        terminate_function term_func = set_terminate(0);
 #else
@@ -386,14 +381,10 @@ ptw32_threadStart (void * vthreadParms)
 
        set_terminate(term_func);
 
-       //  b) call the termination function (if any)
        if (term_func != 0) {
            term_func();
        }
 
-       //  c) if there was no termination function or the termination function did
-       //     not exit thread/process, (we got this far), propagate the exception on!
-       //     (should be caught by the second level try/catch block below)
        throw;
      }
   }
diff --git a/spin.c b/spin.c
index 26e278c..458bb7a 100644
--- a/spin.c
+++ b/spin.c
@@ -2,7 +2,7 @@
  * spin.c
  *
  * Description:
- * This translation unit implements spin locks primitives.
+ * This translation unit implements spin lock primitives.
  *
  * Pthreads-win32 - POSIX Threads Library for Win32
  * Copyright (C) 1998