diff mbox

Add and use new glibc-internal futex API in sparc code.

Message ID 1433960896.10071.97.camel@triegel.csb
State Dropped
Headers

Commit Message

Torvald Riegel June 10, 2015, 6:28 p.m. UTC 
  On Tue, 2015-06-09 at 14:35 -0700, Roland McGrath wrote:
> > On Tue, 2015-06-09 at 13:51 -0700, Roland McGrath wrote:
> > > Arguably every use of ignore_value should have a comment explaining why
> > > it's safe to elide the error checking.
> > 
> > That's always the same pattern.  Would it be okay for you if I just note
> > that pattern in the futex_wait comments?  
> 
> That is certainly good to do, but I think we still want at least a brief
> comment for every use of ignore_value where it's not totally obvious (most
> existing uses are in stubs where it's clear).  A detailed comment about the
> general case in one place means that the individual comments can just be 
> very terse like /* futex_waits errors ignored since we'll retry.  */
> 
> But if it's a commonly-repeated case, then it probably merits a wrapper
> macro that constitutes self-documentation.  i.e., define a
> "futex_wait_ignore_error" or "futex_wait_noerror" macro to use in all these
> places, and then the comment on the macro definition can explain where it's
> appropriate to use and why.  Or perhaps name the macro "futex_wait_in_loop"
> or something better that more obviously says in the name where it's
> appropriate to use rather than just what is concretely does.
> 
> The upshot is that avoiding error checking is always unusual and a casual
> reader of just a little bit of code (especially a reviewer seeing just the
> patch context) should not have to hunt around to convince himself that it
> is safe.  If it's a macro that makes its uses self-documenting, then that
> does it.  If it's a terse comment that points clearly to where fuller
> rationale can be read, then that does it too.

I've added a futex_wait_simple call that returns void and explains the
use case.  See my updated patch for the futex API.  Attached is the
updated sparc code for completeness.
diff mbox

Patch

commit 379cb997b389f786b3dde3652e07b810fbe506f9
Author: Torvald Riegel <triegel@redhat.com>
Date:   Mon Jun 8 23:47:26 2015 +0200

    Add and use new glibc-internal futex API in sparc code.
    
    This uses the futex API in the sparc-specific code.  It also cleans up
    EINTR handling in the sparc semaphore code in that it follows the return
    code documentation of futex_wait.

diff --git a/sysdeps/sparc/nptl/pthread_barrier_wait.c b/sysdeps/sparc/nptl/pthread_barrier_wait.c
index b0437a1..890b5c9 100644
--- a/sysdeps/sparc/nptl/pthread_barrier_wait.c
+++ b/sysdeps/sparc/nptl/pthread_barrier_wait.c
@@ -21,6 +21,7 @@ 
 #include <lowlevellock.h>
 #include <pthreadP.h>
 #include <sparc-nptl.h>
+#include <futex-internal.h>
 
 /* Wait on barrier.  */
 int
@@ -31,6 +32,7 @@  pthread_barrier_wait (barrier)
     = (union sparc_pthread_barrier *) barrier;
   int result = 0;
   int private = ibarrier->s.pshared ? LLL_SHARED : LLL_PRIVATE;
+  int futex_private = ibarrier->s.pshared ? FUTEX_SHARED : FUTEX_PRIVATE;
 
   /* Make sure we are alone.  */
   lll_lock (ibarrier->b.lock, private);
@@ -46,7 +48,7 @@  pthread_barrier_wait (barrier)
       ++ibarrier->b.curr_event;
 
       /* Wake up everybody.  */
-      lll_futex_wake (&ibarrier->b.curr_event, INT_MAX, private);
+      futex_wake (&ibarrier->b.curr_event, INT_MAX, futex_private);
 
       /* This is the thread which finished the serialization.  */
       result = PTHREAD_BARRIER_SERIAL_THREAD;
@@ -62,7 +64,7 @@  pthread_barrier_wait (barrier)
 
       /* Wait for the event counter of the barrier to change.  */
       do
-	lll_futex_wait (&ibarrier->b.curr_event, event, private);
+	futex_wait_simple (&ibarrier->b.curr_event, event, futex_private);
       while (event == ibarrier->b.curr_event);
     }
 
diff --git a/sysdeps/sparc/sparc32/pthread_barrier_wait.c b/sysdeps/sparc/sparc32/pthread_barrier_wait.c
index afab203..6d87991 100644
--- a/sysdeps/sparc/sparc32/pthread_barrier_wait.c
+++ b/sysdeps/sparc/sparc32/pthread_barrier_wait.c
@@ -21,6 +21,7 @@ 
 #include <lowlevellock.h>
 #include <pthreadP.h>
 #include <sparc-nptl.h>
+#include <futex-internal.h>
 
 /* Wait on barrier.  */
 int
@@ -31,6 +32,7 @@  pthread_barrier_wait (barrier)
     = (union sparc_pthread_barrier *) barrier;
   int result = 0;
   int private = ibarrier->s.pshared ? LLL_SHARED : LLL_PRIVATE;
+  int futex_private = ibarrier->s.pshared ? FUTEX_SHARED : FUTEX_PRIVATE;
 
   /* Make sure we are alone.  */
   lll_lock (ibarrier->b.lock, private);
@@ -46,7 +48,7 @@  pthread_barrier_wait (barrier)
       ++ibarrier->b.curr_event;
 
       /* Wake up everybody.  */
-      lll_futex_wake (&ibarrier->b.curr_event, INT_MAX, private);
+      futex_wake (&ibarrier->b.curr_event, INT_MAX, futex_private);
 
       /* This is the thread which finished the serialization.  */
       result = PTHREAD_BARRIER_SERIAL_THREAD;
@@ -62,7 +64,7 @@  pthread_barrier_wait (barrier)
 
       /* Wait for the event counter of the barrier to change.  */
       do
-	lll_futex_wait (&ibarrier->b.curr_event, event, private);
+	futex_wait_simple (&ibarrier->b.curr_event, event, futex_private);
       while (event == ibarrier->b.curr_event);
     }
 
diff --git a/sysdeps/sparc/sparc32/sem_init.c b/sysdeps/sparc/sparc32/sem_init.c
index 7c46cee..9b02df2 100644
--- a/sysdeps/sparc/sparc32/sem_init.c
+++ b/sysdeps/sparc/sparc32/sem_init.c
@@ -20,23 +20,7 @@ 
 #include <semaphore.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
-}
+#include <futex-internal.h>
 
 
 int
diff --git a/sysdeps/sparc/sparc32/sem_open.c b/sysdeps/sparc/sparc32/sem_open.c
index af9233c..8c4889b 100644
--- a/sysdeps/sparc/sparc32/sem_open.c
+++ b/sysdeps/sparc/sparc32/sem_open.c
@@ -198,7 +198,7 @@  sem_open (const char *name, int oflag, ...)
       sem.newsem.nwaiters = 0;
 
       /* This always is a shared semaphore.  */
-      sem.newsem.private = LLL_SHARED;
+      sem.newsem.private = FUTEX_SHARED;
 
       /* Initialize the remaining bytes as well.  */
       memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0',
diff --git a/sysdeps/sparc/sparc32/sem_post.c b/sysdeps/sparc/sparc32/sem_post.c
index c9f85a0..fd1a2fe 100644
--- a/sysdeps/sparc/sparc32/sem_post.c
+++ b/sysdeps/sparc/sparc32/sem_post.c
@@ -23,34 +23,10 @@ 
 #include <lowlevellock.h>
 #include <internaltypes.h>
 #include <semaphore.h>
+#include <futex-internal.h>
 
 #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
diff --git a/sysdeps/sparc/sparc32/sem_wait.c b/sysdeps/sparc/sparc32/sem_wait.c
index c1fd10c..fce7ed4 100644
--- a/sysdeps/sparc/sparc32/sem_wait.c
+++ b/sysdeps/sparc/sparc32/sem_wait.c
@@ -17,6 +17,7 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <lowlevellock.h>	/* lll_futex* used by the old code.  */
 #include "sem_waitcommon.c"
 
 int
diff --git a/sysdeps/sparc/sparc32/sem_waitcommon.c b/sysdeps/sparc/sparc32/sem_waitcommon.c
index 9c1c6a5..9e43d45 100644
--- a/sysdeps/sparc/sparc32/sem_waitcommon.c
+++ b/sysdeps/sparc/sparc32/sem_waitcommon.c
@@ -19,7 +19,7 @@ 
 
 #include <errno.h>
 #include <sysdep.h>
-#include <lowlevellock.h>
+#include <futex-internal.h>
 #include <internaltypes.h>
 #include <semaphore.h>
 #include <sys/time.h>
@@ -28,104 +28,6 @@ 
 #include <shlib-compat.h>
 #include <atomic.h>
 
-/* Wrapper for lll_futex_wait with absolute timeout and error checking.
-   TODO Remove when cleaning up the futex API throughout glibc.  */
-static __always_inline int
-futex_abstimed_wait (unsigned int* futex, unsigned int expected,
-		     const struct timespec* abstime, int private, bool cancel)
-{
-  int err, oldtype;
-  if (abstime == NULL)
-    {
-      if (cancel)
-	oldtype = __pthread_enable_asynccancel ();
-      err = lll_futex_wait (futex, expected, private);
-      if (cancel)
-	__pthread_disable_asynccancel (oldtype);
-    }
-  else
-    {
-      struct timeval tv;
-      struct timespec rt;
-      int sec, nsec;
-
-      /* Get the current time.  */
-      __gettimeofday (&tv, NULL);
-
-      /* Compute relative timeout.  */
-      sec = abstime->tv_sec - tv.tv_sec;
-      nsec = abstime->tv_nsec - tv.tv_usec * 1000;
-      if (nsec < 0)
-        {
-          nsec += 1000000000;
-          --sec;
-        }
-
-      /* Already timed out?  */
-      if (sec < 0)
-        return ETIMEDOUT;
-
-      /* Do wait.  */
-      rt.tv_sec = sec;
-      rt.tv_nsec = nsec;
-      if (cancel)
-	oldtype = __pthread_enable_asynccancel ();
-      err = lll_futex_timed_wait (futex, expected, &rt, private);
-      if (cancel)
-	__pthread_disable_asynccancel (oldtype);
-    }
-  switch (err)
-    {
-    case 0:
-    case -EAGAIN:
-    case -EINTR:
-    case -ETIMEDOUT:
-      return -err;
-
-    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
-    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
-		     being normalized.  Must have been caused by a glibc or
-		     application bug.  */
-    case -ENOSYS: /* Must have been caused by a glibc bug.  */
-    /* No other errors are documented at this time.  */
-    default:
-      abort ();
-    }
-}
-
-/* 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 ();
-    }
-}
-
-
-/* Set this to true if you assume that, in contrast to current Linux futex
-   documentation, lll_futex_wake can return -EINTR only if interrupted by a
-   signal, not spuriously due to some other reason.
-   TODO Discuss EINTR conditions with the Linux kernel community.  For
-   now, we set this to true to not change behavior of semaphores compared
-   to previous glibc builds.  */
-static const int sem_assume_only_signals_cause_futex_EINTR = 1;
 
 static void
 __sem_wait_32_finish (struct new_sem *sem);
@@ -149,8 +51,8 @@  do_futex_wait (struct new_sem *sem, const struct timespec *abstime)
 {
   int err;
 
-  err = futex_abstimed_wait (&sem->value, SEM_NWAITERS_MASK, abstime,
-			     sem->private, true);
+  err = futex_abstimed_wait_cancelable (&sem->value, SEM_NWAITERS_MASK,
+					abstime, sem->private);
 
   return err;
 }
@@ -202,8 +104,7 @@  __new_sem_wait_slow (struct new_sem *sem, const struct timespec *abstime)
 	  __sparc32_atomic_do_unlock24(&sem->pad);
 
 	  err = do_futex_wait(sem, abstime);
-	  if (err == ETIMEDOUT ||
-	      (err == EINTR && sem_assume_only_signals_cause_futex_EINTR))
+	  if (err == ETIMEDOUT || err == EINTR)
 	    {
 	      __set_errno (err);
 	      err = -1;