diff mbox

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

Message ID 1433800497.21461.455.camel@triegel.csb
State New
Headers show

Commit Message

Torvald Riegel June 8, 2015, 9:54 p.m. UTC
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.

This applies on top of the patch with the new glibc-internal futex API:
https://sourceware.org/ml/libc-alpha/2015-06/msg00284.html

Not tested at all, but this is supposed to mirror the changes applied to
the generic code.

Dave, OK to commit provided the generic patch is committed?

2015-06-08  Torvald Riegel  <triegel@redhat.com>

	* sysdeps/sparc/nptl/pthread_barrier_wait.c (pthread_barrier_wait):
	Use futex wrappers with error checking.
	* sysdeps/sparc/sparc32/pthread_barrier_wait.c (pthread_barrier_wait):
	Likewise.
	* sysdeps/sparc/sparc32/sem_init.c (futex_private_if_supported): Remove.
	* sysdeps/sparc/sparc32/sem_post.c (futex_wake): Likewise.
	* sysdeps/sparc/sparc32/sem_open.c (sem_open): Use FUTEX_SHARED.
	* sysdeps/sparc/sparc32/sem_waitcommon.c (futex_abstimed_wait): Remove.
	(futex_wake): Likewise.
	(sem_assume_only_signals_cause_futex_EINTR): Likewise.
	(do_futex_wait): Use futex wrappers with error checking.
	(__new_sem_wait_slow): Update EINTR handling.
	* sysdeps/sparc/sparc32/sem_wait.c: Include lowlevellock.h.

Comments

David Miller June 8, 2015, 10:20 p.m. UTC | #1
From: Torvald Riegel <triegel@redhat.com>
Date: Mon, 08 Jun 2015 23:54:57 +0200

> commit 0c6eaa5f5ffc15919be59ebcb4665255628fb957
> 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.

This is fine, thanks Torvald.
Roland McGrath June 9, 2015, 8:51 p.m. UTC | #2
Arguably every use of ignore_value should have a comment explaining why
it's safe to elide the error checking.
Torvald Riegel June 9, 2015, 9:11 p.m. UTC | #3
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?  Like so:

A common usage of futex_wait is to wait until a futex_word does not have
an expected value anymore:
  while (atomic_load_relaxed (&futex_word) == expected)
    ignore_value (futex_wait (&futex_word, expected, private));
In such a case, we continue waiting even if we were interrupted by a
signal (i.e., when futex_wait returned EINTR).  We could have also woken
up spuriously when 0 is returned by futex_wait, so it is easiest to just
check the value of the futex word to determine whether we need to keep
waiting.
Roland McGrath June 9, 2015, 9:35 p.m. UTC | #4
> 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.


Thanks,
Roland
diff mbox

Patch

commit 0c6eaa5f5ffc15919be59ebcb4665255628fb957
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..c46a2ff 100644
--- a/sysdeps/sparc/nptl/pthread_barrier_wait.c
+++ b/sysdeps/sparc/nptl/pthread_barrier_wait.c
@@ -21,6 +21,8 @@ 
 #include <lowlevellock.h>
 #include <pthreadP.h>
 #include <sparc-nptl.h>
+#include <futex-internal.h>
+#include <libc-internal.h>
 
 /* Wait on barrier.  */
 int
@@ -31,6 +33,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 +49,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 +65,8 @@  pthread_barrier_wait (barrier)
 
       /* Wait for the event counter of the barrier to change.  */
       do
-	lll_futex_wait (&ibarrier->b.curr_event, event, private);
+	ignore_value (futex_wait (&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..048bc40 100644
--- a/sysdeps/sparc/sparc32/pthread_barrier_wait.c
+++ b/sysdeps/sparc/sparc32/pthread_barrier_wait.c
@@ -21,6 +21,8 @@ 
 #include <lowlevellock.h>
 #include <pthreadP.h>
 #include <sparc-nptl.h>
+#include <futex-internal.h>
+#include <libc-internal.h>
 
 /* Wait on barrier.  */
 int
@@ -31,6 +33,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 +49,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 +65,8 @@  pthread_barrier_wait (barrier)
 
       /* Wait for the event counter of the barrier to change.  */
       do
-	lll_futex_wait (&ibarrier->b.curr_event, event, private);
+	ignore_value (futex_wait (&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;