From patchwork Wed Jun 10 18:28:16 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 7102 Received: (qmail 95716 invoked by alias); 10 Jun 2015 18:28:27 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 94844 invoked by uid 89); 10 Jun 2015 18:28:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Subject: Re: [PATCH] Add and use new glibc-internal futex API in sparc code. From: Torvald Riegel To: Roland McGrath Cc: David Miller , GLIBC Devel In-Reply-To: <20150609213509.04CED2C3BE1@topped-with-meat.com> References: <1433800497.21461.455.camel@triegel.csb> <20150609205152.A37C02C3BDC@topped-with-meat.com> <1433884305.10071.27.camel@triegel.csb> <20150609213509.04CED2C3BE1@topped-with-meat.com> Date: Wed, 10 Jun 2015 20:28:16 +0200 Message-ID: <1433960896.10071.97.camel@triegel.csb> Mime-Version: 1.0 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. commit 379cb997b389f786b3dde3652e07b810fbe506f9 Author: Torvald Riegel 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 #include #include +#include /* 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 #include #include +#include /* 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 #include #include "semaphoreP.h" -#include - -/* 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 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 #include #include +#include #include -/* 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 . */ +#include /* 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 #include -#include +#include #include #include #include @@ -28,104 +28,6 @@ #include #include -/* 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;