another cond var fix: requeue count race condition

lock out new waiters during the broadcast. otherwise the wait count
added to the mutex might be lower than the actual number of waiters
moved, and wakeups may be lost.

this issue could also be solved by temporarily setting the mutex
waiter count higher than any possible real count, then relying on the
kernel to tell us how many waiters were requeued, and updating the
counts afterwards. however the logic is more complex, and i don't
really trust the kernel. the solution here is also nice in that it
replaces some atomic cas loops with simple non-atomic ops under lock.

diff --git a/src/thread/pthread_cond_broadcast.c b/src/thread/pthread_cond_broadcast.c
index 9c6a462b781bd5bb6435df47f6890c02c41b2f4a..848e288f5ea6dc25de38b901210bbace6f2bc2de 100644 (file)
--- a/src/thread/pthread_cond_broadcast.c
+++ b/src/thread/pthread_cond_broadcast.c
@@ -22,12 +22,8 @@ int pthread_cond_broadcast(pthread_cond_t *c)
        m = c->_c_mutex;
 
        /* Move waiter count to the mutex */
-       for (;;) {
-               int w = c->_c_waiters2;
-               a_fetch_add(&m->_m_waiters, w);
-               if (a_cas(&c->_c_waiters2, w, 0) == w) break;
-               a_fetch_add(&m->_m_waiters, -w);
-       }
+       a_fetch_add(&m->_m_waiters, c->_c_waiters2);
+       c->_c_waiters2 = 0;
 
        /* Perform the futex requeue, waking one waiter unless we know
         * that the calling thread holds the mutex. */

diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c
index e3dc8147c8b3ae89569a6f4d4a2751147ed5d9e4..db2744bad7b7c36c4f7cf7b0ac47a8f49222316b 100644 (file)
--- a/src/thread/pthread_cond_timedwait.c
+++ b/src/thread/pthread_cond_timedwait.c
@@ -7,8 +7,6 @@ struct cm {
 
 static void unwait(pthread_cond_t *c, pthread_mutex_t *m)
 {
-       int w;
-
        /* Removing a waiter is non-trivial if we could be using requeue
         * based broadcast signals, due to mutex access issues, etc. */
 
@@ -20,10 +18,8 @@ static void unwait(pthread_cond_t *c, pthread_mutex_t *m)
        while (a_swap(&c->_c_lock, 1))
                __wait(&c->_c_lock, &c->_c_lockwait, 1, 1);
 
-       /* Atomically decrement waiters2 if positive, else mutex waiters. */
-       do w = c->_c_waiters2;
-       while (w && a_cas(&c->_c_waiters2, w, w-1)!=w);
-       if (!w) a_dec(&m->_m_waiters);
+       if (c->_c_waiters2) c->_c_waiters2--;
+       else a_dec(&m->_m_waiters);
 
        a_store(&c->_c_lock, 0);
        if (c->_c_lockwait) __wake(&c->_c_lock, 1, 1);
@@ -46,10 +42,16 @@ int pthread_cond_timedwait(pthread_cond_t *c, pthread_mutex_t *m, const struct t
 
        pthread_testcancel();
 
-       if (c->_c_mutex != (void *)-1) c->_c_mutex = m;
-
        a_inc(&c->_c_waiters);
-       a_inc(&c->_c_waiters2);
+
+       if (c->_c_mutex != (void *)-1) {
+               c->_c_mutex = m;
+               while (a_swap(&c->_c_lock, 1))
+                       __wait(&c->_c_lock, &c->_c_lockwait, 1, 1);
+               c->_c_waiters2++;
+               a_store(&c->_c_lock, 0);
+               if (c->_c_lockwait) __wake(&c->_c_lock, 1, 1);
+       }
 
        seq = c->_c_seq;