Fix semaphore destruction (bug 12674).

This commit fixes semaphore destruction by either using 64b atomic operations (where available), or by using two separate fields when only 32b atomic operations are available. In the latter case, we keep a conservative estimate of whether there are any waiting threads in one bit of the field that counts the number of available tokens, thus allowing sem_post to atomically both add a token and determine whether it needs to call futex_wake. See: https://sourceware.org/ml/libc-alpha/2014-12/msg00155.html
git-mirror · Jan 21, 2015 · 042e152 · 042e152
1 parent a8db092
commit 042e152
Show file tree

Hide file tree

Showing 34 changed files with 732 additions and 2,103 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,55 @@
+2015-01-21  Torvald Riegel  <triegel@redhat.com>
+	    Carlos O'Donell  <carlos@redhat.com>
+
+	[BZ #12674]
+	* nptl/sem_waitcommon.c: New file.
+	* nptl/sem_wait.c: Include sem_waitcommon.c.
+	(__sem_wait_cleanup, do_futex_wait): Remove.
+	(__new_sem_wait): Adapt.
+	(__new_sem_trywait): New function.
+	(__old_sem_trywait): Moved here from nptl/sem_trywait.c.
+	* nptl/sem_timedwait.c: Include sem_waitcommon.c.
+	(__sem_wait_cleanup, do_futex_timed_wait): Remove.
+	(sem_timedwait): Adapt.
+	* nptl/sem_post.c (__new_sem_post): Adapt.
+	(futex_wake): New function.
+	(__old_sem_post): Add release MO fence.
+	* nptl/sem_open.c (sem_open): Adapt.
+	* nptl/sem_init.c (__new_sem_init): Adapt.
+	(futex_private_if_supported): New function.
+	* nptl/sem_getvalue.c (__new_sem_getvalue): Adapt.
+	(__old_sem_getvalue): Add using previous code.
+	* sysdeps/nptl/internaltypes.h: Adapt.
+	* nptl/tst-sem13.c (do_test): Adapt.
+	* nptl/tst-sem11.c (main): Adapt.
+	* nptl/sem_trywait.c: Remove.
+	* nptl/DESIGN-sem.txt: Remove.
+	* nptl/Makefile (libpthread-routines): Remove sem_trywait.
+	(gen-as-const-headers): Remove structsem.sym.
+	* nptl/structsem.sym: Remove.
+	* sysdeps/unix/sysv/linux/alpha/sem_post.c: Remove.
+	* sysdeps/unix/sysv/linux/i386/i486/sem_post.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i486/sem_trywait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i486/sem_wait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i586/sem_post.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i586/sem_timedwait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i586/sem_trywait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i586/sem_wait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i686/sem_post.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i686/sem_timedwait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i686/sem_trywait.S: Remove.
+	* sysdeps/unix/sysv/linux/i386/i686/sem_wait.S: Remove.
+	* sysdeps/unix/sysv/linux/powerpc/sem_post.c: Remove.
+	* sysdeps/unix/sysv/linux/sh/sem_post.S: Remove.
+	* sysdeps/unix/sysv/linux/sh/sem_timedwait.S: Remove.
+	* sysdeps/unix/sysv/linux/sh/sem_trywait.S: Remove.
+	* sysdeps/unix/sysv/linux/sh/sem_wait.S: Remove.
+	* sysdeps/unix/sysv/linux/x86_64/sem_post.S: Remove.
+	* sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S: Remove.
+	* sysdeps/unix/sysv/linux/x86_64/sem_trywait.S: Remove.
+	* sysdeps/unix/sysv/linux/x86_64/sem_wait.S: Remove.
+
 2015-01-20  Carlos O'Donell  <carlos@redhat.com>
 
 	* INSTALL: Regenerated.

diff --git a/NEWS b/NEWS
@@ -9,15 +9,22 @@ Version 2.21
 
 * The following bugs are resolved with this release:
 
-  6652, 10672, 12847, 12926, 13862, 14132, 14138, 14171, 14498, 15215,
-  15884, 16009, 16191, 16469, 16617, 16619, 16657, 16740, 16857, 17192,
-  17266, 17273, 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485,
-  17501, 17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574,
-  17582, 17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616, 17625,
-  17630, 17633, 17634, 17635, 17647, 17653, 17657, 17658, 17664, 17665,
-  17668, 17682, 17717, 17719, 17722, 17723, 17724, 17725, 17732, 17733,
-  17744, 17745, 17746, 17747, 17748, 17775, 17777, 17780, 17781, 17782,
-  17791, 17793, 17796, 17797, 17803, 17806, 17834, 17844, 17848
+  6652, 10672, 12674, 12847, 12926, 13862, 14132, 14138, 14171, 14498,
+  15215, 15884, 16009, 16191, 16469, 16617, 16619, 16657, 16740, 16857,
+  17192, 17266, 17273, 17344, 17363, 17370, 17371, 17411, 17460, 17475,
+  17485, 17501, 17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573,
+  17574, 17582, 17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616,
+  17625, 17630, 17633, 17634, 17635, 17647, 17653, 17657, 17658, 17664,
+  17665, 17668, 17682, 17717, 17719, 17722, 17723, 17724, 17725, 17732,
+  17733, 17744, 17745, 17746, 17747, 17748, 17775, 17777, 17780, 17781,
+  17782, 17791, 17793, 17796, 17797, 17803, 17806, 17834, 17844, 17848
+
+* A new semaphore algorithm has been implemented in generic C code for all
+  machines. Previous custom assembly implementations of semaphore were
+  difficult to reason about or ensure that they were safe. The new version
+  of semaphore supports machines with 64-bit or 32-bit atomic operations.
+  The new semaphore algorithm is used by sem_init, sem_open, sem_post,
+  sem_wait, sem_timedwait, sem_trywait, and sem_getvalue.
 
 * Port to Altera Nios II has been contributed by Mentor Graphics.
 

diff --git a/nptl/DESIGN-sem.txt b/nptl/DESIGN-sem.txt
diff --git a/nptl/Makefile b/nptl/Makefile
@@ -100,7 +100,7 @@ libpthread-routines = nptl-init vars events version \
 		      sem_init sem_destroy \
 		      sem_open sem_close sem_unlink \
 		      sem_getvalue \
-		      sem_wait sem_trywait sem_timedwait sem_post \
+		      sem_wait sem_timedwait sem_post \
 		      cleanup cleanup_defer cleanup_compat \
 		      cleanup_defer_compat unwind \
 		      pt-longjmp pt-cleanup\
@@ -283,8 +283,7 @@ tests-nolibpthread = tst-unload
 gen-as-const-headers = pthread-errnos.sym \
 		       lowlevelcond.sym lowlevelrwlock.sym \
 		       lowlevelbarrier.sym unwindbuf.sym \
-		       lowlevelrobustlock.sym pthread-pi-defines.sym \
-		       structsem.sym
+		       lowlevelrobustlock.sym pthread-pi-defines.sym
 
 
 LDFLAGS-pthread.so = -Wl,--enable-new-dtags,-z,nodelete,-z,initfirst

diff --git a/nptl/sem_getvalue.c b/nptl/sem_getvalue.c
@@ -19,23 +19,37 @@
 #include <semaphore.h>
 #include <shlib-compat.h>
 #include "semaphoreP.h"
+#include <atomic.h>
 
 
 int
-__new_sem_getvalue (sem, sval)
-     sem_t *sem;
-     int *sval;
+__new_sem_getvalue (sem_t *sem, int *sval)
 {
   struct new_sem *isem = (struct new_sem *) sem;
 
   /* XXX Check for valid SEM parameter.  */
-
-  *sval = isem->value;
+  /* FIXME This uses relaxed MO, even though POSIX specifies that this function
+     should be linearizable.  However, its debatable whether linearizability
+     is the right requirement.  We need to follow up with POSIX and, if
+     necessary, use a stronger MO here and elsewhere (e.g., potentially
+     release MO in all places where we consume a token).  */
+
+#if __HAVE_64B_ATOMICS
+  *sval = atomic_load_relaxed (&isem->data) & SEM_VALUE_MASK;
+#else
+  *sval = atomic_load_relaxed (&isem->value) >> SEM_VALUE_SHIFT;
+#endif
 
   return 0;
 }
 versioned_symbol (libpthread, __new_sem_getvalue, sem_getvalue, GLIBC_2_1);
 #if SHLIB_COMPAT(libpthread, GLIBC_2_0, GLIBC_2_1)
-strong_alias (__new_sem_getvalue, __old_sem_getvalue)
+int
+__old_sem_getvalue (sem_t *sem, int *sval)
+{
+  struct old_sem *isem = (struct old_sem *) sem;
+  *sval = isem->value;
+  return 0;
+}
 compat_symbol (libpthread, __old_sem_getvalue, sem_getvalue, GLIBC_2_0);
 #endif
diff --git a/nptl/sem_init.c b/nptl/sem_init.c
@@ -18,17 +18,29 @@
 
 #include <errno.h>
 #include <semaphore.h>
-#include <lowlevellock.h>
 #include <shlib-compat.h>
 #include "semaphoreP.h"
 #include <kernel-features.h>
 
+/* Returns FUTEX_PRIVATE if pshared is zero and private futexes are supported;
+   returns FUTEX_SHARED otherwise.
+   TODO Remove when cleaning up the futex API throughout glibc.  */
+static __always_inline int
+futex_private_if_supported (int pshared)
+{
+  if (pshared != 0)
+    return LLL_SHARED;
+#ifdef __ASSUME_PRIVATE_FUTEX
+  return LLL_PRIVATE;
+#else
+  return THREAD_GETMEM (THREAD_SELF, header.private_futex)
+      ^ FUTEX_PRIVATE_FLAG;
+#endif
+}
+
 
 int
-__new_sem_init (sem, pshared, value)
-     sem_t *sem;
-     int pshared;
-     unsigned int value;
+__new_sem_init (sem_t *sem, int pshared, unsigned int value)
 {
   /* Parameter sanity check.  */
   if (__glibc_unlikely (value > SEM_VALUE_MAX))
@@ -40,16 +52,15 @@ __new_sem_init (sem, pshared, value)
   /* Map to the internal type.  */
   struct new_sem *isem = (struct new_sem *) sem;
 
-  /* Use the values the user provided.  */
-  isem->value = value;
-#ifdef __ASSUME_PRIVATE_FUTEX
-  isem->private = pshared ? 0 : FUTEX_PRIVATE_FLAG;
+  /* Use the values the caller provided.  */
+#if __HAVE_64B_ATOMICS
+  isem->data = value;
 #else
-  isem->private = pshared ? 0 : THREAD_GETMEM (THREAD_SELF,
-					       header.private_futex);
+  isem->value = value << SEM_VALUE_SHIFT;
+  isem->nwaiters = 0;
 #endif
 
-  isem->nwaiters = 0;
+  isem->private = futex_private_if_supported (pshared);
 
   return 0;
 }

diff --git a/nptl/sem_open.c b/nptl/sem_open.c
@@ -193,9 +193,14 @@ sem_open (const char *name, int oflag, ...)
 	struct new_sem newsem;
       } sem;
 
-      sem.newsem.value = value;
-      sem.newsem.private = 0;
+#if __HAVE_64B_ATOMICS
+      sem.newsem.data = value;
+#else
+      sem.newsem.value = value << SEM_VALUE_SHIFT;
       sem.newsem.nwaiters = 0;
+#endif
+      /* This always is a shared semaphore.  */
+      sem.newsem.private = LLL_SHARED;
 
       /* Initialize the remaining bytes as well.  */
       memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0',

diff --git a/nptl/sem_post.c b/nptl/sem_post.c
@@ -26,34 +26,78 @@
 
 #include <shlib-compat.h>
 
+/* Wrapper for lll_futex_wake, with error checking.
+   TODO Remove when cleaning up the futex API throughout glibc.  */
+static __always_inline void
+futex_wake (unsigned int* futex, int processes_to_wake, int private)
+{
+  int res = lll_futex_wake (futex, processes_to_wake, private);
+  /* No error.  Ignore the number of woken processes.  */
+  if (res >= 0)
+    return;
+  switch (res)
+    {
+    case -EFAULT: /* Could have happened due to memory reuse.  */
+    case -EINVAL: /* Could be either due to incorrect alignment (a bug in
+		     glibc or in the application) or due to memory being
+		     reused for a PI futex.  We cannot distinguish between the
+		     two causes, and one of them is correct use, so we do not
+		     act in this case.  */
+      return;
+    case -ENOSYS: /* Must have been caused by a glibc bug.  */
+    /* No other errors are documented at this time.  */
+    default:
+      abort ();
+    }
+}
+
+
+/* See sem_wait for an explanation of the algorithm.  */
 int
 __new_sem_post (sem_t *sem)
 {
   struct new_sem *isem = (struct new_sem *) sem;
+  int private = isem->private;
 
-  __typeof (isem->value) cur;
+#if __HAVE_64B_ATOMICS
+  /* Add a token to the semaphore.  We use release MO to make sure that a
+     thread acquiring this token synchronizes with us and other threads that
+     added tokens before (the release sequence includes atomic RMW operations
+     by other threads).  */
+  /* TODO Use atomic_fetch_add to make it scale better than a CAS loop?  */
+  unsigned long int d = atomic_load_relaxed (&isem->data);
   do
     {
-      cur = isem->value;
-      if (isem->value == SEM_VALUE_MAX)
+      if ((d & SEM_VALUE_MASK) == SEM_VALUE_MAX)
 	{
 	  __set_errno (EOVERFLOW);
 	  return -1;
 	}
     }
-  while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + 1, cur));
+  while (!atomic_compare_exchange_weak_release (&isem->data, &d, d + 1));
 
-  atomic_full_barrier ();
-  if (isem->nwaiters > 0)
+  /* If there is any potentially blocked waiter, wake one of them.  */
+  if ((d >> SEM_NWAITERS_SHIFT) > 0)
+    futex_wake (((unsigned int *) &isem->data) + SEM_VALUE_OFFSET, 1, private);
+#else
+  /* Add a token to the semaphore.  Similar to 64b version.  */
+  unsigned int v = atomic_load_relaxed (&isem->value);
+  do
     {
-      int err = lll_futex_wake (&isem->value, 1,
-				isem->private ^ FUTEX_PRIVATE_FLAG);
-      if (__builtin_expect (err, 0) < 0)
+      if ((v << SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
 	{
-	  __set_errno (-err);
+	  __set_errno (EOVERFLOW);
 	  return -1;
 	}
     }
+  while (!atomic_compare_exchange_weak_release (&isem->value,
+      &v, v + (1 << SEM_VALUE_SHIFT)));
+
+  /* If there is any potentially blocked waiter, wake one of them.  */
+  if ((v & SEM_NWAITERS_MASK) != 0)
+    futex_wake (&isem->value, 1, private);
+#endif
+
   return 0;
 }
 versioned_symbol (libpthread, __new_sem_post, sem_post, GLIBC_2_1);
@@ -66,6 +110,9 @@ __old_sem_post (sem_t *sem)
 {
   int *futex = (int *) sem;
 
+  /* We must need to synchronize with consumers of this token, so the atomic
+     increment must have release MO semantics.  */
+  atomic_write_barrier ();
   (void) atomic_increment_val (futex);
   /* We always have to assume it is a shared semaphore.  */
   int err = lll_futex_wake (futex, 1, LLL_SHARED);