From patchwork Tue Dec 19 13:40:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 25007 Received: (qmail 12788 invoked by alias); 19 Dec 2017 13:40:39 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 12779 invoked by uid 89); 19 Dec 2017 13:40:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=1000000000, 2612 X-HELO: mail-qk0-f179.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=QAG+oxLvH0xgJJKRN+mv0s9Lc5UsLn6tEahLxsTSW3g=; b=qmmXWv1Lhy+/dk94BqCp2Dn6Ce4qmp7H3sz/o6HREbmmqtqauvV300oGNjRFQnkJD8 s6+bZmVn1jQiGfbBcCKqKMVjXqo4et6UdN7hI8a/FkhnOdrChLIIf//OymKq1sa+0Mdc e16+P8SIPny1gEFIzouP176GWtsT7i8oOAAqkk5ewL8BSqnf1XqZ5Q94PR0UO4M+xsr1 v/78w8mSG0nSvnzppegcLXRZWDcKfhEmuOo0t1muDZ0ey//5uY/l8R5+GnLMzeEE2QOH +45KQ/9VJrZTMUWebMi97qq5aCuvhbEkyBJT4ErFhBfDncDrdqtjV/NgzGdS5ns36e6H JcZQ== X-Gm-Message-State: AKGB3mJdX0Ix8z4hkKSVm66IJLhHsfzBb4ejjWAoRcF3hfsfMMwTV5zu HEwvjF0v4ws9+CsSfnnbPXetBW4mfkk= X-Google-Smtp-Source: ACJfBovqbzDkVyZmMt1YQXjN8OF2pms2A4TQzkQ1KJ+1Mgu+2MMfO21c5Ru/X06JFaXO2FEJn4s5ug== X-Received: by 10.55.209.210 with SMTP id o79mr4865778qkl.4.1513690834367; Tue, 19 Dec 2017 05:40:34 -0800 (PST) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] nptl: Consolidate pthread_{timed,try}join{_np} Date: Tue, 19 Dec 2017 11:40:27 -0200 Message-Id: <1513690827-11941-1-git-send-email-adhemerval.zanella@linaro.org> 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). 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. Reviewed-by: Carlos O'Donell --- 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); #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 . */ -#include -#include - -#include #include "pthreadP.h" -#include - - -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 #include "pthreadP.h" +#include + 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)