Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/lib/libpthread - Make pthread_condvar and pthread_mutex work...



details:   https://anonhg.NetBSD.org/src/rev/450cbfc891bb
branches:  trunk
changeset: 934384:450cbfc891bb
user:      ad <ad%NetBSD.org@localhost>
date:      Wed Jun 10 22:45:15 2020 +0000

description:
- Make pthread_condvar and pthread_mutex work on the stack rather than in
  pthread_t, so there's less chance of bad things happening if someone calls
  (for example) pthread_cond_broadcast() from a signal handler.

- Remove all the deferred waiter handling except for the one case that really
  matters which is transferring waiters from condvar -> mutex on wakeup, and
  do that by splicing the condvar's waiters onto the mutex.

- Remove the mutex waiters bit as it's another complication that's not
  strictly needed.

diffstat:

 lib/libpthread/pthread.c       |   87 +++----------------
 lib/libpthread/pthread_cond.c  |  120 ++++++++++++----------------
 lib/libpthread/pthread_int.h   |   21 +---
 lib/libpthread/pthread_mutex.c |  172 ++++++++++++++++++++++------------------
 lib/libpthread/pthread_types.h |    6 +-
 5 files changed, 174 insertions(+), 232 deletions(-)

diffs (truncated from 802 to 300 lines):

diff -r a6e1ec82a2cc -r 450cbfc891bb lib/libpthread/pthread.c
--- a/lib/libpthread/pthread.c  Wed Jun 10 22:24:22 2020 +0000
+++ b/lib/libpthread/pthread.c  Wed Jun 10 22:45:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: pthread.c,v 1.174 2020/06/04 00:45:32 joerg Exp $      */
+/*     $NetBSD: pthread.c,v 1.175 2020/06/10 22:45:15 ad Exp $ */
 
 /*-
  * Copyright (c) 2001, 2002, 2003, 2006, 2007, 2008, 2020
@@ -31,7 +31,7 @@
  */
 
 #include <sys/cdefs.h>
-__RCSID("$NetBSD: pthread.c,v 1.174 2020/06/04 00:45:32 joerg Exp $");
+__RCSID("$NetBSD: pthread.c,v 1.175 2020/06/10 22:45:15 ad Exp $");
 
 #define        __EXPOSE_STACK  1
 
@@ -297,11 +297,7 @@
 
        t->pt_self = t;
        t->pt_magic = PT_MAGIC;
-       t->pt_willpark = 0;
-       t->pt_waiters[0] = 0;
-       t->pt_nwaiters = 0;
        t->pt_sleepobj = NULL;
-       t->pt_signalled = 0;
        t->pt_havespecific = 0;
        t->pt_lwpctl = &pthread__dummy_lwpctl;
 
@@ -602,48 +598,6 @@
        return errno;
 }
 
-/*
- * Wake all deferred waiters hanging off self.
- *
- * It's possible for threads to have called _lwp_exit() before we wake them,
- * because of cancellation and timeout, so ESRCH is tolerated here.  If a
- * thread exits and its LID is reused, and the a thread receives an wakeup
- * meant for the previous incarnation of the LID, no harm will be done.
- */
-void
-pthread__clear_waiters(pthread_t self)
-{
-       int rv;
-
-       pthread__smt_wake();
-
-       switch (self->pt_nwaiters) {
-       case 0:
-               break;
-       case 1:
-               if (self->pt_willpark) {
-                       break;
-               }
-               rv = _lwp_unpark(self->pt_waiters[0], NULL);
-               self->pt_waiters[0] = 0;
-               self->pt_nwaiters = 0;
-               if (rv != 0 && errno != ESRCH) {
-                       pthread__errorfunc(__FILE__, __LINE__, __func__,
-                           "_lwp_unpark failed: %d", errno);
-               }
-               break;
-       default:
-               rv = _lwp_unpark_all(self->pt_waiters, self->pt_nwaiters, NULL);
-               self->pt_waiters[0] = 0;
-               self->pt_nwaiters = 0;
-               if (rv != 0 && errno != ESRCH) {
-                       pthread__errorfunc(__FILE__, __LINE__, __func__,
-                           "_lwp_unpark_all failed: %d", errno);
-               }
-               break;
-       }
-}
-
 void
 pthread_exit(void *retval)
 {
@@ -658,7 +612,6 @@
        self = pthread__self();
 
        /* Disable cancellability. */
-       self->pt_willpark = 0;
        pthread_mutex_lock(&self->pt_lock);
        self->pt_flags |= PT_FLAG_CS_DISABLED;
        self->pt_cancel = 0;
@@ -692,14 +645,10 @@
        if (self->pt_flags & PT_FLAG_DETACHED) {
                /* pthread__reap() will drop the lock. */
                pthread__reap(self);
-               pthread__assert(!self->pt_willpark);
-               pthread__clear_waiters(self);
                _lwp_exit();
        } else {
                self->pt_state = PT_STATE_ZOMBIE;
-               pthread__assert(!self->pt_willpark);
                pthread_mutex_unlock(&self->pt_lock);
-               pthread__clear_waiters(self);
                /* Note: name will be freed by the joiner. */
                _lwp_exit();
        }
@@ -1166,9 +1115,7 @@
 {
        int rv, error;
 
-       self->pt_willpark = 1;
        pthread_mutex_unlock(lock);
-       self->pt_willpark = 0;
 
        /*
         * Wait until we are awoken by a pending unpark operation,
@@ -1194,13 +1141,8 @@
                 * If we deferred unparking a thread, arrange to
                 * have _lwp_park() restart it before blocking.
                 */
-               pthread__assert(self->pt_nwaiters <= 1);
-               pthread__assert(self->pt_nwaiters != 0 ||
-                   self->pt_waiters[0] == 0);
                error = _lwp_park(CLOCK_REALTIME, TIMER_ABSTIME,
-                   __UNCONST(abstime), self->pt_waiters[0], NULL, NULL);
-               self->pt_waiters[0] = 0;
-               self->pt_nwaiters = 0;
+                   __UNCONST(abstime), 0, NULL, NULL);
                if (error != 0) {
                        switch (rv = errno) {
                        case EINTR:
@@ -1230,31 +1172,34 @@
        pthread_t target;
 
        target = PTQ_FIRST(queue);
-       if (self->pt_nwaiters == pthread__unpark_max) {
-               pthread__clear_waiters(self);
-       }
        target->pt_sleepobj = NULL;
-       self->pt_waiters[self->pt_nwaiters++] = target->pt_lid;
        PTQ_REMOVE(queue, target, pt_sleep);
-       pthread__mutex_deferwake(self, interlock);
+       (void)_lwp_unpark(target->pt_lid, NULL);
 }
 
 void
 pthread__unpark_all(pthread_queue_t *queue, pthread_t self,
                    pthread_mutex_t *interlock)
 {
-       const size_t max = pthread__unpark_max;
+       lwpid_t lids[PTHREAD__UNPARK_MAX];
+       const size_t mlid = pthread__unpark_max;
        pthread_t target;
+       size_t nlid = 0;
 
        PTQ_FOREACH(target, queue, pt_sleep) {
-               if (self->pt_nwaiters == max) {
-                       pthread__clear_waiters(self);
+               if (nlid == mlid) {
+                       (void)_lwp_unpark_all(lids, nlid, NULL);
+                       nlid = 0;
                }
                target->pt_sleepobj = NULL;
-               self->pt_waiters[self->pt_nwaiters++] = target->pt_lid;
+               lids[nlid++] = target->pt_lid;
        }
        PTQ_INIT(queue);
-       pthread__mutex_deferwake(self, interlock);
+       if (nlid == 1) {
+               (void)_lwp_unpark(lids[0], NULL);
+       } else if (nlid > 1) {
+               (void)_lwp_unpark_all(lids, nlid, NULL);
+       }
 }
 
 #undef OOPS
diff -r a6e1ec82a2cc -r 450cbfc891bb lib/libpthread/pthread_cond.c
--- a/lib/libpthread/pthread_cond.c     Wed Jun 10 22:24:22 2020 +0000
+++ b/lib/libpthread/pthread_cond.c     Wed Jun 10 22:45:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: pthread_cond.c,v 1.73 2020/06/06 22:23:59 ad Exp $     */
+/*     $NetBSD: pthread_cond.c,v 1.74 2020/06/10 22:45:15 ad Exp $     */
 
 /*-
  * Copyright (c) 2001, 2006, 2007, 2008, 2020 The NetBSD Foundation, Inc.
@@ -30,7 +30,7 @@
  */
 
 #include <sys/cdefs.h>
-__RCSID("$NetBSD: pthread_cond.c,v 1.73 2020/06/06 22:23:59 ad Exp $");
+__RCSID("$NetBSD: pthread_cond.c,v 1.74 2020/06/10 22:45:15 ad Exp $");
 
 #include <stdlib.h>
 #include <errno.h>
@@ -111,8 +111,9 @@
 pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
                       const struct timespec *abstime)
 {
-       pthread_t self, next, waiters;
-       int retval, cancel;
+       struct pthread__waiter waiter, *next, *waiters;
+       pthread_t self;
+       int error, cancel;
        clockid_t clkid = pthread_cond_getclock(cond);
 
        if (__predict_false(__uselibcstub))
@@ -126,7 +127,7 @@
            mutex->ptm_owner != NULL);
 
        self = pthread__self();
-       pthread__assert(!self->pt_condwait);
+       pthread__assert(self->pt_lid != 0);
 
        if (__predict_false(self->pt_cancel)) {
                pthread__cancelled();
@@ -134,44 +135,37 @@
 
        /* Note this thread as waiting on the CV. */
        cond->ptc_mutex = mutex;
-       self->pt_condwait = 1;
        for (waiters = cond->ptc_waiters;; waiters = next) {
-               self->pt_condnext = waiters;
+               waiter.lid = self->pt_lid;
+               waiter.next = waiters;
 #ifndef PTHREAD__ATOMIC_IS_MEMBAR
                membar_producer();
 #endif
-               next = atomic_cas_ptr(&cond->ptc_waiters, waiters, self);
-               if (next == waiters)
+               next = atomic_cas_ptr(&cond->ptc_waiters, waiters, &waiter);
+               if (__predict_true(next == waiters)) {
                        break;
+               }
        }
 
        /* Drop the interlock */
-       self->pt_willpark = 1;
        pthread_mutex_unlock(mutex);
-       self->pt_willpark = 0;
+       error = 0;
 
-       do {
-               pthread__assert(self->pt_nwaiters <= 1);
-               pthread__assert(self->pt_nwaiters != 0 ||
-                   self->pt_waiters[0] == 0);
-               retval = _lwp_park(clkid, TIMER_ABSTIME, __UNCONST(abstime),
-                   self->pt_waiters[0], NULL, NULL);
-               self->pt_waiters[0] = 0;
-               self->pt_nwaiters = 0;
-               if (__predict_false(retval != 0)) {
-                       if (errno == EINTR || errno == EALREADY ||
-                           errno == ESRCH) {
-                               retval = 0;
-                       } else {
-                               retval = errno;
-                       }
+       while (waiter.lid && !(cancel = self->pt_cancel)) {
+               int rv = _lwp_park(clkid, TIMER_ABSTIME, __UNCONST(abstime),
+                   0, NULL, NULL);
+               if (rv == 0) {
+                       continue;
                }
-               cancel = self->pt_cancel;
-       } while (self->pt_condwait && !cancel && !retval);
+               if (errno != EINTR && errno != EALREADY && errno != ESRCH) {
+                       error = errno;
+                       break;
+               }
+       }
 
        /*
         * If this thread absorbed a wakeup from pthread_cond_signal() and
-        * cannot take the wakeup, we must ensure that another thread does.
+        * cannot take the wakeup, we should ensure that another thread does.
         *
         * And if awoken early, we may still be on the waiter list and must
         * remove self.
@@ -181,18 +175,20 @@
         * - wakeup could be deferred until mutex release
         * - it would be mixing up two sets of waitpoints
         */
-       if (__predict_false(self->pt_condwait | cancel | retval)) {
+       if (__predict_false(cancel | error)) {
                pthread_cond_broadcast(cond);
 
                /*
                 * Might have raced with another thread to do the wakeup. 
-                * Wait until released - this thread can't wait on a condvar
-                * again until the data structures are no longer in us.
+                * Wait until released, otherwise "waiter" is still globally
+                * visible.
                 */
-               while (self->pt_condwait) {
-                       (void)_lwp_park(CLOCK_REALTIME, TIMER_ABSTIME, NULL,
-                           0, NULL, NULL);
+               while (__predict_false(waiter.lid)) {
+                       (void)_lwp_park(CLOCK_MONOTONIC, 0, NULL, 0, NULL,
+                           NULL);
                }
+       } else {
+               pthread__assert(!waiter.lid);
        }
 
        /*



Home | Main Index | Thread Index | Old Index