redesign cond var implementation to fix multiple issues
authorRich Felker <dalias@aerifal.cx>
Mon, 18 Aug 2014 02:09:47 +0000 (22:09 -0400)
committerRich Felker <dalias@aerifal.cx>
Mon, 18 Aug 2014 02:09:47 +0000 (22:09 -0400)
the immediate issue that was reported by Jens Gustedt and needed to be
fixed was corruption of the cv/mutex waiter states when switching to
using a new mutex with the cv after all waiters were unblocked but
before they finished returning from the wait function.

self-synchronized destruction was also handled poorly and may have had
race conditions. and the use of sequence numbers for waking waiters
admitted a theoretical missed-wakeup if the sequence number wrapped
through the full 32-bit space.

the new implementation is largely documented in the comments in the
source. the basic principle is to use linked lists initially attached
to the cv object, but detachable on signal/broadcast, made up of nodes
residing in automatic storage (stack) on the threads that are waiting.
this eliminates the need for waiters to access the cv object after
they are signaled, and allows us to limit wakeup to one waiter at a
time during broadcasts even when futex requeue cannot be used.

performance is also greatly improved, roughly double some tests.

basically nothing is changed in the process-shared cond var case,
where this implementation does not work, since processes do not have
access to one another's local storage.

src/internal/pthread_impl.h
src/thread/pthread_cond_broadcast.c
src/thread/pthread_cond_destroy.c
src/thread/pthread_cond_init.c
src/thread/pthread_cond_signal.c
src/thread/pthread_cond_timedwait.c

index 848ff668cd6656043fc535320435273cb8be830c..2d090f8f6e2199288d3d2382cb3088c9b464353f 100644 (file)
@@ -66,14 +66,13 @@ struct __timer {
 #define _m_prev __u.__p[3]
 #define _m_next __u.__p[4]
 #define _m_count __u.__i[5]
-#define _c_mutex __u.__p[0]
+#define _c_shared __u.__p[0]
 #define _c_seq __u.__i[2]
 #define _c_waiters __u.__i[3]
 #define _c_clock __u.__i[4]
-#define _c_lock __u.__i[5]
-#define _c_lockwait __u.__i[6]
-#define _c_waiters2 __u.__i[7]
-#define _c_destroy __u.__i[8]
+#define _c_lock __u.__i[8]
+#define _c_head __u.__p[1]
+#define _c_tail __u.__p[5]
 #define _rw_lock __u.__i[0]
 #define _rw_waiters __u.__i[1]
 #define _rw_shared __u.__i[2]
index 18e778f39b624f62fe4bcf8cea8f770d692b292c..69f840fb4faee7d1f402771dba6f02b157a935c5 100644 (file)
@@ -1,43 +1,12 @@
 #include "pthread_impl.h"
 
+int __private_cond_signal(pthread_cond_t *, int);
+
 int pthread_cond_broadcast(pthread_cond_t *c)
 {
-       pthread_mutex_t *m;
-
+       if (!c->_c_shared) return __private_cond_signal(c, -1);
        if (!c->_c_waiters) return 0;
-
        a_inc(&c->_c_seq);
-
-       /* If cond var is process-shared, simply wake all waiters. */
-       if (c->_c_mutex == (void *)-1) {
-               __wake(&c->_c_seq, -1, 0);
-               return 0;
-       }
-
-       /* Block waiters from returning so we can use the mutex. */
-       while (a_swap(&c->_c_lock, 1))
-               __wait(&c->_c_lock, &c->_c_lockwait, 1, 1);
-       if (!c->_c_waiters)
-               goto out;
-       m = c->_c_mutex;
-
-       /* Move waiter count to the mutex */
-       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. */
-       int wake_cnt = !(m->_m_type & 3)
-               || (m->_m_lock&INT_MAX)!=__pthread_self()->tid;
-       if (m->_m_type & 128) wake_cnt = INT_MAX;
-       __syscall(SYS_futex, &c->_c_seq, FUTEX_REQUEUE | 128,
-               wake_cnt, INT_MAX, &m->_m_lock) != -EINVAL ||
-       __syscall(SYS_futex, &c->_c_seq, FUTEX_REQUEUE,
-               wake_cnt, INT_MAX, &m->_m_lock);
-
-out:
-       a_store(&c->_c_lock, 0);
-       if (c->_c_lockwait) __wake(&c->_c_lock, 1, 1);
-
+       __wake(&c->_c_seq, -1, 0);
        return 0;
 }
index a096c5547fdb0297735922568aadbf72c52fa36f..8c5551600e8c35d0a9f5fe194ee060d8303c22f6 100644 (file)
@@ -2,12 +2,13 @@
 
 int pthread_cond_destroy(pthread_cond_t *c)
 {
-       int priv = c->_c_mutex != (void *)-1;
-       int cnt;
-       c->_c_destroy = 1;
-       if (c->_c_waiters)
-               __wake(&c->_c_seq, -1, priv);
-       while ((cnt = c->_c_waiters))
-               __wait(&c->_c_waiters, 0, cnt, priv);
+       if (c->_c_shared && c->_c_waiters) {
+               int cnt;
+               a_or(&c->_c_waiters, 0x80000000);
+               a_inc(&c->_c_seq);
+               __wake(&c->_c_seq, -1, 0);
+               while ((cnt = c->_c_waiters) & 0x7fffffff)
+                       __wait(&c->_c_waiters, 0, cnt, 0);
+       }
        return 0;
 }
index 357ecd55ea8787b10f73e321f6e7c43334afffcd..8c484ddcded2a10acb6bc9ce4e8dcaf43eb24e39 100644 (file)
@@ -5,7 +5,7 @@ int pthread_cond_init(pthread_cond_t *restrict c, const pthread_condattr_t *rest
        *c = (pthread_cond_t){0};
        if (a) {
                c->_c_clock = a->__attr & 0x7fffffff;
-               if (a->__attr>>31) c->_c_mutex = (void *)-1;
+               if (a->__attr>>31) c->_c_shared = (void *)-1;
        }
        return 0;
 }
index 5fd72f903fdf27f70f3580b462d215fa80519028..119c00abcd9904b46fa7eabbc6093dfb736df276 100644 (file)
@@ -1,9 +1,12 @@
 #include "pthread_impl.h"
 
+int __private_cond_signal(pthread_cond_t *, int);
+
 int pthread_cond_signal(pthread_cond_t *c)
 {
+       if (!c->_c_shared) return __private_cond_signal(c, 1);
        if (!c->_c_waiters) return 0;
        a_inc(&c->_c_seq);
-       if (c->_c_waiters) __wake(&c->_c_seq, 1, c->_c_mutex!=(void*)-1);
+       __wake(&c->_c_seq, 1, 0);
        return 0;
 }
index 44e89567ecbb3e4df147da27ad0c2b0ee98ff938..7aaba95475957719fa0119eb9247bc02ed32da66 100644 (file)
 #include "pthread_impl.h"
 
-struct cm {
-       pthread_cond_t *c;
-       pthread_mutex_t *m;
+/*
+ * struct waiter
+ *
+ * Waiter objects have automatic storage on the waiting thread, and
+ * are used in building a linked list representing waiters currently
+ * waiting on the condition variable or a group of waiters woken
+ * together by a broadcast or signal; in the case of signal, this is a
+ * degenerate list of one member.
+ *
+ * Waiter lists attached to the condition variable itself are
+ * protected by the lock on the cv. Detached waiter lists are
+ * protected by the associated mutex. The hand-off between protections
+ * is handled by a "barrier" lock in each node, which disallows
+ * signaled waiters from making forward progress to the code that will
+ * access the list using the mutex until the list is in a consistent
+ * state and the cv lock as been released.
+ *
+ * Since process-shared cond var semantics do not necessarily allow
+ * one thread to see another's automatic storage (they may be in
+ * different processes), the waiter list is not used for the
+ * process-shared case, but the structure is still used to store data
+ * needed by the cancellation cleanup handler.
+ */
+
+struct waiter {
+       struct waiter *prev, *next;
+       int state, barrier, requeued, mutex_ret;
+       int *notify;
+       pthread_mutex_t *mutex;
+       pthread_cond_t *cond;
+       int shared;
 };
 
-static void unwait(pthread_cond_t *c, pthread_mutex_t *m)
-{
-       /* Removing a waiter is non-trivial if we could be using requeue
-        * based broadcast signals, due to mutex access issues, etc. */
+/* Self-synchronized-destruction-safe lock functions */
 
-       if (c->_c_mutex == (void *)-1) {
-               a_dec(&c->_c_waiters);
-               if (c->_c_destroy) __wake(&c->_c_waiters, 1, 0);
-               return;
+static inline void lock(volatile int *l)
+{
+       if (a_cas(l, 0, 1)) {
+               a_cas(l, 1, 2);
+               do __wait(l, 0, 2, 1);
+               while (a_cas(l, 0, 2));
        }
+}
 
-       while (a_swap(&c->_c_lock, 1))
-               __wait(&c->_c_lock, &c->_c_lockwait, 1, 1);
+static inline void unlock(volatile int *l)
+{
+       if (a_swap(l, 0)==2)
+               __wake(l, 1, 1);
+}
 
-       if (c->_c_waiters2) c->_c_waiters2--;
-       else a_dec(&m->_m_waiters);
+enum {
+       WAITING,
+       SIGNALED,
+       LEAVING,
+};
 
-       a_store(&c->_c_lock, 0);
-       if (c->_c_lockwait) __wake(&c->_c_lock, 1, 1);
+static void unwait(void *arg)
+{
+       struct waiter *node = arg, *p;
+
+       if (node->shared) {
+               pthread_cond_t *c = node->cond;
+               pthread_mutex_t *m = node->mutex;
+               if (a_fetch_add(&c->_c_waiters, -1) == -0x7fffffff)
+                       __wake(&c->_c_waiters, 1, 0);
+               node->mutex_ret = pthread_mutex_lock(m);
+               return;
+       }
 
-       a_dec(&c->_c_waiters);
-       if (c->_c_destroy) __wake(&c->_c_waiters, 1, 1);
-}
+       int oldstate = a_cas(&node->state, WAITING, LEAVING);
+
+       if (oldstate == WAITING) {
+               /* Access to cv object is valid because this waiter was not
+                * yet signaled and a new signal/broadcast cannot return
+                * after seeing a LEAVING waiter without getting notified
+                * via the futex notify below. */
+
+               pthread_cond_t *c = node->cond;
+               lock(&c->_c_lock);
+               
+               if (c->_c_head == node) c->_c_head = node->next;
+               else if (node->prev) node->prev->next = node->next;
+               if (c->_c_tail == node) c->_c_tail = node->prev;
+               else if (node->next) node->next->prev = node->prev;
+               
+               unlock(&c->_c_lock);
+
+               if (node->notify) {
+                       if (a_fetch_add(node->notify, -1)==1)
+                               __wake(node->notify, 1, 1);
+               }
+       }
 
-static void cleanup(void *p)
-{
-       struct cm *cm = p;
-       unwait(cm->c, cm->m);
-       pthread_mutex_lock(cm->m);
+       node->mutex_ret = pthread_mutex_lock(node->mutex);
+
+       if (oldstate == WAITING) return;
+
+       /* If the mutex can't be locked, we're in big trouble because
+        * it's all that protects access to the shared list state.
+        * In order to prevent catastrophic stack corruption from
+        * unsynchronized access, simply deadlock. */
+       if (node->mutex_ret && node->mutex_ret != EOWNERDEAD)
+               for (;;) lock(&(int){0});
+
+       /* Wait until control of the list has been handed over from
+        * the cv lock (signaling thread) to the mutex (waiters). */
+       lock(&node->barrier);
+
+       /* If this thread was requeued to the mutex, undo the extra
+        * waiter count that was added to the mutex. */
+       if (node->requeued) a_dec(&node->mutex->_m_waiters);
+
+       /* Find a thread to requeue to the mutex, starting from the
+        * end of the list (oldest waiters). */
+       for (p=node; p->next; p=p->next);
+       if (p==node) p=node->prev;
+       for (; p && p->requeued; p=p->prev);
+       if (p==node) p=node->prev;
+       if (p) {
+               p->requeued = 1;
+               a_inc(&node->mutex->_m_waiters);
+               /* The futex requeue command cannot requeue from
+                * private to shared, so for process-shared mutexes,
+                * simply wake the target. */
+               int wake = node->mutex->_m_type & 128;
+               __syscall(SYS_futex, &p->state, FUTEX_REQUEUE|128,
+                       wake, 1, &node->mutex->_m_lock) != -EINVAL
+               || __syscall(SYS_futex, &p->state, FUTEX_REQUEUE,
+                       0, 1, &node->mutex->_m_lock);
+       }
+
+       /* Remove this thread from the list. */
+       if (node->next) node->next->prev = node->prev;
+       if (node->prev) node->prev->next = node->next;
 }
 
 int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts)
 {
-       struct cm cm = { .c=c, .m=m };
-       int r, e=0, seq;
+       struct waiter node = { .cond = c, .mutex = m };
+       int e, seq, *fut, clock = c->_c_clock;
 
        if ((m->_m_type&15) && (m->_m_lock&INT_MAX) != __pthread_self()->tid)
                return EPERM;
@@ -49,29 +149,77 @@ int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict
 
        pthread_testcancel();
 
-       a_inc(&c->_c_waiters);
-
-       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);
+       if (c->_c_shared) {
+               node.shared = 1;
+               fut = &c->_c_seq;
+               seq = c->_c_seq;
+               a_inc(&c->_c_waiters);
+       } else {
+               lock(&c->_c_lock);
+
+               node.barrier = 1;
+               fut = &node.state;
+               seq = node.state = WAITING;
+               node.next = c->_c_head;
+               c->_c_head = &node;
+               if (!c->_c_tail) c->_c_tail = &node;
+               else node.next->prev = &node;
+
+               unlock(&c->_c_lock);
        }
 
-       seq = c->_c_seq;
-
        pthread_mutex_unlock(m);
 
-       do e = __timedwait(&c->_c_seq, seq, c->_c_clock, ts, cleanup, &cm,
-               c->_c_mutex != (void *)-1);
-       while (c->_c_seq == seq && (!e || e==EINTR));
+       do e = __timedwait(fut, seq, clock, ts, unwait, &node, !node.shared);
+       while (*fut==seq && (!e || e==EINTR));
        if (e == EINTR) e = 0;
 
-       unwait(c, m);
+       unwait(&node);
 
-       if ((r=pthread_mutex_lock(m))) return r;
+       return node.mutex_ret ? node.mutex_ret : e;
+}
 
-       return e;
+int __private_cond_signal(pthread_cond_t *c, int n)
+{
+       struct waiter *p, *q=0;
+       int ref = 0, cur;
+
+       lock(&c->_c_lock);
+       for (p=c->_c_tail; n && p; p=p->prev) {
+               /* The per-waiter-node barrier lock is held at this
+                * point, so while the following CAS may allow forward
+                * progress in the target thread, it doesn't allow
+                * access to the waiter list yet. Ideally the target
+                * does not run until the futex wake anyway. */
+               if (a_cas(&p->state, WAITING, SIGNALED) != WAITING) {
+                       ref++;
+                       p->notify = &ref;
+               } else {
+                       n--;
+                       if (!q) q=p;
+               }
+       }
+       /* Split the list, leaving any remainder on the cv. */
+       if (p) {
+               if (p->next) p->next->prev = 0;
+               p->next = 0;
+       } else {
+               c->_c_head = 0;
+       }
+       c->_c_tail = p;
+       unlock(&c->_c_lock);
+
+       /* Wait for any waiters in the LEAVING state to remove
+        * themselves from the list before returning or allowing
+        * signaled threads to proceed. */
+       while ((cur = ref)) __wait(&ref, 0, cur, 1);
+
+       /* Wake the first signaled thread and unlock the per-waiter
+        * barriers preventing their forward progress. */
+       for (p=q; p; p=q) {
+               q = p->prev;
+               if (!p->next) __wake(&p->state, 1, 1);
+               unlock(&p->barrier);
+       }
+       return 0;
 }