Fix some issues in libthread_xu's condvar implementation.
authorMatthew Dillon <dillon@dragonflybsd.org>
Mon, 14 Apr 2008 20:12:41 +0000 (20:12 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Mon, 14 Apr 2008 20:12:41 +0000 (20:12 +0000)
* Non-broadcast mode is not guaranteed to signal just one waiter, loosen
  it up a bit to close race conditions and signal more if necessary.

* Clean up the condition structure.  Do not try to track non-broadcast
  wakeups.  Do not try to block waiting for individual wakeups... the
  spec does not require it and, in fact, doing so can create more
  problems then it solves.

* Load oldseq from cv->c_seqno *BEFORE* releasing the passed mutex to
  close a race.  The mutex is there precisely so that userland can
  guarantee that no race will occur between waiter and signaler.

Reported-by: Jordan Gordeev <jgordeev@dir.bg>,
             "Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>

lib/libthread_xu/thread/thr_cond.c
lib/libthread_xu/thread/thr_private.h
lib/libthread_xu/thread/thr_umtx.c
lib/libthread_xu/thread/thr_umtx.h

index e98d65d..914351f 100644 (file)
@@ -23,7 +23,7 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
- * $DragonFly: src/lib/libthread_xu/thread/thr_cond.c,v 1.10 2006/04/06 23:50:13 davidxu Exp $
+ * $DragonFly: src/lib/libthread_xu/thread/thr_cond.c,v 1.11 2008/04/14 20:12:41 dillon Exp $
  */
 
 #include "namespace.h"
@@ -66,7 +66,7 @@ cond_init(pthread_cond_t *cond, const pthread_condattr_t *cond_attr)
                _thr_umtx_init(&pcond->c_lock);
                pcond->c_seqno = 0;
                pcond->c_waiters = 0;
-               pcond->c_wakeups = 0;
+               pcond->c_broadcast = 0;
                if (cond_attr == NULL || *cond_attr == NULL) {
                        pcond->c_pshared = 0;
                        pcond->c_clockid = CLOCK_REALTIME;
@@ -116,7 +116,7 @@ _pthread_cond_destroy(pthread_cond_t *cond)
        else {
                /* Lock the condition variable structure: */
                THR_LOCK_ACQUIRE(curthread, &(*cond)->c_lock);
-               if ((*cond)->c_waiters + (*cond)->c_wakeups != 0) {
+               if ((*cond)->c_waiters != 0) {
                        THR_LOCK_RELEASE(curthread, &(*cond)->c_lock);
                        return (EBUSY);
                }
@@ -161,14 +161,12 @@ cond_cancel_handler(void *arg)
 
        cv = *(info->cond);
        THR_LOCK_ACQUIRE(curthread, &cv->c_lock);
-       if (cv->c_seqno != info->seqno && cv->c_wakeups != 0) {
-               if (cv->c_waiters > 0) {
-                       cv->c_seqno++;
-                       _thr_umtx_wake(&cv->c_seqno, 1);
-               } else
-                       cv->c_wakeups--;
-       } else {
-               cv->c_waiters--;
+       if (--cv->c_waiters == 0)
+               cv->c_broadcast = 0;
+       if (cv->c_seqno != info->seqno) {
+               _thr_umtx_wake(&cv->c_seqno, 1);
+               /* cv->c_seqno++; XXX why was this here? */
+               _thr_umtx_wake(&cv->c_seqno, 1);
        }
        THR_LOCK_RELEASE(curthread, &cv->c_lock);
 
@@ -196,19 +194,28 @@ cond_wait_common(pthread_cond_t *cond, pthread_mutex_t *mutex,
                return (ret);
 
        cv = *cond;
+/*     fprintf(stderr, "waiton1 %p\n", cv);*/
        THR_LOCK_ACQUIRE(curthread, &cv->c_lock);
+       oldseq = cv->c_seqno;
+/*     fprintf(stderr, "waiton2 %p %d\n", cv, oldseq);*/
        ret = _mutex_cv_unlock(mutex, &info.count);
        if (ret) {
+               assert(0);
                THR_LOCK_RELEASE(curthread, &cv->c_lock);
                return (ret);
        }
-       oldseq = seq = cv->c_seqno;
+       seq = cv->c_seqno;
        info.mutex = mutex;
        info.cond  = cond;
        info.seqno = oldseq;
 
-       cv->c_waiters++;
-       do {
+       ++cv->c_waiters;
+
+       /*
+        * loop if we have never been told to wake up
+        * or we lost a race.
+        */
+       while (seq == oldseq /* || cv->c_wakeups == 0*/) {
                THR_LOCK_RELEASE(curthread, &cv->c_lock);
 
                if (abstime != NULL) {
@@ -234,19 +241,11 @@ cond_wait_common(pthread_cond_t *cond, pthread_mutex_t *mutex,
                seq = cv->c_seqno;
                if (abstime != NULL && ret == ETIMEDOUT)
                        break;
-
-               /*
-                * loop if we have never been told to wake up
-                * or we lost a race.
-                */
-       } while (seq == oldseq || cv->c_wakeups == 0);
-       
-       if (seq != oldseq && cv->c_wakeups != 0) {
-               cv->c_wakeups--;
-               ret = 0;
-       } else {
-               cv->c_waiters--;
        }
+       if (--cv->c_waiters == 0)
+               cv->c_broadcast = 0;
+       if (seq != oldseq)
+               ret = 0;
        THR_LOCK_RELEASE(curthread, &cv->c_lock);
        _mutex_cv_lock(mutex, info.count);
        return (ret);
@@ -304,20 +303,18 @@ cond_signal_common(pthread_cond_t *cond, int broadcast)
                return (ret);
 
        cv = *cond;
+/*     fprintf(stderr, "signal %p\n", cv);*/
        /* Lock the condition variable structure. */
        THR_LOCK_ACQUIRE(curthread, &cv->c_lock);
+       cv->c_seqno++;
+       if (cv->c_broadcast == 0)
+               cv->c_broadcast = broadcast;
+
        if (cv->c_waiters) {
-               if (!broadcast) {
-                       cv->c_wakeups++;
-                       cv->c_waiters--;
-                       cv->c_seqno++;
-                       _thr_umtx_wake(&cv->c_seqno, 1);
-               } else {
-                       cv->c_wakeups += cv->c_waiters;
-                       cv->c_waiters = 0;
-                       cv->c_seqno++;
+               if (cv->c_broadcast)
                        _thr_umtx_wake(&cv->c_seqno, INT_MAX);
-               }
+               else
+                       _thr_umtx_wake(&cv->c_seqno, 1);
        }
        THR_LOCK_RELEASE(curthread, &cv->c_lock);
        return (ret);
index f2362ec..8f59513 100644 (file)
@@ -32,7 +32,7 @@
  * Private thread definitions for the uthread kernel.
  *
  * $FreeBSD: src/lib/libpthread/thread/thr_private.h,v 1.120 2004/11/01 10:49:34 davidxu Exp $
- * $DragonFly: src/lib/libthread_xu/thread/thr_private.h,v 1.17 2007/06/26 23:30:05 josepht Exp $
+ * $DragonFly: src/lib/libthread_xu/thread/thr_private.h,v 1.18 2008/04/14 20:12:41 dillon Exp $
  */
 
 #ifndef _THR_PRIVATE_H
@@ -170,7 +170,7 @@ struct pthread_cond {
        volatile umtx_t c_lock;
        volatile umtx_t c_seqno;
        volatile int    c_waiters;
-       volatile int    c_wakeups;
+       volatile int    c_broadcast;
        int             c_pshared;
        int             c_clockid;
 };
index 1c27ef0..4dd61a3 100644 (file)
@@ -25,7 +25,7 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- * $DragonFly: src/lib/libthread_xu/thread/thr_umtx.c,v 1.3 2006/04/04 14:04:39 davidxu Exp $
+ * $DragonFly: src/lib/libthread_xu/thread/thr_umtx.c,v 1.4 2008/04/14 20:12:41 dillon Exp $
  */
 
 #include <assert.h>
@@ -119,7 +119,7 @@ __thr_umtx_timedlock(volatile umtx_t *mtx, const struct timespec *timeout)
 
 int
 _thr_umtx_wait(volatile umtx_t *mtx, int exp, const struct timespec *timeout,
-       int clockid)
+              int clockid)
 {
     struct timespec ts, ts2, ts3;
     int timo, ret = 0;
@@ -128,9 +128,20 @@ _thr_umtx_wait(volatile umtx_t *mtx, int exp, const struct timespec *timeout,
        return (0);
 
     if (timeout == NULL) {
-       if (umtx_sleep(mtx, exp, 0) < 0) {
-           if (errno == EINTR)
+       while (umtx_sleep(mtx, exp, 10000000) < 0) {
+           if (errno == EBUSY) 
+               break;
+           if (errno == EINTR) {
                ret = EINTR;
+               break;
+           }
+           if (errno == ETIMEDOUT || errno == EWOULDBLOCK) {
+               if (*mtx != exp) {
+                   fprintf(stderr, "thr_umtx_wait: FAULT VALUE CHANGE %d -> %d oncond %p\n", exp, *mtx, mtx);
+               }
+           }
+           if (*mtx != exp)
+               return(0);
        }
        return (ret);
     }
index db49466..d91828d 100644 (file)
@@ -23,7 +23,7 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- * $DragonFly: src/lib/libthread_xu/thread/thr_umtx.h,v 1.4 2006/04/06 13:03:09 davidxu Exp $
+ * $DragonFly: src/lib/libthread_xu/thread/thr_umtx.h,v 1.5 2008/04/14 20:12:41 dillon Exp $
  */
 
 #ifndef _THR_DFLY_UMTX_H_
@@ -81,7 +81,7 @@ _thr_umtx_unlock(volatile umtx_t *mtx, int id __unused)
 }
 
 int _thr_umtx_wait(volatile umtx_t *mtx, umtx_t exp,
-    const struct timespec *timeout, int clockid);
+                  const struct timespec *timeout, int clockid);
 void _thr_umtx_wake(volatile umtx_t *mtx, int count);
 
 #endif