| Message ID | 1513690827-11941-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 4735850f7a4f07f05ee9ff405238332f4bb3ab50 |
| Headers | show |
| Series | nptl: Consolidate pthread_{timed,try}join{_np} | expand |
On 12/19/2017 05:40 AM, Adhemerval Zanella wrote: > This patch consolidates the pthread_join and gnu extensions to avoid > code duplication. The function pthread_join, pthread_tryjoin_np, and > pthread_timedjoin_np are now based on pthread_timedjoin_ex. > > It also fixes some inconsistencies on ESRCH, EINVAL, EDEADLK handling > (where each implementation differs from each other) and also on > clenup handler (which now always use a CAS). High level: Refactoring common code like this is a good idea. Design: Moving everything into __pthread_timedjoin_ex and using a bool for blocking seems like a good refactoring, anything less would duplicate code again. I like the removal of the EINVAL checks in the lowlevellock.h code which harmonizes this more with all the arches. Implementation: We have started using pthread_cond_common.c, pthread_rwlock_common.c, and I'd like to see us use pthread_join_common.c for the common file with all others including it. This might sound like bike-shedding, but it helps one quickly determine where the common code lies. New developers will thank us for the logical choices like this. > Checked on i686-linux-gnu and x86_64-linux-gnu. > > * nptl/pthreadP.h (__pthread_timedjoin_np): Define. > * nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_np. > * nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise. > * nptl/pthread_timedjoin.c (cleanup): Use CAS on argument setting. > (pthread_timedjoin_np): Define internal symbol and common code from > pthread_join. > * sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid): > Remove superflous checks. > * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid): > Likewise. OK with rework of pthread_join_common.c. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > ChangeLog | 13 ++++ > nptl/pthreadP.h | 3 + > nptl/pthread_join.c | 94 +-------------------------- > nptl/pthread_timedjoin.c | 80 +++++++++++++++-------- > nptl/pthread_tryjoin.c | 43 ++---------- > sysdeps/unix/sysv/linux/i386/lowlevellock.h | 7 +- > sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 7 +- > 7 files changed, 75 insertions(+), 172 deletions(-) > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 713000e..2646b8f 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype); > extern int __pthread_enable_asynccancel (void) attribute_hidden; > extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden; > extern void __pthread_testcancel (void); > +extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *, > + bool); OK. > > #if IS_IN (libpthread) > hidden_proto (__pthread_mutex_init) > @@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate) > hidden_proto (__pthread_testcancel) > hidden_proto (__pthread_mutexattr_init) > hidden_proto (__pthread_mutexattr_settype) > +hidden_proto (__pthread_timedjoin_ex) OK. > #endif > > extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond); > diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c > index afc8c37..47379a6 100644 > --- a/nptl/pthread_join.c > +++ b/nptl/pthread_join.c > @@ -16,103 +16,11 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include <errno.h> > -#include <stdlib.h> > - > -#include <atomic.h> OK. > #include "pthreadP.h" > > -#include <stap-probe.h> > - > - > -static void > -cleanup (void *arg) > -{ > - /* If we already changed the waiter ID, reset it. The call cannot > - fail for any reason but the thread not having done that yet so > - there is no reason for a loop. */ > - (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL, > - THREAD_SELF); > -} > - > - > int > __pthread_join (pthread_t threadid, void **thread_return) > { > - struct pthread *pd = (struct pthread *) threadid; > - > - /* Make sure the descriptor is valid. */ > - if (INVALID_NOT_TERMINATED_TD_P (pd)) > - /* Not a valid thread handle. */ > - return ESRCH; > - > - /* Is the thread joinable?. */ > - if (IS_DETACHED (pd)) > - /* We cannot wait for the thread. */ > - return EINVAL; > - > - struct pthread *self = THREAD_SELF; > - int result = 0; > - > - LIBC_PROBE (pthread_join, 1, threadid); > - > - /* During the wait we change to asynchronous cancellation. If we > - are canceled the thread we are waiting for must be marked as > - un-wait-ed for again. */ > - pthread_cleanup_push (cleanup, &pd->joinid); > - > - /* Switch to asynchronous cancellation. */ > - int oldtype = CANCEL_ASYNC (); > - > - if ((pd == self > - || (self->joinid == pd > - && (pd->cancelhandling > - & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK > - | TERMINATED_BITMASK)) == 0)) > - && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) > - /* This is a deadlock situation. The threads are waiting for each > - other to finish. Note that this is a "may" error. To be 100% > - sure we catch this error we would have to lock the data > - structures but it is not necessary. In the unlikely case that > - two threads are really caught in this situation they will > - deadlock. It is the programmer's problem to figure this > - out. */ > - result = EDEADLK; > - /* Wait for the thread to finish. If it is already locked something > - is wrong. There can only be one waiter. */ > - else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, > - self, > - NULL), 0)) > - /* There is already somebody waiting for the thread. */ > - result = EINVAL; > - else > - /* Wait for the child. */ > - lll_wait_tid (pd->tid); > - > - > - /* Restore cancellation mode. */ > - CANCEL_RESET (oldtype); > - > - /* Remove the handler. */ > - pthread_cleanup_pop (0); > - > - > - if (__glibc_likely (result == 0)) > - { > - /* We mark the thread as terminated and as joined. */ > - pd->tid = -1; > - > - /* Store the return value if the caller is interested. */ > - if (thread_return != NULL) > - *thread_return = pd->result; > - > - > - /* Free the TCB. */ > - __free_tcb (pd); > - } > - > - LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); > - > - return result; > + return __pthread_timedjoin_ex (threadid, thread_return, NULL, true); OK. > } > weak_alias (__pthread_join, pthread_join) > diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c > index 567c171..6b894f2 100644 > --- a/nptl/pthread_timedjoin.c > +++ b/nptl/pthread_timedjoin.c > @@ -21,21 +21,25 @@ > #include <atomic.h> > #include "pthreadP.h" > > +#include <stap-probe.h> > + > > static void > cleanup (void *arg) > { > - *(void **) arg = NULL; > + /* If we already changed the waiter ID, reset it. The call cannot > + fail for any reason but the thread not having done that yet so > + there is no reason for a loop. */ > + struct pthread *self = THREAD_SELF; > + atomic_compare_exchange_weak_acquire (&arg, &self, NULL); > } OK. > > > int > -pthread_timedjoin_np (pthread_t threadid, void **thread_return, > - const struct timespec *abstime) > +__pthread_timedjoin_ex (pthread_t threadid, void **thread_return, > + const struct timespec *abstime, bool block) > { > - struct pthread *self; > struct pthread *pd = (struct pthread *) threadid; > - int result; > > /* Make sure the descriptor is valid. */ > if (INVALID_NOT_TERMINATED_TD_P (pd)) > @@ -47,8 +51,17 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return, > /* We cannot wait for the thread. */ > return EINVAL; > > - self = THREAD_SELF; > - if (pd == self || self->joinid == pd) > + struct pthread *self = THREAD_SELF; > + int result = 0; > + > + LIBC_PROBE (pthread_join, 1, threadid); OK. > + > + if ((pd == self > + || (self->joinid == pd > + && (pd->cancelhandling > + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK > + | TERMINATED_BITMASK)) == 0)) > + && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) > /* This is a deadlock situation. The threads are waiting for each > other to finish. Note that this is a "may" error. To be 100% > sure we catch this error we would have to lock the data > @@ -60,45 +73,56 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return, > > /* Wait for the thread to finish. If it is already locked something > is wrong. There can only be one waiter. */ > - if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, > - self, NULL), 0)) > + else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid, > + &self, > + NULL))) > /* There is already somebody waiting for the thread. */ > return EINVAL; > > + if (block) > + { > + /* During the wait we change to asynchronous cancellation. If we > + are cancelled the thread we are waiting for must be marked as > + un-wait-ed for again. */ > + pthread_cleanup_push (cleanup, &pd->joinid); > > - /* During the wait we change to asynchronous cancellation. If we > - are cancelled the thread we are waiting for must be marked as > - un-wait-ed for again. */ > - pthread_cleanup_push (cleanup, &pd->joinid); > - > - /* Switch to asynchronous cancellation. */ > - int oldtype = CANCEL_ASYNC (); > - > - > - /* Wait for the child. */ > - result = lll_timedwait_tid (pd->tid, abstime); > - > + int oldtype = CANCEL_ASYNC (); OK. > > - /* Restore cancellation mode. */ > - CANCEL_RESET (oldtype); > + if (abstime != NULL) > + result = lll_timedwait_tid (pd->tid, abstime); > + else > + lll_wait_tid (pd->tid); > > - /* Remove the handler. */ > - pthread_cleanup_pop (0); > + CANCEL_RESET (oldtype); > > + pthread_cleanup_pop (0); > + } > > - /* We might have timed out. */ > - if (result == 0) > + if (__glibc_likely (result == 0)) > { > + /* We mark the thread as terminated and as joined. */ > + pd->tid = -1; OK. > + > /* Store the return value if the caller is interested. */ > if (thread_return != NULL) > *thread_return = pd->result; > > - > /* Free the TCB. */ > __free_tcb (pd); > } > else > pd->joinid = NULL; > > + LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); > + > return result; > } > +hidden_def (__pthread_timedjoin_ex) > + > +int > +__pthread_timedjoin_np (pthread_t threadid, void **thread_return, > + const struct timespec *abstime) > +{ > + return __pthread_timedjoin_ex (threadid, thread_return, abstime, true); > +} OK. > +weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np) > diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c > index 6a3b62e..e7c7595 100644 > --- a/nptl/pthread_tryjoin.c > +++ b/nptl/pthread_tryjoin.c > @@ -26,47 +26,12 @@ > int > pthread_tryjoin_np (pthread_t threadid, void **thread_return) > { > - struct pthread *self; > - struct pthread *pd = (struct pthread *) threadid; > - > - /* Make sure the descriptor is valid. */ > - if (DEBUGGING_P && __find_in_stack_list (pd) == NULL) > - /* Not a valid thread handle. */ > - return ESRCH; > - > - /* Is the thread joinable?. */ > - if (IS_DETACHED (pd)) > - /* We cannot wait for the thread. */ > - return EINVAL; > - > - self = THREAD_SELF; > - if (pd == self || self->joinid == pd) > - /* This is a deadlock situation. The threads are waiting for each > - other to finish. Note that this is a "may" error. To be 100% > - sure we catch this error we would have to lock the data > - structures but it is not necessary. In the unlikely case that > - two threads are really caught in this situation they will > - deadlock. It is the programmer's problem to figure this > - out. */ > - return EDEADLK; > - > /* Return right away if the thread hasn't terminated yet. */ > + struct pthread *pd = (struct pthread *) threadid; > if (pd->tid != 0) > return EBUSY; > OK. > - /* Wait for the thread to finish. If it is already locked something > - is wrong. There can only be one waiter. */ > - if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL)) > - /* There is already somebody waiting for the thread. */ > - return EINVAL; > - > - /* Store the return value if the caller is interested. */ > - if (thread_return != NULL) > - *thread_return = pd->result; > - > - > - /* Free the TCB. */ > - __free_tcb (pd); > - > - return 0; > + /* If pd->tid != 0 then lll_wait_tid will not block on futex > + operation. */ > + return __pthread_timedjoin_ex (threadid, thread_return, NULL, false); OK. > } > diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h > index 197bb1f..098c64b 100644 > --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h > +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h > @@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime) > ({ \ > int __result = 0; \ > if ((tid) != 0) \ > - { \ > - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ > - __result = EINVAL; \ > - else \ > - __result = __lll_timedwait_tid (&(tid), (abstime)); \ > - } \ > + __result = __lll_timedwait_tid (&(tid), (abstime)); \ OK. Don't handle EINVAL in any special way and harmonize in the common code. > __result; }) > > > diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > index cbf6597..0b56a2e 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h > @@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *) > ({ \ > int __result = 0; \ > if ((tid) != 0) \ > - { \ > - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ > - __result = EINVAL; \ > - else \ > - __result = __lll_timedwait_tid (&(tid), (abstime)); \ > - } \ > + __result = __lll_timedwait_tid (&(tid), (abstime)); \ OK. > __result; }) > > extern int __lll_lock_elision (int *futex, short *adapt_count, int private) > -- Cheers, Carlos.
On 20/12/2017 03:44, Carlos O'Donell wrote: > On 12/19/2017 05:40 AM, Adhemerval Zanella wrote: >> This patch consolidates the pthread_join and gnu extensions to avoid >> code duplication. The function pthread_join, pthread_tryjoin_np, and >> pthread_timedjoin_np are now based on pthread_timedjoin_ex. >> >> It also fixes some inconsistencies on ESRCH, EINVAL, EDEADLK handling >> (where each implementation differs from each other) and also on >> clenup handler (which now always use a CAS). > > High level: > > Refactoring common code like this is a good idea. > > Design: > > Moving everything into __pthread_timedjoin_ex and using a bool for > blocking seems like a good refactoring, anything less would duplicate > code again. > > I like the removal of the EINVAL checks in the lowlevellock.h code > which harmonizes this more with all the arches. > > Implementation: > > We have started using pthread_cond_common.c, pthread_rwlock_common.c, > and I'd like to see us use pthread_join_common.c for the common file > with all others including it. This might sound like bike-shedding, but > it helps one quickly determine where the common code lies. New developers > will thank us for the logical choices like this. > >> Checked on i686-linux-gnu and x86_64-linux-gnu. >> >> * nptl/pthreadP.h (__pthread_timedjoin_np): Define. >> * nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_np. >> * nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise. >> * nptl/pthread_timedjoin.c (cleanup): Use CAS on argument setting. >> (pthread_timedjoin_np): Define internal symbol and common code from >> pthread_join. >> * sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid): >> Remove superflous checks. >> * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid): >> Likewise. > > OK with rework of pthread_join_common.c. Thanks for the reviewed, I pushed the following patch (with the pthread_join_common.c change you requested): diff --git a/ChangeLog b/ChangeLog index ad2c9db..35d1d10 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2017-12-20 Adhemerval Zanella <adhemerval.zanella@linaro.org> + + * nptl/Makefile (libpthread-routines): Add pthread_join_common. + * nptl/pthreadP.h (__pthread_timedjoin_ex): New prototype. + * nptl/pthread_join_common.c: New file: common function used on + pthread_join, pthread_timedjoin_np, pthread_tryjoin_np. + * nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_ex. + * nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise. + * nptl/pthread_timedjoin.c (pthread_timedjoin_np): Likewise. + (cleanup): Move definition to pthread_join_common.c. + * sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid): + Remove superflous checks. + * sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid): + Likewise. + 2017-12-20 Szabolcs Nagy <szabolcs.nagy@arm.com> * sysdeps/aarch64/libm-test-ulps: Update. diff --git a/nptl/Makefile b/nptl/Makefile index ab8cc98..cf2ba81 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -49,6 +49,7 @@ pthread-compat-wrappers = \ libpthread-routines = nptl-init vars events version pt-interp \ pthread_create pthread_exit pthread_detach \ pthread_join pthread_tryjoin pthread_timedjoin \ + pthread_join_common \ compat-pthread_self pthread_equal pthread_yield \ pthread_getconcurrency pthread_setconcurrency \ pthread_getschedparam pthread_setschedparam \ diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 713000e..2646b8f 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype); extern int __pthread_enable_asynccancel (void) attribute_hidden; extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden; extern void __pthread_testcancel (void); +extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *, + bool); #if IS_IN (libpthread) hidden_proto (__pthread_mutex_init) @@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate) hidden_proto (__pthread_testcancel) hidden_proto (__pthread_mutexattr_init) hidden_proto (__pthread_mutexattr_settype) +hidden_proto (__pthread_timedjoin_ex) #endif extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond); diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c index afc8c37..47379a6 100644 --- a/nptl/pthread_join.c +++ b/nptl/pthread_join.c @@ -16,103 +16,11 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <errno.h> -#include <stdlib.h> - -#include <atomic.h> #include "pthreadP.h" -#include <stap-probe.h> - - -static void -cleanup (void *arg) -{ - /* If we already changed the waiter ID, reset it. The call cannot - fail for any reason but the thread not having done that yet so - there is no reason for a loop. */ - (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL, - THREAD_SELF); -} - - int __pthread_join (pthread_t threadid, void **thread_return) { - struct pthread *pd = (struct pthread *) threadid; - - /* Make sure the descriptor is valid. */ - if (INVALID_NOT_TERMINATED_TD_P (pd)) - /* Not a valid thread handle. */ - return ESRCH; - - /* Is the thread joinable?. */ - if (IS_DETACHED (pd)) - /* We cannot wait for the thread. */ - return EINVAL; - - struct pthread *self = THREAD_SELF; - int result = 0; - - LIBC_PROBE (pthread_join, 1, threadid); - - /* During the wait we change to asynchronous cancellation. If we - are canceled the thread we are waiting for must be marked as - un-wait-ed for again. */ - pthread_cleanup_push (cleanup, &pd->joinid); - - /* Switch to asynchronous cancellation. */ - int oldtype = CANCEL_ASYNC (); - - if ((pd == self - || (self->joinid == pd - && (pd->cancelhandling - & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK - | TERMINATED_BITMASK)) == 0)) - && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) - /* This is a deadlock situation. The threads are waiting for each - other to finish. Note that this is a "may" error. To be 100% - sure we catch this error we would have to lock the data - structures but it is not necessary. In the unlikely case that - two threads are really caught in this situation they will - deadlock. It is the programmer's problem to figure this - out. */ - result = EDEADLK; - /* Wait for the thread to finish. If it is already locked something - is wrong. There can only be one waiter. */ - else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, - self, - NULL), 0)) - /* There is already somebody waiting for the thread. */ - result = EINVAL; - else - /* Wait for the child. */ - lll_wait_tid (pd->tid); - - - /* Restore cancellation mode. */ - CANCEL_RESET (oldtype); - - /* Remove the handler. */ - pthread_cleanup_pop (0); - - - if (__glibc_likely (result == 0)) - { - /* We mark the thread as terminated and as joined. */ - pd->tid = -1; - - /* Store the return value if the caller is interested. */ - if (thread_return != NULL) - *thread_return = pd->result; - - - /* Free the TCB. */ - __free_tcb (pd); - } - - LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); - - return result; + return __pthread_timedjoin_ex (threadid, thread_return, NULL, true); } weak_alias (__pthread_join, pthread_join) diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c new file mode 100644 index 0000000..59c5445 --- /dev/null +++ b/nptl/pthread_join_common.c @@ -0,0 +1,115 @@ +/* Common definition for pthread_{timed,try}join{_np}. + Copyright (C) 2017 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/>. */ + +#include "pthreadP.h" +#include <atomic.h> +#include <stap-probe.h> + +static void +cleanup (void *arg) +{ + /* If we already changed the waiter ID, reset it. The call cannot + fail for any reason but the thread not having done that yet so + there is no reason for a loop. */ + struct pthread *self = THREAD_SELF; + atomic_compare_exchange_weak_acquire (&arg, &self, NULL); +} + +int +__pthread_timedjoin_ex (pthread_t threadid, void **thread_return, + const struct timespec *abstime, bool block) +{ + struct pthread *pd = (struct pthread *) threadid; + + /* Make sure the descriptor is valid. */ + if (INVALID_NOT_TERMINATED_TD_P (pd)) + /* Not a valid thread handle. */ + return ESRCH; + + /* Is the thread joinable?. */ + if (IS_DETACHED (pd)) + /* We cannot wait for the thread. */ + return EINVAL; + + struct pthread *self = THREAD_SELF; + int result = 0; + + LIBC_PROBE (pthread_join, 1, threadid); + + if ((pd == self + || (self->joinid == pd + && (pd->cancelhandling + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK + | TERMINATED_BITMASK)) == 0)) + && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) + /* This is a deadlock situation. The threads are waiting for each + other to finish. Note that this is a "may" error. To be 100% + sure we catch this error we would have to lock the data + structures but it is not necessary. In the unlikely case that + two threads are really caught in this situation they will + deadlock. It is the programmer's problem to figure this + out. */ + return EDEADLK; + + /* Wait for the thread to finish. If it is already locked something + is wrong. There can only be one waiter. */ + else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid, + &self, + NULL))) + /* There is already somebody waiting for the thread. */ + return EINVAL; + + if (block) + { + /* During the wait we change to asynchronous cancellation. If we + are cancelled the thread we are waiting for must be marked as + un-wait-ed for again. */ + pthread_cleanup_push (cleanup, &pd->joinid); + + int oldtype = CANCEL_ASYNC (); + + if (abstime != NULL) + result = lll_timedwait_tid (pd->tid, abstime); + else + lll_wait_tid (pd->tid); + + CANCEL_RESET (oldtype); + + pthread_cleanup_pop (0); + } + + if (__glibc_likely (result == 0)) + { + /* We mark the thread as terminated and as joined. */ + pd->tid = -1; + + /* Store the return value if the caller is interested. */ + if (thread_return != NULL) + *thread_return = pd->result; + + /* Free the TCB. */ + __free_tcb (pd); + } + else + pd->joinid = NULL; + + LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); + + return result; +} +hidden_def (__pthread_timedjoin_ex) diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c index 567c171..0f74a98 100644 --- a/nptl/pthread_timedjoin.c +++ b/nptl/pthread_timedjoin.c @@ -16,89 +16,12 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <errno.h> -#include <stdlib.h> -#include <atomic.h> #include "pthreadP.h" - -static void -cleanup (void *arg) -{ - *(void **) arg = NULL; -} - - int -pthread_timedjoin_np (pthread_t threadid, void **thread_return, - const struct timespec *abstime) +__pthread_timedjoin_np (pthread_t threadid, void **thread_return, + const struct timespec *abstime) { - struct pthread *self; - struct pthread *pd = (struct pthread *) threadid; - int result; - - /* Make sure the descriptor is valid. */ - if (INVALID_NOT_TERMINATED_TD_P (pd)) - /* Not a valid thread handle. */ - return ESRCH; - - /* Is the thread joinable?. */ - if (IS_DETACHED (pd)) - /* We cannot wait for the thread. */ - return EINVAL; - - self = THREAD_SELF; - if (pd == self || self->joinid == pd) - /* This is a deadlock situation. The threads are waiting for each - other to finish. Note that this is a "may" error. To be 100% - sure we catch this error we would have to lock the data - structures but it is not necessary. In the unlikely case that - two threads are really caught in this situation they will - deadlock. It is the programmer's problem to figure this - out. */ - return EDEADLK; - - /* Wait for the thread to finish. If it is already locked something - is wrong. There can only be one waiter. */ - if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, - self, NULL), 0)) - /* There is already somebody waiting for the thread. */ - return EINVAL; - - - /* During the wait we change to asynchronous cancellation. If we - are cancelled the thread we are waiting for must be marked as - un-wait-ed for again. */ - pthread_cleanup_push (cleanup, &pd->joinid); - - /* Switch to asynchronous cancellation. */ - int oldtype = CANCEL_ASYNC (); - - - /* Wait for the child. */ - result = lll_timedwait_tid (pd->tid, abstime); - - - /* Restore cancellation mode. */ - CANCEL_RESET (oldtype); - - /* Remove the handler. */ - pthread_cleanup_pop (0); - - - /* We might have timed out. */ - if (result == 0) - { - /* Store the return value if the caller is interested. */ - if (thread_return != NULL) - *thread_return = pd->result; - - - /* Free the TCB. */ - __free_tcb (pd); - } - else - pd->joinid = NULL; - - return result; + return __pthread_timedjoin_ex (threadid, thread_return, abstime, true); } +weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np) diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c index 6a3b62e..b34df54 100644 --- a/nptl/pthread_tryjoin.c +++ b/nptl/pthread_tryjoin.c @@ -16,57 +16,17 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <errno.h> -#include <stdlib.h> - -#include <atomic.h> #include "pthreadP.h" - int pthread_tryjoin_np (pthread_t threadid, void **thread_return) { - struct pthread *self; - struct pthread *pd = (struct pthread *) threadid; - - /* Make sure the descriptor is valid. */ - if (DEBUGGING_P && __find_in_stack_list (pd) == NULL) - /* Not a valid thread handle. */ - return ESRCH; - - /* Is the thread joinable?. */ - if (IS_DETACHED (pd)) - /* We cannot wait for the thread. */ - return EINVAL; - - self = THREAD_SELF; - if (pd == self || self->joinid == pd) - /* This is a deadlock situation. The threads are waiting for each - other to finish. Note that this is a "may" error. To be 100% - sure we catch this error we would have to lock the data - structures but it is not necessary. In the unlikely case that - two threads are really caught in this situation they will - deadlock. It is the programmer's problem to figure this - out. */ - return EDEADLK; - /* Return right away if the thread hasn't terminated yet. */ + struct pthread *pd = (struct pthread *) threadid; if (pd->tid != 0) return EBUSY; - /* Wait for the thread to finish. If it is already locked something - is wrong. There can only be one waiter. */ - if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL)) - /* There is already somebody waiting for the thread. */ - return EINVAL; - - /* Store the return value if the caller is interested. */ - if (thread_return != NULL) - *thread_return = pd->result; - - - /* Free the TCB. */ - __free_tcb (pd); - - return 0; + /* If pd->tid != 0 then lll_wait_tid will not block on futex + operation. */ + return __pthread_timedjoin_ex (threadid, thread_return, NULL, false); } diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h index 197bb1f..098c64b 100644 --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h @@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime) ({ \ int __result = 0; \ if ((tid) != 0) \ - { \ - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ - __result = EINVAL; \ - else \ - __result = __lll_timedwait_tid (&(tid), (abstime)); \ - } \ + __result = __lll_timedwait_tid (&(tid), (abstime)); \ __result; }) diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h index cbf6597..0b56a2e 100644 --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h @@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *) ({ \ int __result = 0; \ if ((tid) != 0) \ - { \ - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ - __result = EINVAL; \ - else \ - __result = __lll_timedwait_tid (&(tid), (abstime)); \ - } \ + __result = __lll_timedwait_tid (&(tid), (abstime)); \ __result; }) extern int __lll_lock_elision (int *futex, short *adapt_count, int private) -- 2.7.4
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 713000e..2646b8f 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype); extern int __pthread_enable_asynccancel (void) attribute_hidden; extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden; extern void __pthread_testcancel (void); +extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *, + bool); #if IS_IN (libpthread) hidden_proto (__pthread_mutex_init) @@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate) hidden_proto (__pthread_testcancel) hidden_proto (__pthread_mutexattr_init) hidden_proto (__pthread_mutexattr_settype) +hidden_proto (__pthread_timedjoin_ex) #endif extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond); diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c index afc8c37..47379a6 100644 --- a/nptl/pthread_join.c +++ b/nptl/pthread_join.c @@ -16,103 +16,11 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <errno.h> -#include <stdlib.h> - -#include <atomic.h> #include "pthreadP.h" -#include <stap-probe.h> - - -static void -cleanup (void *arg) -{ - /* If we already changed the waiter ID, reset it. The call cannot - fail for any reason but the thread not having done that yet so - there is no reason for a loop. */ - (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL, - THREAD_SELF); -} - - int __pthread_join (pthread_t threadid, void **thread_return) { - struct pthread *pd = (struct pthread *) threadid; - - /* Make sure the descriptor is valid. */ - if (INVALID_NOT_TERMINATED_TD_P (pd)) - /* Not a valid thread handle. */ - return ESRCH; - - /* Is the thread joinable?. */ - if (IS_DETACHED (pd)) - /* We cannot wait for the thread. */ - return EINVAL; - - struct pthread *self = THREAD_SELF; - int result = 0; - - LIBC_PROBE (pthread_join, 1, threadid); - - /* During the wait we change to asynchronous cancellation. If we - are canceled the thread we are waiting for must be marked as - un-wait-ed for again. */ - pthread_cleanup_push (cleanup, &pd->joinid); - - /* Switch to asynchronous cancellation. */ - int oldtype = CANCEL_ASYNC (); - - if ((pd == self - || (self->joinid == pd - && (pd->cancelhandling - & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK - | TERMINATED_BITMASK)) == 0)) - && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) - /* This is a deadlock situation. The threads are waiting for each - other to finish. Note that this is a "may" error. To be 100% - sure we catch this error we would have to lock the data - structures but it is not necessary. In the unlikely case that - two threads are really caught in this situation they will - deadlock. It is the programmer's problem to figure this - out. */ - result = EDEADLK; - /* Wait for the thread to finish. If it is already locked something - is wrong. There can only be one waiter. */ - else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, - self, - NULL), 0)) - /* There is already somebody waiting for the thread. */ - result = EINVAL; - else - /* Wait for the child. */ - lll_wait_tid (pd->tid); - - - /* Restore cancellation mode. */ - CANCEL_RESET (oldtype); - - /* Remove the handler. */ - pthread_cleanup_pop (0); - - - if (__glibc_likely (result == 0)) - { - /* We mark the thread as terminated and as joined. */ - pd->tid = -1; - - /* Store the return value if the caller is interested. */ - if (thread_return != NULL) - *thread_return = pd->result; - - - /* Free the TCB. */ - __free_tcb (pd); - } - - LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); - - return result; + return __pthread_timedjoin_ex (threadid, thread_return, NULL, true); } weak_alias (__pthread_join, pthread_join) diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c index 567c171..6b894f2 100644 --- a/nptl/pthread_timedjoin.c +++ b/nptl/pthread_timedjoin.c @@ -21,21 +21,25 @@ #include <atomic.h> #include "pthreadP.h" +#include <stap-probe.h> + static void cleanup (void *arg) { - *(void **) arg = NULL; + /* If we already changed the waiter ID, reset it. The call cannot + fail for any reason but the thread not having done that yet so + there is no reason for a loop. */ + struct pthread *self = THREAD_SELF; + atomic_compare_exchange_weak_acquire (&arg, &self, NULL); } int -pthread_timedjoin_np (pthread_t threadid, void **thread_return, - const struct timespec *abstime) +__pthread_timedjoin_ex (pthread_t threadid, void **thread_return, + const struct timespec *abstime, bool block) { - struct pthread *self; struct pthread *pd = (struct pthread *) threadid; - int result; /* Make sure the descriptor is valid. */ if (INVALID_NOT_TERMINATED_TD_P (pd)) @@ -47,8 +51,17 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return, /* We cannot wait for the thread. */ return EINVAL; - self = THREAD_SELF; - if (pd == self || self->joinid == pd) + struct pthread *self = THREAD_SELF; + int result = 0; + + LIBC_PROBE (pthread_join, 1, threadid); + + if ((pd == self + || (self->joinid == pd + && (pd->cancelhandling + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK + | TERMINATED_BITMASK)) == 0)) + && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) /* This is a deadlock situation. The threads are waiting for each other to finish. Note that this is a "may" error. To be 100% sure we catch this error we would have to lock the data @@ -60,45 +73,56 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return, /* Wait for the thread to finish. If it is already locked something is wrong. There can only be one waiter. */ - if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid, - self, NULL), 0)) + else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid, + &self, + NULL))) /* There is already somebody waiting for the thread. */ return EINVAL; + if (block) + { + /* During the wait we change to asynchronous cancellation. If we + are cancelled the thread we are waiting for must be marked as + un-wait-ed for again. */ + pthread_cleanup_push (cleanup, &pd->joinid); - /* During the wait we change to asynchronous cancellation. If we - are cancelled the thread we are waiting for must be marked as - un-wait-ed for again. */ - pthread_cleanup_push (cleanup, &pd->joinid); - - /* Switch to asynchronous cancellation. */ - int oldtype = CANCEL_ASYNC (); - - - /* Wait for the child. */ - result = lll_timedwait_tid (pd->tid, abstime); - + int oldtype = CANCEL_ASYNC (); - /* Restore cancellation mode. */ - CANCEL_RESET (oldtype); + if (abstime != NULL) + result = lll_timedwait_tid (pd->tid, abstime); + else + lll_wait_tid (pd->tid); - /* Remove the handler. */ - pthread_cleanup_pop (0); + CANCEL_RESET (oldtype); + pthread_cleanup_pop (0); + } - /* We might have timed out. */ - if (result == 0) + if (__glibc_likely (result == 0)) { + /* We mark the thread as terminated and as joined. */ + pd->tid = -1; + /* Store the return value if the caller is interested. */ if (thread_return != NULL) *thread_return = pd->result; - /* Free the TCB. */ __free_tcb (pd); } else pd->joinid = NULL; + LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result); + return result; } +hidden_def (__pthread_timedjoin_ex) + +int +__pthread_timedjoin_np (pthread_t threadid, void **thread_return, + const struct timespec *abstime) +{ + return __pthread_timedjoin_ex (threadid, thread_return, abstime, true); +} +weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np) diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c index 6a3b62e..e7c7595 100644 --- a/nptl/pthread_tryjoin.c +++ b/nptl/pthread_tryjoin.c @@ -26,47 +26,12 @@ int pthread_tryjoin_np (pthread_t threadid, void **thread_return) { - struct pthread *self; - struct pthread *pd = (struct pthread *) threadid; - - /* Make sure the descriptor is valid. */ - if (DEBUGGING_P && __find_in_stack_list (pd) == NULL) - /* Not a valid thread handle. */ - return ESRCH; - - /* Is the thread joinable?. */ - if (IS_DETACHED (pd)) - /* We cannot wait for the thread. */ - return EINVAL; - - self = THREAD_SELF; - if (pd == self || self->joinid == pd) - /* This is a deadlock situation. The threads are waiting for each - other to finish. Note that this is a "may" error. To be 100% - sure we catch this error we would have to lock the data - structures but it is not necessary. In the unlikely case that - two threads are really caught in this situation they will - deadlock. It is the programmer's problem to figure this - out. */ - return EDEADLK; - /* Return right away if the thread hasn't terminated yet. */ + struct pthread *pd = (struct pthread *) threadid; if (pd->tid != 0) return EBUSY; - /* Wait for the thread to finish. If it is already locked something - is wrong. There can only be one waiter. */ - if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL)) - /* There is already somebody waiting for the thread. */ - return EINVAL; - - /* Store the return value if the caller is interested. */ - if (thread_return != NULL) - *thread_return = pd->result; - - - /* Free the TCB. */ - __free_tcb (pd); - - return 0; + /* If pd->tid != 0 then lll_wait_tid will not block on futex + operation. */ + return __pthread_timedjoin_ex (threadid, thread_return, NULL, false); } diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h index 197bb1f..098c64b 100644 --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h @@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime) ({ \ int __result = 0; \ if ((tid) != 0) \ - { \ - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ - __result = EINVAL; \ - else \ - __result = __lll_timedwait_tid (&(tid), (abstime)); \ - } \ + __result = __lll_timedwait_tid (&(tid), (abstime)); \ __result; }) diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h index cbf6597..0b56a2e 100644 --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h @@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *) ({ \ int __result = 0; \ if ((tid) != 0) \ - { \ - if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000) \ - __result = EINVAL; \ - else \ - __result = __lll_timedwait_tid (&(tid), (abstime)); \ - } \ + __result = __lll_timedwait_tid (&(tid), (abstime)); \ __result; }) extern int __lll_lock_elision (int *futex, short *adapt_count, int private)