Source-Changes-HG archive

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

[src/trunk]: src/lib/libpthread In the interests of reliability simplify wait...



details:   https://anonhg.NetBSD.org/src/rev/1d62a3c9f726
branches:  trunk
changeset: 972647:1d62a3c9f726
user:      ad <ad%NetBSD.org@localhost>
date:      Mon Jun 01 11:44:59 2020 +0000

description:
In the interests of reliability simplify waiter handling more and redo
condvars to manage the list of waiters with atomic ops.

diffstat:

 lib/libpthread/pthread.c        |  126 ++++++--------------
 lib/libpthread/pthread_cond.c   |  241 ++++++++++++++++-----------------------
 lib/libpthread/pthread_int.h    |    6 +-
 lib/libpthread/pthread_mutex.c  |   54 +++-----
 lib/libpthread/pthread_rwlock.c |   58 ++++-----
 lib/libpthread/pthread_types.h  |   10 +-
 6 files changed, 195 insertions(+), 300 deletions(-)

diffs (truncated from 846 to 300 lines):

diff -r e8c15e05353d -r 1d62a3c9f726 lib/libpthread/pthread.c
--- a/lib/libpthread/pthread.c  Mon Jun 01 11:08:57 2020 +0000
+++ b/lib/libpthread/pthread.c  Mon Jun 01 11:44:59 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: pthread.c,v 1.170 2020/05/16 22:53:37 ad Exp $ */
+/*     $NetBSD: pthread.c,v 1.171 2020/06/01 11:44:59 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.170 2020/05/16 22:53:37 ad Exp $");
+__RCSID("$NetBSD: pthread.c,v 1.171 2020/06/01 11:44:59 ad Exp $");
 
 #define        __EXPOSE_STACK  1
 
@@ -294,12 +294,11 @@
        t->pt_self = t;
        t->pt_magic = PT_MAGIC;
        t->pt_willpark = 0;
-       t->pt_unpark = 0;
+       t->pt_waiters[0] = 0;
        t->pt_nwaiters = 0;
        t->pt_sleepobj = NULL;
        t->pt_signalled = 0;
        t->pt_havespecific = 0;
-       t->pt_early = NULL;
        t->pt_lwpctl = &pthread__dummy_lwpctl;
 
        memcpy(&t->pt_lockops, pthread__lock_ops, sizeof(t->pt_lockops));
@@ -609,51 +608,32 @@
 {
        int rv;
 
-       /* Zero waiters or one waiter in error case (pthread_exit()). */
-       if (self->pt_nwaiters == 0) {
-               if (self->pt_unpark != 0 && self->pt_willpark == 0) {
-                       rv = (ssize_t)_lwp_unpark(self->pt_unpark, NULL);
-                       self->pt_unpark = 0;
-                       if (rv != 0 && errno != EALREADY && errno != EINTR &&
-                           errno != ESRCH) {
-                               pthread__errorfunc(__FILE__, __LINE__, __func__,
-                                   "_lwp_unpark failed");
-                       }
-               }
-               return;
-       }
+       pthread__smt_wake();
 
-       /* One waiter or two waiters (the second being a deferred wakeup). */
-       if (self->pt_nwaiters == 1) {
-               if (self->pt_unpark != 0) {
-                       /* Fall through to multiple waiters case. */
-                       self->pt_waiters[1] = self->pt_unpark;
-                       self->pt_nwaiters = 2;
-                       self->pt_unpark = 0;
-               } else if (self->pt_willpark) {
-                       /* Defer to _lwp_park(). */
-                       self->pt_unpark = self->pt_waiters[0];
-                       self->pt_nwaiters = 0;
-                       return;
-               } else {
-                       /* Wake one now. */
-                       rv = (ssize_t)_lwp_unpark(self->pt_waiters[0], NULL);
-                       self->pt_nwaiters = 0;
-                       if (rv != 0 && errno != EALREADY && errno != EINTR &&
-                           errno != ESRCH) {
-                               pthread__errorfunc(__FILE__, __LINE__, __func__,
-                                   "_lwp_unpark failed");
-                       }
-                       return;
+       switch (self->pt_nwaiters) {
+       case 0:
+               break;
+       case 1:
+               if (self->pt_willpark) {
+                       break;
                }
-       }
-
-       /* Multiple waiters. */
-       rv = _lwp_unpark_all(self->pt_waiters, self->pt_nwaiters, NULL);
-       self->pt_nwaiters = 0;
-       if (rv != 0 && errno != EINTR) {
-               pthread__errorfunc(__FILE__, __LINE__, __func__,
-                   "_lwp_unpark_all failed");
+               rv = _lwp_unpark(self->pt_waiters[0], NULL);
+               self->pt_waiters[0] = 0;
+               self->pt_nwaiters = 0;
+               if (rv != 0) {
+                       pthread__errorfunc(__FILE__, __LINE__, __func__,
+                           "_lwp_unpark failed");
+               }
+               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) {
+                       pthread__errorfunc(__FILE__, __LINE__, __func__,
+                           "_lwp_unpark_all failed");
+               }
+               break;
        }
 }
 
@@ -1115,7 +1095,7 @@
            function ? "\"" : "");
 
        _sys_write(STDERR_FILENO, buf, (size_t)len);
-       (void)_lwp_kill(_lwp_self(), SIGABRT);
+       (void)raise(SIGABRT);
        _exit(1);
 }
 
@@ -1163,16 +1143,12 @@
  * http://www.sun.com/software/whitepapers/solaris9/multithread.pdf
  */
 
-#define        OOPS(msg)                       \
-    pthread__errorfunc(__FILE__, __LINE__, __func__, msg)
-
 int
 pthread__park(pthread_t self, pthread_mutex_t *lock,
              pthread_queue_t *queue, const struct timespec *abstime,
              int cancelpt)
 {
        int rv, error;
-       void *obj;
 
        self->pt_willpark = 1;
        pthread_mutex_unlock(lock);
@@ -1186,26 +1162,15 @@
         * It is fine to test the value of pt_sleepobj without
         * holding any locks, because:
         *
-        * o Only the blocking thread (this thread) ever sets them
+        * o Only the blocking thread (this thread) ever sets it
         *   to a non-NULL value.
         *
-        * o Other threads may set them NULL, but if they do so they
+        * o Other threads may set it NULL, but if they do so they
         *   must also make this thread return from _lwp_park.
         *
         * o _lwp_park, _lwp_unpark and _lwp_unpark_all are system
         *   calls and all make use of spinlocks in the kernel.  So
-        *   these system calls act as full memory barriers, and will
-        *   ensure that the calling CPU's store buffers are drained.
-        *   In combination with the spinlock release before unpark,
-        *   this means that modification of pt_sleepobj/onq by another
-        *   thread will become globally visible before that thread
-        *   schedules an unpark operation on this thread.
-        *
-        * Note: the test in the while() statement dodges the park op if
-        * we have already been awoken, unless there is another thread to
-        * awaken.  This saves a syscall - if we were already awakened,
-        * the next call to _lwp_park() would need to return early in order
-        * to eat the previous wakeup.
+        *   these system calls act as full memory barriers.
         */
        rv = 0;
        do {
@@ -1213,9 +1178,13 @@
                 * 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_unpark, NULL, NULL);
-               self->pt_unpark = 0;
+                   __UNCONST(abstime), self->pt_waiters[0], NULL, NULL);
+               self->pt_waiters[0] = 0;
+               self->pt_nwaiters = 0;
                if (error != 0) {
                        switch (rv = errno) {
                        case EINTR:
@@ -1225,7 +1194,8 @@
                        case ETIMEDOUT:
                                break;
                        default:
-                               OOPS("_lwp_park failed");
+                               pthread__errorfunc(__FILE__, __LINE__,
+                                   __func__, "_lwp_park failed");
                                break;
                        }
                }
@@ -1233,24 +1203,6 @@
                if (cancelpt && self->pt_cancel)
                        rv = EINTR;
        } while (self->pt_sleepobj != NULL && rv == 0);
-
-       /*
-        * If we have been awoken early but are still on the queue,
-        * then remove ourself.  Again, it's safe to do the test
-        * without holding any locks.
-        */
-       if (__predict_false(self->pt_sleepobj != NULL)) {
-               pthread_mutex_lock(lock);
-               if ((obj = self->pt_sleepobj) != NULL) {
-                       PTQ_REMOVE(queue, self, pt_sleep);
-                       self->pt_sleepobj = NULL;
-                       if (obj != NULL && self->pt_early != NULL)
-                               (*self->pt_early)(obj);
-               }
-               pthread_mutex_unlock(lock);
-       }
-       self->pt_early = NULL;
-
        return rv;
 }
 
diff -r e8c15e05353d -r 1d62a3c9f726 lib/libpthread/pthread_cond.c
--- a/lib/libpthread/pthread_cond.c     Mon Jun 01 11:08:57 2020 +0000
+++ b/lib/libpthread/pthread_cond.c     Mon Jun 01 11:44:59 2020 +0000
@@ -1,7 +1,7 @@
-/*     $NetBSD: pthread_cond.c,v 1.69 2020/05/16 22:53:37 ad Exp $     */
+/*     $NetBSD: pthread_cond.c,v 1.70 2020/06/01 11:44:59 ad Exp $     */
 
 /*-
- * Copyright (c) 2001, 2006, 2007, 2008 The NetBSD Foundation, Inc.
+ * Copyright (c) 2001, 2006, 2007, 2008, 2020 The NetBSD Foundation, Inc.
  * All rights reserved.
  *
  * This code is derived from software contributed to The NetBSD Foundation
@@ -29,24 +29,8 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
-/*
- * We assume that there will be no contention on pthread_cond_t::ptc_lock
- * because functioning applications must call both the wait and wakeup
- * functions while holding the same application provided mutex.  The
- * spinlock is present only to prevent libpthread causing the application
- * to crash or malfunction as a result of corrupted data structures, in
- * the event that the application is buggy.
- *
- * If there is contention on spinlock when real-time threads are in use,
- * it could cause a deadlock due to priority inversion: the thread holding
- * the spinlock may not get CPU time to make forward progress and release
- * the spinlock to a higher priority thread that is waiting for it.
- * Contention on the spinlock will only occur with buggy applications,
- * so at the time of writing it's not considered a major bug in libpthread.
- */
-
 #include <sys/cdefs.h>
-__RCSID("$NetBSD: pthread_cond.c,v 1.69 2020/05/16 22:53:37 ad Exp $");
+__RCSID("$NetBSD: pthread_cond.c,v 1.70 2020/06/01 11:44:59 ad Exp $");
 
 #include <stdlib.h>
 #include <errno.h>
@@ -91,8 +75,7 @@
            (attr == NULL) || (attr->ptca_magic == _PT_CONDATTR_MAGIC));
 
        cond->ptc_magic = _PT_COND_MAGIC;
-       pthread_lockinit(&cond->ptc_lock);
-       PTQ_INIT(&cond->ptc_waiters);
+       cond->ptc_waiters = NULL;
        cond->ptc_mutex = NULL;
        if (attr && attr->ptca_private) {
                cond->ptc_private = malloc(sizeof(clockid_t));
@@ -116,7 +99,7 @@
        pthread__error(EINVAL, "Invalid condition variable",
            cond->ptc_magic == _PT_COND_MAGIC);
        pthread__error(EBUSY, "Destroying condition variable in use",
-           cond->ptc_mutex == NULL);
+           cond->ptc_waiters == NULL);
 
        cond->ptc_magic = _PT_COND_DEAD;
        free(cond->ptc_private);
@@ -128,7 +111,7 @@
 pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
                       const struct timespec *abstime)
 {
-       pthread_t self;
+       pthread_t self, next, waiters;
        int retval;
        clockid_t clkid = pthread_cond_getclock(cond);
 
@@ -149,52 +132,58 @@
        }
 
        /* Note this thread as waiting on the CV. */
-       pthread__spinlock(self, &cond->ptc_lock);
        cond->ptc_mutex = mutex;
-       PTQ_INSERT_HEAD(&cond->ptc_waiters, self, pt_sleep);
-       self->pt_sleepobj = cond;
-       pthread__spinunlock(self, &cond->ptc_lock);
+       self->pt_condwait = 1;
+       for (waiters = cond->ptc_waiters;; waiters = next) {
+               self->pt_condnext = waiters;
+#ifndef PTHREAD__ATOMIC_IS_MEMBAR
+               membar_producer();
+#endif
+               next = atomic_cas_ptr(&cond->ptc_waiters, waiters, self);
+               if (next == waiters)
+                       break;



Home | Main Index | Thread Index | Old Index