Message ID | 20190212201819.23399-1-adhemerval.zanella@linaro.org |
---|---|
State | Accepted |
Commit | eb76e5b465a4b7b569cde4b4f57d1fcb4695c1c6 |
Headers | show |
Series | nptl: Reinstate pthread_timedjoin_np as a cancellation point (BZ#24215) | expand |
On 2/12/19 3:18 PM, Adhemerval Zanella wrote: > Patch ce7eb0e90315 ("nptl: Cleanup cancellation macros") changed the > join sequence for internal common __pthread_timedjoin_ex to use the > new macro lll_wait_tid. The idea was this macro would issue the > cancellable futex operation depending whether the timeout is used or > not. However if a timeout is used, __lll_timedwait_tid is called and > it is not a cancellable entrypoint. Thanks for following up on this. OK for master with additional comment below (see Fix:). Once you commit I'll clean up the straggling comments that I fixed before (pthread_tryjoin.c still has a wrong comment). Reviewed-by: Carlos O'Donell <carlos@redhat.com> > This patch fixes it by simplifying the code in various ways: > > - Instead of adding the cancellation handling on __lll_timedwait_tid, > it moves the generic implementation to pthread_join_common.c (called > now timedwait_tid with some fixes to use the correct type for pid). > > - The llvm_wait_tid macro is removed, along with its replication on > x86_64, i686, and sparc arch-specific lowlevellock.h. > > - sparc32 __lll_timedwait_tid is also removed, since the code is similar > to generic one. > > - x86_64 and i386 provides arch-specific __lll_timedwait_tid which is > also removed since they are similar in functionality to generic C code > and there is no indication it is better than compiler generated code. Yay! > New tests, tst-join8 and tst-join9, are provided to check if > pthread_timedjoin_np acts as a cancellation point. Double yay! > Checked on x86_64-linux-gnu, i686-linux-gnu, sparcv9-linux-gnu, and > aarch64-linux-gnu. > > [BZ #2421] > * nptl/Makefile (lpthread-routines): Remove lll_timedwait_tid. > (tests): Add tst-join8 tst-join9. > * nptl/lll_timedwait_tid.c: Remove file. > * sysdeps/sparc/sparc32/lll_timedwait_tid.c: Likewise. > * sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c: Likewise. > * sysdeps/sysv/linux/x86_64/lll_timedwait_tid.c: Likewise. > * nptl/pthread_join_common.c (timedwait_tid): New function. > (__pthread_timedjoin_ex): Act as cancellation entrypoint is block > is set. > * nptl/tst-join5.c (thread_join): New function. > (tf1, tf2, do_test): Use libsupport and add pthread_timedjoin_np > check. > * nptl/tst-join8.c: New file. > * nptl/tst-join9.c: Likewise. > * sysdeps/nptl/lowlevellock-futex.h (lll_futex_wait_cancel, > lll_futex_timed_wait_cancel): Add generic macros. > * sysdeps/nptl/lowlevellock.h (__lll_timedwait_tid, lll_wait_tid): > Remove definitions. > * sysdeps/unix/sysv/linux/i386/lowlevellock.h: Likewise. > * sysdeps/unix/sysv/linux/sparc/lowlevellock.h: Likewise. > * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Likewise. > * sysdeps/sparc/sparc32/lowlevellock.c (__lll_timedwait_tid): > Remove function. > * sysdeps/unix/sysv/linux/i386/lowlevellock.S (__lll_timedwait_tid): > Likewise. > * sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: Likewise. > * sysdeps/unix/sysv/linux/lowlevellock-futex.h > (lll_futex_timed_wait_cancel): New macro. > --- > nptl/Makefile | 3 +- > nptl/lll_timedwait_tid.c | 70 -------------- > nptl/pthread_join_common.c | 57 +++++++++++- > nptl/tst-join5.c | 92 +++++++------------ > nptl/tst-join8.c | 20 ++++ > nptl/tst-join9.c | 21 +++++ > sysdeps/nptl/lowlevellock-futex.h | 7 ++ > sysdeps/nptl/lowlevellock.h | 26 ------ > sysdeps/sparc/sparc32/lll_timedwait_tid.c | 1 - > sysdeps/sparc/sparc32/lowlevellock.c | 40 -------- > .../unix/sysv/linux/i386/lll_timedwait_tid.c | 1 - > sysdeps/unix/sysv/linux/i386/lowlevellock.S | 66 ------------- > sysdeps/unix/sysv/linux/i386/lowlevellock.h | 25 ----- > sysdeps/unix/sysv/linux/lowlevellock-futex.h | 7 ++ > sysdeps/unix/sysv/linux/sparc/lowlevellock.h | 25 ----- > .../sysv/linux/x86_64/lll_timedwait_tid.c | 1 - > sysdeps/unix/sysv/linux/x86_64/lowlevellock.S | 83 ----------------- > sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 25 ----- > 18 files changed, 147 insertions(+), 423 deletions(-) > delete mode 100644 nptl/lll_timedwait_tid.c > create mode 100644 nptl/tst-join8.c > create mode 100644 nptl/tst-join9.c > delete mode 100644 sysdeps/sparc/sparc32/lll_timedwait_tid.c > delete mode 100644 sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c > delete mode 100644 sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c > > diff --git a/nptl/Makefile b/nptl/Makefile > index 0e316edfac..e79ca8f064 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -120,7 +120,7 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \ > pt-longjmp pt-cleanup\ > cancellation \ > lowlevellock \ > - lll_timedlock_wait lll_timedwait_tid \ > + lll_timedlock_wait \ OK. > pt-fork pt-vfork pt-fcntl \ > $(pthread-compat-wrappers) \ > pt-raise pt-system \ > @@ -271,6 +271,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \ > tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \ > tst-raise1 \ > tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \ > + tst-join8 tst-join9 \ OK. > tst-detach1 \ > tst-eintr1 tst-eintr2 tst-eintr3 tst-eintr4 tst-eintr5 \ > tst-tsd1 tst-tsd2 tst-tsd3 tst-tsd4 tst-tsd5 tst-tsd6 \ > diff --git a/nptl/lll_timedwait_tid.c b/nptl/lll_timedwait_tid.c > deleted file mode 100644 > index 59ecbb4446..0000000000 > --- a/nptl/lll_timedwait_tid.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -/* Timed waiting for thread death. Generic futex-using version. > - Copyright (C) 2003-2019 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library; if not, see > - <http://www.gnu.org/licenses/>. */ > - > -#include <atomic.h> > -#include <errno.h> > -#include <lowlevellock.h> > -#include <sys/time.h> > - > - > -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > - wake-up when the clone terminates. The memory location contains the > - thread ID while the clone is running and is reset to zero by the kernel > - afterwards. The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. */ > -int > -__lll_timedwait_tid (int *tidp, const struct timespec *abstime) > -{ > - int tid; > - > - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) > - return EINVAL; > - > - /* Repeat until thread terminated. */ > - while ((tid = *tidp) != 0) > - { > - struct timeval tv; > - struct timespec rt; > - > - /* Get the current time. */ > - (void) __gettimeofday (&tv, NULL); > - > - /* Compute relative timeout. */ > - rt.tv_sec = abstime->tv_sec - tv.tv_sec; > - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; > - if (rt.tv_nsec < 0) > - { > - rt.tv_nsec += 1000000000; > - --rt.tv_sec; > - } > - > - /* Already timed out? */ > - if (rt.tv_sec < 0) > - return ETIMEDOUT; > - > - /* If *tidp == tid, wait until thread terminates or the wait times out. > - The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. > - */ > - if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) > - return ETIMEDOUT; > - } > - > - return 0; > -} OK. > diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c > index ecb78ffba5..3f9cf1674b 100644 > --- a/nptl/pthread_join_common.c > +++ b/nptl/pthread_join_common.c OK. The comment I pointed out as wrong in pthread_join_common.c is now correct thanks to this change. > @@ -30,6 +30,52 @@ cleanup (void *arg) > atomic_compare_exchange_weak_acquire (&arg, &self, NULL); > } > > +/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > + wake-up when the clone terminates. The memory location contains the > + thread ID while the clone is running and is reset to zero by the kernel > + afterwards. The kernel up to version 3.16.3 does not use the private futex > + operations for futex wake-up when the clone terminates. */ > +static int > +timedwait_tid (pid_t *tidp, const struct timespec *abstime) > +{ > + pid_t tid; > + > + if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) > + return EINVAL; > + > + /* Repeat until thread terminated. */ > + while ((tid = *tidp) != 0) > + { > + struct timeval tv; > + struct timespec rt; > + > + /* Get the current time. */ > + __gettimeofday (&tv, NULL); > + > + /* Compute relative timeout. */ > + rt.tv_sec = abstime->tv_sec - tv.tv_sec; > + rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; > + if (rt.tv_nsec < 0) > + { > + rt.tv_nsec += 1000000000; > + --rt.tv_sec; > + } > + > + /* Already timed out? */ > + if (rt.tv_sec < 0) > + return ETIMEDOUT; > + > + /* If *tidp == tid, wait until thread terminates or the wait times out. > + The kernel up to version 3.16.3 does not use the private futex > + operations for futex wake-up when the clone terminates. */ > + if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED) OK, uses the *cancel variant. Fix: Needs a comment explaining *why* we use the cancel variant. > + == -ETIMEDOUT) > + return ETIMEDOUT; > + } > + > + return 0; > +} OK. > + > int > __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, > const struct timespec *abstime, bool block) > @@ -81,7 +127,16 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, > un-wait-ed for again. */ > pthread_cleanup_push (cleanup, &pd->joinid); > > - result = lll_wait_tid (pd->tid, abstime); > + if (abstime != NULL) > + result = timedwait_tid (&pd->tid, abstime); OK. > + else > + { > + pid_t tid; > + /* We need acquire MO here so that we synchronize with the > + kernel's store to 0 when the clone terminates. (see above) */ > + while ((tid = atomic_load_acquire (&pd->tid)) != 0) > + lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED); OK. Fix: Needs a comment explaining why we need cancellation. > + } > > pthread_cleanup_pop (0); > } > diff --git a/nptl/tst-join5.c b/nptl/tst-join5.c > index f60a024baa..39f74dce09 100644 > --- a/nptl/tst-join5.c > +++ b/nptl/tst-join5.c > @@ -23,6 +23,8 @@ > #include <time.h> > #include <unistd.h> > > +#include <support/check.h> > +#include <support/xthread.h> OK. > > static void > wait_code (void) > @@ -37,22 +39,31 @@ wait_code (void) > static pthread_barrier_t b; > #endif > > +static int > +thread_join (pthread_t thread, void **retval) > +{ > +#ifdef USE_PTHREAD_TIMEDJOIN_NP > + struct timespec tv; > + TEST_COMPARE (clock_gettime (CLOCK_REALTIME, &tv), 0); > + /* Arbitrary large timeout to make it act as pthread_join. */ OK. > + tv.tv_sec += 1000; > + return pthread_timedjoin_np ((pthread_t) thread, retval, &tv); > +#else > + return pthread_join ((pthread_t) thread, retval); > +#endif OK. > +} > + > > static void * > tf1 (void *arg) > { > #ifdef WAIT_IN_CHILD > - int e = pthread_barrier_wait (&b); > - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) > - { > - printf ("%s: barrier_wait failed\n", __func__); > - exit (1); > - } > + xpthread_barrier_wait (&b); OK. > > wait_code (); > #endif > > - pthread_join ((pthread_t) arg, NULL); > + thread_join ((pthread_t) arg, NULL); OK. > > exit (42); > } > @@ -62,16 +73,12 @@ static void * > tf2 (void *arg) > { > #ifdef WAIT_IN_CHILD > - int e = pthread_barrier_wait (&b); > - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) > - { > - printf ("%s: barrier_wait failed\n", __func__); > - exit (1); > - } > + xpthread_barrier_wait (&b); OK. > > wait_code (); > #endif > - pthread_join ((pthread_t) arg, NULL); > + > + thread_join ((pthread_t) arg, NULL); OK. > > exit (43); > } > @@ -81,16 +88,12 @@ static int > do_test (void) > { > #ifdef WAIT_IN_CHILD > - if (pthread_barrier_init (&b, NULL, 2) != 0) > - { > - puts ("barrier_init failed"); > - return 1; > - } > + xpthread_barrier_init (&b, NULL, 2); OK. > #endif > > pthread_t th; > > - int err = pthread_join (pthread_self (), NULL); > + int err = thread_join (pthread_self (), NULL); OK. > if (err == 0) > { > puts ("1st circular join succeeded"); > @@ -102,33 +105,20 @@ do_test (void) > return 1; > } > > - if (pthread_create (&th, NULL, tf1, (void *) pthread_self ()) != 0) > - { > - puts ("1st create failed"); > - return 1; > - } > + th = xpthread_create (NULL, tf1, (void *) pthread_self ()); OK. > > #ifndef WAIT_IN_CHILD > wait_code (); > #endif > > - if (pthread_cancel (th) != 0) > - { > - puts ("cannot cancel 1st thread"); > - return 1; > - } > + xpthread_cancel (th); OK. > > #ifdef WAIT_IN_CHILD > - int e = pthread_barrier_wait (&b); > - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) > - { > - printf ("%s: barrier_wait failed\n", __func__); > - return 1; > - } > + xpthread_barrier_wait (&b); OK. > #endif > > void *r; > - err = pthread_join (th, &r); > + err = thread_join (th, &r); OK. > if (err != 0) > { > printf ("cannot join 1st thread: %d\n", err); > @@ -140,7 +130,7 @@ do_test (void) > return 1; > } > > - err = pthread_join (pthread_self (), NULL); > + err = thread_join (pthread_self (), NULL); OK. > if (err == 0) > { > puts ("2nd circular join succeeded"); > @@ -152,32 +142,19 @@ do_test (void) > return 1; > } > > - if (pthread_create (&th, NULL, tf2, (void *) pthread_self ()) != 0) > - { > - puts ("2nd create failed"); > - return 1; > - } > + th = xpthread_create (NULL, tf2, (void *) pthread_self ()); OK. > > #ifndef WAIT_IN_CHILD > wait_code (); > #endif > > - if (pthread_cancel (th) != 0) > - { > - puts ("cannot cancel 2nd thread"); > - return 1; > - } > + xpthread_cancel (th); OK. > > #ifdef WAIT_IN_CHILD > - e = pthread_barrier_wait (&b); > - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) > - { > - printf ("%s: barrier_wait failed\n", __func__); > - return 1; > - } > + xpthread_barrier_wait (&b); OK. > #endif > > - if (pthread_join (th, &r) != 0) > + if (thread_join (th, &r) != 0) OK. > { > puts ("cannot join 2nd thread"); > return 1; > @@ -188,7 +165,7 @@ do_test (void) > return 1; > } > > - err = pthread_join (pthread_self (), NULL); > + err = thread_join (pthread_self (), NULL); OK. > if (err == 0) > { > puts ("3rd circular join succeeded"); > @@ -203,5 +180,4 @@ do_test (void) > return 0; > } > > -#define TEST_FUNCTION do_test () > -#include "../test-skeleton.c" > +#include <support/test-driver.c> OK. > diff --git a/nptl/tst-join8.c b/nptl/tst-join8.c > new file mode 100644 > index 0000000000..f3241fae1c > --- /dev/null > +++ b/nptl/tst-join8.c > @@ -0,0 +1,20 @@ > +/* Check if pthread_timedjoin_np is a cancellation entrypoint. OK. > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#define USE_PTHREAD_TIMEDJOIN_NP 1 > +#include <nptl/tst-join5.c> OK. > diff --git a/nptl/tst-join9.c b/nptl/tst-join9.c > new file mode 100644 > index 0000000000..7a47f582f1 > --- /dev/null > +++ b/nptl/tst-join9.c > @@ -0,0 +1,21 @@ > +/* Check if pthread_timedjoin_np is a cancellation entrypoint. > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#define USE_PTHREAD_TIMEDJOIN_NP 1 > +#define WAIT_IN_CHILD 1 > +#include <nptl/tst-join5.c> OK. > diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h > index 50ceba71ff..bb8effe288 100644 > --- a/sysdeps/nptl/lowlevellock-futex.h > +++ b/sysdeps/nptl/lowlevellock-futex.h > @@ -82,5 +82,12 @@ > val, private) \ > -ENOSYS > > +/* Like lll_futex_wait, but acting as a cancellable entrypoint. */ > +#define lll_futex_wait_cancel(futexp, val, private) \ > + -ENOSYS > + > +/* Like lll_futex_timed_wait, but acting as a cancellable entrypoint. */ > +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \ > + -ENOSYS OK. > > #endif /* lowlevellock-futex.h */ > diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h > index 36bbc472e7..6f017afdd5 100644 > --- a/sysdeps/nptl/lowlevellock.h > +++ b/sysdeps/nptl/lowlevellock.h > @@ -175,30 +175,4 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, > #define LLL_LOCK_INITIALIZER (0) > #define LLL_LOCK_INITIALIZER_LOCKED (1) > > -extern int __lll_timedwait_tid (int *, const struct timespec *) > - attribute_hidden; > - > -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > - wake-up when the clone terminates. The memory location contains the > - thread ID while the clone is running and is reset to zero by the kernel > - afterwards. The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. > - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout > - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. > - The futex operation are issues with cancellable versions. */ > -#define lll_wait_tid(tid, abstime) \ > - ({ \ > - int __res = 0; \ > - __typeof (tid) __tid; \ > - if (abstime != NULL) \ > - __res = __lll_timedwait_tid (&(tid), (abstime)); \ > - else \ > - /* We need acquire MO here so that we synchronize with the \ > - kernel's store to 0 when the clone terminates. (see above) */ \ > - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ > - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ > - __res; \ > - }) > - > - OK. Unused. > #endif /* lowlevellock.h */ > diff --git a/sysdeps/sparc/sparc32/lll_timedwait_tid.c b/sysdeps/sparc/sparc32/lll_timedwait_tid.c > deleted file mode 100644 > index 511608ead9..0000000000 > --- a/sysdeps/sparc/sparc32/lll_timedwait_tid.c > +++ /dev/null > @@ -1 +0,0 @@ > -/* __lll_timedwait_tid is in lowlevellock.c. */ > diff --git a/sysdeps/sparc/sparc32/lowlevellock.c b/sysdeps/sparc/sparc32/lowlevellock.c > index a6b2961bd3..1a0b7bbe00 100644 > --- a/sysdeps/sparc/sparc32/lowlevellock.c > +++ b/sysdeps/sparc/sparc32/lowlevellock.c > @@ -88,44 +88,4 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) > > return 0; > } > - > - > -int > -__lll_timedwait_tid (int *tidp, const struct timespec *abstime) > -{ > - int tid; > - > - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) > - return EINVAL; > - > - /* Repeat until thread terminated. */ > - while ((tid = *tidp) != 0) > - { > - struct timeval tv; > - struct timespec rt; > - > - /* Get the current time. */ > - (void) __gettimeofday (&tv, NULL); > - > - /* Compute relative timeout. */ > - rt.tv_sec = abstime->tv_sec - tv.tv_sec; > - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; > - if (rt.tv_nsec < 0) > - { > - rt.tv_nsec += 1000000000; > - --rt.tv_sec; > - } > - > - /* Already timed out? */ > - if (rt.tv_sec < 0) > - return ETIMEDOUT; > - > - /* Wait until thread terminates. The kernel so far does not use > - the private futex operations for this. */ > - if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) > - return ETIMEDOUT; > - } > - > - return 0; > -} OK. Removed. > #endif > diff --git a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c > deleted file mode 100644 > index 43900c6294..0000000000 > --- a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c > +++ /dev/null > @@ -1 +0,0 @@ > -/* __lll_timedwait_tid is in lowlevellock.S. */ > diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.S b/sysdeps/unix/sysv/linux/i386/lowlevellock.S > index b5f5b5b11c..83191a3899 100644 > --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.S > +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.S > @@ -365,70 +365,4 @@ __lll_unlock_wake: > ret > cfi_endproc > .size __lll_unlock_wake,.-__lll_unlock_wake > - > - .globl __lll_timedwait_tid > - .type __lll_timedwait_tid,@function > - .hidden __lll_timedwait_tid > - .align 16 > -__lll_timedwait_tid: > - pushl %edi > - pushl %esi > - pushl %ebx > - pushl %ebp > - > - movl %eax, %ebp > - movl %edx, %edi > - subl $8, %esp > - > - /* Get current time. */ > -2: movl %esp, %ebx > - xorl %ecx, %ecx > - movl $__NR_gettimeofday, %eax > - ENTER_KERNEL > - > - /* Compute relative timeout. */ > - movl 4(%esp), %eax > - movl $1000, %edx > - mul %edx /* Milli seconds to nano seconds. */ > - movl (%edi), %ecx > - movl 4(%edi), %edx > - subl (%esp), %ecx > - subl %eax, %edx > - jns 5f > - addl $1000000000, %edx > - subl $1, %ecx > -5: testl %ecx, %ecx > - js 6f /* Time is already up. */ > - > - movl %ecx, (%esp) /* Store relative timeout. */ > - movl %edx, 4(%esp) > - > - movl (%ebp), %edx > - testl %edx, %edx > - jz 4f > - > - movl %esp, %esi > - /* XXX The kernel so far uses global futex for the wakeup at > - all times. */ > - xorl %ecx, %ecx /* movl $FUTEX_WAIT, %ecx */ > - movl %ebp, %ebx > - movl $SYS_futex, %eax > - ENTER_KERNEL > - > - cmpl $0, (%ebx) > - jne 1f > -4: xorl %eax, %eax > - > -3: addl $8, %esp > - popl %ebp > - popl %ebx > - popl %esi > - popl %edi > - ret > - > -1: cmpl $-ETIMEDOUT, %eax > - jne 2b > -6: movl $ETIMEDOUT, %eax > - jmp 3b > - .size __lll_timedwait_tid,.-__lll_timedwait_tid OK. ZOMG thank you for deleting assembly. > #endif > diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h > index aa8f4bfdf6..94dccc4ce7 100644 > --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h > +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h > @@ -219,31 +219,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count, > #define lll_islocked(futex) \ > (futex != LLL_LOCK_INITIALIZER) > > -extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime) > - __attribute__ ((regparm (2))) attribute_hidden; > - > -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > - wake-up when the clone terminates. The memory location contains the > - thread ID while the clone is running and is reset to zero by the kernel > - afterwards. The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. > - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout > - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. > - The futex operation are issues with cancellable versions. */ > -#define lll_wait_tid(tid, abstime) \ > - ({ \ > - int __res = 0; \ > - __typeof (tid) __tid; \ > - if (abstime != NULL) \ > - __res = __lll_timedwait_tid (&(tid), (abstime)); \ > - else \ > - /* We need acquire MO here so that we synchronize with the \ > - kernel's store to 0 when the clone terminates. (see above) */ \ > - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ > - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ > - __res; \ > - }) > - OK. Thank you for deleting duplicate code. > extern int __lll_lock_elision (int *futex, short *adapt_count, int private) > attribute_hidden; > > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > index 8ca3417aaa..6f060b1739 100644 > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > @@ -135,6 +135,13 @@ > __err; \ > }) > > +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \ > + ({ \ > + int __oldtype = CANCEL_ASYNC (); \ > + long int __err = lll_futex_timed_wait (futexp, val, timeout, private); \ > + CANCEL_RESET (__oldtype); \ > + __err; \ > + }) OK. Acceptable. Thank you for the generic version. > > #endif /* !__ASSEMBLER__ */ > > diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h > index 5aed572d43..c778f4259b 100644 > --- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h > +++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h > @@ -108,29 +108,4 @@ __lll_timedlock (int *futex, const struct timespec *abstime, int private) > #define LLL_LOCK_INITIALIZER (0) > #define LLL_LOCK_INITIALIZER_LOCKED (1) > > -extern int __lll_timedwait_tid (int *, const struct timespec *) > - attribute_hidden; > - > -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > - wake-up when the clone terminates. The memory location contains the > - thread ID while the clone is running and is reset to zero by the kernel > - afterwards. The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. > - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout > - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. > - The futex operation are issues with cancellable versions. */ > -#define lll_wait_tid(tid, abstime) \ > - ({ \ > - int __res = 0; \ > - __typeof (tid) __tid; \ > - if (abstime != NULL) \ > - __res = __lll_timedwait_tid (&(tid), (abstime)); \ > - else \ > - /* We need acquire MO here so that we synchronize with the \ > - kernel's store to 0 when the clone terminates. (see above) */ \ > - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ > - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ > - __res; \ > - }) > - OK. Deleted unused. > #endif /* lowlevellock.h */ > diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c > deleted file mode 100644 > index 43900c6294..0000000000 > --- a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c > +++ /dev/null > @@ -1 +0,0 @@ > -/* __lll_timedwait_tid is in lowlevellock.S. */ > diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S > index f9abb4cb1f..166dbcfd3b 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S > +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S > @@ -345,87 +345,4 @@ __lll_unlock_wake: > retq > cfi_endproc > .size __lll_unlock_wake,.-__lll_unlock_wake > - > - .globl __lll_timedwait_tid > - .type __lll_timedwait_tid,@function > - .hidden __lll_timedwait_tid > - .align 16 > -__lll_timedwait_tid: > - cfi_startproc > - pushq %r12 > - cfi_adjust_cfa_offset(8) > - pushq %r13 > - cfi_adjust_cfa_offset(8) > - cfi_offset(%r12, -16) > - cfi_offset(%r13, -24) > - > - movq %rdi, %r12 > - movq %rsi, %r13 > - > - /* Align stack to 16 bytes when calling __gettimeofday. */ > - subq $24, %rsp > - cfi_adjust_cfa_offset(24) > - > - /* Get current time. */ > -2: movq %rsp, %rdi > - xorl %esi, %esi > - /* This call works because we directly jump to a system call entry > - which preserves all the registers. */ > - call JUMPTARGET(__gettimeofday) > - > - /* Compute relative timeout. */ > - movq 8(%rsp), %rax > - movl $1000, %edi > - mul %rdi /* Milli seconds to nano seconds. */ > - movq (%r13), %rdi > - movq 8(%r13), %rsi > - subq (%rsp), %rdi > - subq %rax, %rsi > - jns 5f > - addq $1000000000, %rsi > - decq %rdi > -5: testq %rdi, %rdi > - js 6f /* Time is already up. */ > - > - movq %rdi, (%rsp) /* Store relative timeout. */ > - movq %rsi, 8(%rsp) > - > - movl (%r12), %edx > - testl %edx, %edx > - jz 4f > - > - movq %rsp, %r10 > - /* XXX The kernel so far uses global futex for the wakeup at > - all times. */ > -#if FUTEX_WAIT == 0 > - xorl %esi, %esi > -#else > - movl $FUTEX_WAIT, %esi > -#endif > - movq %r12, %rdi > - movl $SYS_futex, %eax > - syscall > - > - cmpl $0, (%rdi) > - jne 1f > -4: xorl %eax, %eax > - > -8: addq $24, %rsp > - cfi_adjust_cfa_offset(-24) > - popq %r13 > - cfi_adjust_cfa_offset(-8) > - cfi_restore(%r13) > - popq %r12 > - cfi_adjust_cfa_offset(-8) > - cfi_restore(%r12) > - retq > - > - cfi_adjust_cfa_offset(32) > -1: cmpq $-ETIMEDOUT, %rax > - jne 2b > - > -6: movl $ETIMEDOUT, %eax > - jmp 8b > - cfi_endproc > - .size __lll_timedwait_tid,.-__lll_timedwait_tid OK. Amazing. More assembly deleted. > #endif > diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > index 47d4eabd61..8cbc1caa5b 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > @@ -222,31 +222,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count, > #define lll_islocked(futex) \ > (futex != LLL_LOCK_INITIALIZER) > > -extern int __lll_timedwait_tid (int *, const struct timespec *) > - attribute_hidden; > - > -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex > - wake-up when the clone terminates. The memory location contains the > - thread ID while the clone is running and is reset to zero by the kernel > - afterwards. The kernel up to version 3.16.3 does not use the private futex > - operations for futex wake-up when the clone terminates. > - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout > - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. > - The futex operation are issues with cancellable versions. */ > -#define lll_wait_tid(tid, abstime) \ > - ({ \ > - int __res = 0; \ > - __typeof (tid) __tid; \ > - if (abstime != NULL) \ > - __res = __lll_timedwait_tid (&(tid), (abstime)); \ > - else \ > - /* We need acquire MO here so that we synchronize with the \ > - kernel's store to 0 when the clone terminates. (see above) */ \ > - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ > - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ > - __res; \ > - }) > - OK. Unused. > extern int __lll_lock_elision (int *futex, short *adapt_count, int private) > attribute_hidden; > > -- Cheers, Carlos.
On 13/02/2019 12:09, Carlos O'Donell wrote: > On 2/12/19 3:18 PM, Adhemerval Zanella wrote: >> Patch ce7eb0e90315 ("nptl: Cleanup cancellation macros") changed the >> join sequence for internal common __pthread_timedjoin_ex to use the >> new macro lll_wait_tid. The idea was this macro would issue the >> cancellable futex operation depending whether the timeout is used or >> not. However if a timeout is used, __lll_timedwait_tid is called and >> it is not a cancellable entrypoint. > > Thanks for following up on this. > > OK for master with additional comment below (see Fix:). > > Once you commit I'll clean up the straggling comments that I fixed > before (pthread_tryjoin.c still has a wrong comment). Thanks for the review, I added the change locally: diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c index 3f9cf1674b..f719613268 100644 --- a/nptl/pthread_join_common.c +++ b/nptl/pthread_join_common.c @@ -120,6 +120,10 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, /* There is already somebody waiting for the thread. */ return EINVAL; + /* BLOCK waits either indefinitely or based on an absolute time. POSIX also + states a cancellation points shall occur for pthread_join, and we use the + same rationale for posix_timedjoin_np. Both timedwait_tid and the futex + call use the cancellable variant. */ if (block) { /* During the wait we change to asynchronous cancellation. If we
On Feb 14 2019, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c > index 3f9cf1674b..f719613268 100644 > --- a/nptl/pthread_join_common.c > +++ b/nptl/pthread_join_common.c > @@ -120,6 +120,10 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, > /* There is already somebody waiting for the thread. */ > return EINVAL; > > + /* BLOCK waits either indefinitely or based on an absolute time. POSIX also > + states a cancellation points shall occur for pthread_join, and we use the s/points/point/ Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
diff --git a/nptl/Makefile b/nptl/Makefile index 0e316edfac..e79ca8f064 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -120,7 +120,7 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \ pt-longjmp pt-cleanup\ cancellation \ lowlevellock \ - lll_timedlock_wait lll_timedwait_tid \ + lll_timedlock_wait \ pt-fork pt-vfork pt-fcntl \ $(pthread-compat-wrappers) \ pt-raise pt-system \ @@ -271,6 +271,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \ tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \ tst-raise1 \ tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \ + tst-join8 tst-join9 \ tst-detach1 \ tst-eintr1 tst-eintr2 tst-eintr3 tst-eintr4 tst-eintr5 \ tst-tsd1 tst-tsd2 tst-tsd3 tst-tsd4 tst-tsd5 tst-tsd6 \ diff --git a/nptl/lll_timedwait_tid.c b/nptl/lll_timedwait_tid.c deleted file mode 100644 index 59ecbb4446..0000000000 --- a/nptl/lll_timedwait_tid.c +++ /dev/null @@ -1,70 +0,0 @@ -/* Timed waiting for thread death. Generic futex-using version. - Copyright (C) 2003-2019 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - <http://www.gnu.org/licenses/>. */ - -#include <atomic.h> -#include <errno.h> -#include <lowlevellock.h> -#include <sys/time.h> - - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. */ -int -__lll_timedwait_tid (int *tidp, const struct timespec *abstime) -{ - int tid; - - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) - return EINVAL; - - /* Repeat until thread terminated. */ - while ((tid = *tidp) != 0) - { - struct timeval tv; - struct timespec rt; - - /* Get the current time. */ - (void) __gettimeofday (&tv, NULL); - - /* Compute relative timeout. */ - rt.tv_sec = abstime->tv_sec - tv.tv_sec; - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; - if (rt.tv_nsec < 0) - { - rt.tv_nsec += 1000000000; - --rt.tv_sec; - } - - /* Already timed out? */ - if (rt.tv_sec < 0) - return ETIMEDOUT; - - /* If *tidp == tid, wait until thread terminates or the wait times out. - The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - */ - if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) - return ETIMEDOUT; - } - - return 0; -} diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c index ecb78ffba5..3f9cf1674b 100644 --- a/nptl/pthread_join_common.c +++ b/nptl/pthread_join_common.c @@ -30,6 +30,52 @@ cleanup (void *arg) atomic_compare_exchange_weak_acquire (&arg, &self, NULL); } +/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex + wake-up when the clone terminates. The memory location contains the + thread ID while the clone is running and is reset to zero by the kernel + afterwards. The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. */ +static int +timedwait_tid (pid_t *tidp, const struct timespec *abstime) +{ + pid_t tid; + + if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) + return EINVAL; + + /* Repeat until thread terminated. */ + while ((tid = *tidp) != 0) + { + struct timeval tv; + struct timespec rt; + + /* Get the current time. */ + __gettimeofday (&tv, NULL); + + /* Compute relative timeout. */ + rt.tv_sec = abstime->tv_sec - tv.tv_sec; + rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; + if (rt.tv_nsec < 0) + { + rt.tv_nsec += 1000000000; + --rt.tv_sec; + } + + /* Already timed out? */ + if (rt.tv_sec < 0) + return ETIMEDOUT; + + /* If *tidp == tid, wait until thread terminates or the wait times out. + The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. */ + if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED) + == -ETIMEDOUT) + return ETIMEDOUT; + } + + return 0; +} + int __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, const struct timespec *abstime, bool block) @@ -81,7 +127,16 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return, un-wait-ed for again. */ pthread_cleanup_push (cleanup, &pd->joinid); - result = lll_wait_tid (pd->tid, abstime); + if (abstime != NULL) + result = timedwait_tid (&pd->tid, abstime); + else + { + pid_t tid; + /* We need acquire MO here so that we synchronize with the + kernel's store to 0 when the clone terminates. (see above) */ + while ((tid = atomic_load_acquire (&pd->tid)) != 0) + lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED); + } pthread_cleanup_pop (0); } diff --git a/nptl/tst-join5.c b/nptl/tst-join5.c index f60a024baa..39f74dce09 100644 --- a/nptl/tst-join5.c +++ b/nptl/tst-join5.c @@ -23,6 +23,8 @@ #include <time.h> #include <unistd.h> +#include <support/check.h> +#include <support/xthread.h> static void wait_code (void) @@ -37,22 +39,31 @@ wait_code (void) static pthread_barrier_t b; #endif +static int +thread_join (pthread_t thread, void **retval) +{ +#ifdef USE_PTHREAD_TIMEDJOIN_NP + struct timespec tv; + TEST_COMPARE (clock_gettime (CLOCK_REALTIME, &tv), 0); + /* Arbitrary large timeout to make it act as pthread_join. */ + tv.tv_sec += 1000; + return pthread_timedjoin_np ((pthread_t) thread, retval, &tv); +#else + return pthread_join ((pthread_t) thread, retval); +#endif +} + static void * tf1 (void *arg) { #ifdef WAIT_IN_CHILD - int e = pthread_barrier_wait (&b); - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) - { - printf ("%s: barrier_wait failed\n", __func__); - exit (1); - } + xpthread_barrier_wait (&b); wait_code (); #endif - pthread_join ((pthread_t) arg, NULL); + thread_join ((pthread_t) arg, NULL); exit (42); } @@ -62,16 +73,12 @@ static void * tf2 (void *arg) { #ifdef WAIT_IN_CHILD - int e = pthread_barrier_wait (&b); - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) - { - printf ("%s: barrier_wait failed\n", __func__); - exit (1); - } + xpthread_barrier_wait (&b); wait_code (); #endif - pthread_join ((pthread_t) arg, NULL); + + thread_join ((pthread_t) arg, NULL); exit (43); } @@ -81,16 +88,12 @@ static int do_test (void) { #ifdef WAIT_IN_CHILD - if (pthread_barrier_init (&b, NULL, 2) != 0) - { - puts ("barrier_init failed"); - return 1; - } + xpthread_barrier_init (&b, NULL, 2); #endif pthread_t th; - int err = pthread_join (pthread_self (), NULL); + int err = thread_join (pthread_self (), NULL); if (err == 0) { puts ("1st circular join succeeded"); @@ -102,33 +105,20 @@ do_test (void) return 1; } - if (pthread_create (&th, NULL, tf1, (void *) pthread_self ()) != 0) - { - puts ("1st create failed"); - return 1; - } + th = xpthread_create (NULL, tf1, (void *) pthread_self ()); #ifndef WAIT_IN_CHILD wait_code (); #endif - if (pthread_cancel (th) != 0) - { - puts ("cannot cancel 1st thread"); - return 1; - } + xpthread_cancel (th); #ifdef WAIT_IN_CHILD - int e = pthread_barrier_wait (&b); - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) - { - printf ("%s: barrier_wait failed\n", __func__); - return 1; - } + xpthread_barrier_wait (&b); #endif void *r; - err = pthread_join (th, &r); + err = thread_join (th, &r); if (err != 0) { printf ("cannot join 1st thread: %d\n", err); @@ -140,7 +130,7 @@ do_test (void) return 1; } - err = pthread_join (pthread_self (), NULL); + err = thread_join (pthread_self (), NULL); if (err == 0) { puts ("2nd circular join succeeded"); @@ -152,32 +142,19 @@ do_test (void) return 1; } - if (pthread_create (&th, NULL, tf2, (void *) pthread_self ()) != 0) - { - puts ("2nd create failed"); - return 1; - } + th = xpthread_create (NULL, tf2, (void *) pthread_self ()); #ifndef WAIT_IN_CHILD wait_code (); #endif - if (pthread_cancel (th) != 0) - { - puts ("cannot cancel 2nd thread"); - return 1; - } + xpthread_cancel (th); #ifdef WAIT_IN_CHILD - e = pthread_barrier_wait (&b); - if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) - { - printf ("%s: barrier_wait failed\n", __func__); - return 1; - } + xpthread_barrier_wait (&b); #endif - if (pthread_join (th, &r) != 0) + if (thread_join (th, &r) != 0) { puts ("cannot join 2nd thread"); return 1; @@ -188,7 +165,7 @@ do_test (void) return 1; } - err = pthread_join (pthread_self (), NULL); + err = thread_join (pthread_self (), NULL); if (err == 0) { puts ("3rd circular join succeeded"); @@ -203,5 +180,4 @@ do_test (void) return 0; } -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" +#include <support/test-driver.c> diff --git a/nptl/tst-join8.c b/nptl/tst-join8.c new file mode 100644 index 0000000000..f3241fae1c --- /dev/null +++ b/nptl/tst-join8.c @@ -0,0 +1,20 @@ +/* Check if pthread_timedjoin_np is a cancellation entrypoint. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#define USE_PTHREAD_TIMEDJOIN_NP 1 +#include <nptl/tst-join5.c> diff --git a/nptl/tst-join9.c b/nptl/tst-join9.c new file mode 100644 index 0000000000..7a47f582f1 --- /dev/null +++ b/nptl/tst-join9.c @@ -0,0 +1,21 @@ +/* Check if pthread_timedjoin_np is a cancellation entrypoint. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#define USE_PTHREAD_TIMEDJOIN_NP 1 +#define WAIT_IN_CHILD 1 +#include <nptl/tst-join5.c> diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h index 50ceba71ff..bb8effe288 100644 --- a/sysdeps/nptl/lowlevellock-futex.h +++ b/sysdeps/nptl/lowlevellock-futex.h @@ -82,5 +82,12 @@ val, private) \ -ENOSYS +/* Like lll_futex_wait, but acting as a cancellable entrypoint. */ +#define lll_futex_wait_cancel(futexp, val, private) \ + -ENOSYS + +/* Like lll_futex_timed_wait, but acting as a cancellable entrypoint. */ +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \ + -ENOSYS #endif /* lowlevellock-futex.h */ diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h index 36bbc472e7..6f017afdd5 100644 --- a/sysdeps/nptl/lowlevellock.h +++ b/sysdeps/nptl/lowlevellock.h @@ -175,30 +175,4 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, #define LLL_LOCK_INITIALIZER (0) #define LLL_LOCK_INITIALIZER_LOCKED (1) -extern int __lll_timedwait_tid (int *, const struct timespec *) - attribute_hidden; - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. - The futex operation are issues with cancellable versions. */ -#define lll_wait_tid(tid, abstime) \ - ({ \ - int __res = 0; \ - __typeof (tid) __tid; \ - if (abstime != NULL) \ - __res = __lll_timedwait_tid (&(tid), (abstime)); \ - else \ - /* We need acquire MO here so that we synchronize with the \ - kernel's store to 0 when the clone terminates. (see above) */ \ - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ - __res; \ - }) - - #endif /* lowlevellock.h */ diff --git a/sysdeps/sparc/sparc32/lll_timedwait_tid.c b/sysdeps/sparc/sparc32/lll_timedwait_tid.c deleted file mode 100644 index 511608ead9..0000000000 --- a/sysdeps/sparc/sparc32/lll_timedwait_tid.c +++ /dev/null @@ -1 +0,0 @@ -/* __lll_timedwait_tid is in lowlevellock.c. */ diff --git a/sysdeps/sparc/sparc32/lowlevellock.c b/sysdeps/sparc/sparc32/lowlevellock.c index a6b2961bd3..1a0b7bbe00 100644 --- a/sysdeps/sparc/sparc32/lowlevellock.c +++ b/sysdeps/sparc/sparc32/lowlevellock.c @@ -88,44 +88,4 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) return 0; } - - -int -__lll_timedwait_tid (int *tidp, const struct timespec *abstime) -{ - int tid; - - if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000) - return EINVAL; - - /* Repeat until thread terminated. */ - while ((tid = *tidp) != 0) - { - struct timeval tv; - struct timespec rt; - - /* Get the current time. */ - (void) __gettimeofday (&tv, NULL); - - /* Compute relative timeout. */ - rt.tv_sec = abstime->tv_sec - tv.tv_sec; - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000; - if (rt.tv_nsec < 0) - { - rt.tv_nsec += 1000000000; - --rt.tv_sec; - } - - /* Already timed out? */ - if (rt.tv_sec < 0) - return ETIMEDOUT; - - /* Wait until thread terminates. The kernel so far does not use - the private futex operations for this. */ - if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) - return ETIMEDOUT; - } - - return 0; -} #endif diff --git a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c deleted file mode 100644 index 43900c6294..0000000000 --- a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c +++ /dev/null @@ -1 +0,0 @@ -/* __lll_timedwait_tid is in lowlevellock.S. */ diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.S b/sysdeps/unix/sysv/linux/i386/lowlevellock.S index b5f5b5b11c..83191a3899 100644 --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.S +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.S @@ -365,70 +365,4 @@ __lll_unlock_wake: ret cfi_endproc .size __lll_unlock_wake,.-__lll_unlock_wake - - .globl __lll_timedwait_tid - .type __lll_timedwait_tid,@function - .hidden __lll_timedwait_tid - .align 16 -__lll_timedwait_tid: - pushl %edi - pushl %esi - pushl %ebx - pushl %ebp - - movl %eax, %ebp - movl %edx, %edi - subl $8, %esp - - /* Get current time. */ -2: movl %esp, %ebx - xorl %ecx, %ecx - movl $__NR_gettimeofday, %eax - ENTER_KERNEL - - /* Compute relative timeout. */ - movl 4(%esp), %eax - movl $1000, %edx - mul %edx /* Milli seconds to nano seconds. */ - movl (%edi), %ecx - movl 4(%edi), %edx - subl (%esp), %ecx - subl %eax, %edx - jns 5f - addl $1000000000, %edx - subl $1, %ecx -5: testl %ecx, %ecx - js 6f /* Time is already up. */ - - movl %ecx, (%esp) /* Store relative timeout. */ - movl %edx, 4(%esp) - - movl (%ebp), %edx - testl %edx, %edx - jz 4f - - movl %esp, %esi - /* XXX The kernel so far uses global futex for the wakeup at - all times. */ - xorl %ecx, %ecx /* movl $FUTEX_WAIT, %ecx */ - movl %ebp, %ebx - movl $SYS_futex, %eax - ENTER_KERNEL - - cmpl $0, (%ebx) - jne 1f -4: xorl %eax, %eax - -3: addl $8, %esp - popl %ebp - popl %ebx - popl %esi - popl %edi - ret - -1: cmpl $-ETIMEDOUT, %eax - jne 2b -6: movl $ETIMEDOUT, %eax - jmp 3b - .size __lll_timedwait_tid,.-__lll_timedwait_tid #endif diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h index aa8f4bfdf6..94dccc4ce7 100644 --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h @@ -219,31 +219,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count, #define lll_islocked(futex) \ (futex != LLL_LOCK_INITIALIZER) -extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime) - __attribute__ ((regparm (2))) attribute_hidden; - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. - The futex operation are issues with cancellable versions. */ -#define lll_wait_tid(tid, abstime) \ - ({ \ - int __res = 0; \ - __typeof (tid) __tid; \ - if (abstime != NULL) \ - __res = __lll_timedwait_tid (&(tid), (abstime)); \ - else \ - /* We need acquire MO here so that we synchronize with the \ - kernel's store to 0 when the clone terminates. (see above) */ \ - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ - __res; \ - }) - extern int __lll_lock_elision (int *futex, short *adapt_count, int private) attribute_hidden; diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h index 8ca3417aaa..6f060b1739 100644 --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h @@ -135,6 +135,13 @@ __err; \ }) +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \ + ({ \ + int __oldtype = CANCEL_ASYNC (); \ + long int __err = lll_futex_timed_wait (futexp, val, timeout, private); \ + CANCEL_RESET (__oldtype); \ + __err; \ + }) #endif /* !__ASSEMBLER__ */ diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h index 5aed572d43..c778f4259b 100644 --- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h @@ -108,29 +108,4 @@ __lll_timedlock (int *futex, const struct timespec *abstime, int private) #define LLL_LOCK_INITIALIZER (0) #define LLL_LOCK_INITIALIZER_LOCKED (1) -extern int __lll_timedwait_tid (int *, const struct timespec *) - attribute_hidden; - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. - The futex operation are issues with cancellable versions. */ -#define lll_wait_tid(tid, abstime) \ - ({ \ - int __res = 0; \ - __typeof (tid) __tid; \ - if (abstime != NULL) \ - __res = __lll_timedwait_tid (&(tid), (abstime)); \ - else \ - /* We need acquire MO here so that we synchronize with the \ - kernel's store to 0 when the clone terminates. (see above) */ \ - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ - __res; \ - }) - #endif /* lowlevellock.h */ diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c deleted file mode 100644 index 43900c6294..0000000000 --- a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c +++ /dev/null @@ -1 +0,0 @@ -/* __lll_timedwait_tid is in lowlevellock.S. */ diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S index f9abb4cb1f..166dbcfd3b 100644 --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S @@ -345,87 +345,4 @@ __lll_unlock_wake: retq cfi_endproc .size __lll_unlock_wake,.-__lll_unlock_wake - - .globl __lll_timedwait_tid - .type __lll_timedwait_tid,@function - .hidden __lll_timedwait_tid - .align 16 -__lll_timedwait_tid: - cfi_startproc - pushq %r12 - cfi_adjust_cfa_offset(8) - pushq %r13 - cfi_adjust_cfa_offset(8) - cfi_offset(%r12, -16) - cfi_offset(%r13, -24) - - movq %rdi, %r12 - movq %rsi, %r13 - - /* Align stack to 16 bytes when calling __gettimeofday. */ - subq $24, %rsp - cfi_adjust_cfa_offset(24) - - /* Get current time. */ -2: movq %rsp, %rdi - xorl %esi, %esi - /* This call works because we directly jump to a system call entry - which preserves all the registers. */ - call JUMPTARGET(__gettimeofday) - - /* Compute relative timeout. */ - movq 8(%rsp), %rax - movl $1000, %edi - mul %rdi /* Milli seconds to nano seconds. */ - movq (%r13), %rdi - movq 8(%r13), %rsi - subq (%rsp), %rdi - subq %rax, %rsi - jns 5f - addq $1000000000, %rsi - decq %rdi -5: testq %rdi, %rdi - js 6f /* Time is already up. */ - - movq %rdi, (%rsp) /* Store relative timeout. */ - movq %rsi, 8(%rsp) - - movl (%r12), %edx - testl %edx, %edx - jz 4f - - movq %rsp, %r10 - /* XXX The kernel so far uses global futex for the wakeup at - all times. */ -#if FUTEX_WAIT == 0 - xorl %esi, %esi -#else - movl $FUTEX_WAIT, %esi -#endif - movq %r12, %rdi - movl $SYS_futex, %eax - syscall - - cmpl $0, (%rdi) - jne 1f -4: xorl %eax, %eax - -8: addq $24, %rsp - cfi_adjust_cfa_offset(-24) - popq %r13 - cfi_adjust_cfa_offset(-8) - cfi_restore(%r13) - popq %r12 - cfi_adjust_cfa_offset(-8) - cfi_restore(%r12) - retq - - cfi_adjust_cfa_offset(32) -1: cmpq $-ETIMEDOUT, %rax - jne 2b - -6: movl $ETIMEDOUT, %eax - jmp 8b - cfi_endproc - .size __lll_timedwait_tid,.-__lll_timedwait_tid #endif diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h index 47d4eabd61..8cbc1caa5b 100644 --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h @@ -222,31 +222,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count, #define lll_islocked(futex) \ (futex != LLL_LOCK_INITIALIZER) -extern int __lll_timedwait_tid (int *, const struct timespec *) - attribute_hidden; - -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wake-up when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero by the kernel - afterwards. The kernel up to version 3.16.3 does not use the private futex - operations for futex wake-up when the clone terminates. - If ABSTIME is not NULL, is used a timeout for futex call. If the timeout - occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL. - The futex operation are issues with cancellable versions. */ -#define lll_wait_tid(tid, abstime) \ - ({ \ - int __res = 0; \ - __typeof (tid) __tid; \ - if (abstime != NULL) \ - __res = __lll_timedwait_tid (&(tid), (abstime)); \ - else \ - /* We need acquire MO here so that we synchronize with the \ - kernel's store to 0 when the clone terminates. (see above) */ \ - while ((__tid = atomic_load_acquire (&(tid))) != 0) \ - lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED); \ - __res; \ - }) - extern int __lll_lock_elision (int *futex, short *adapt_count, int private) attribute_hidden;