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: 122381 Delivered-To: patch@linaro.org Received: by 10.140.22.227 with SMTP id 90csp4167488qgn; Tue, 19 Dec 2017 05:40:49 -0800 (PST) X-Google-Smtp-Source: ACJfBovpBg/9aKJ9YULt6s0aWtBnTTnZ6Yg3VfUnqIQLUQkuJXbGaulukEL+rDdsM8kT16iTfHpK X-Received: by 10.98.10.143 with SMTP id 15mr3253459pfk.89.1513690849806; Tue, 19 Dec 2017 05:40:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1513690849; cv=none; d=google.com; s=arc-20160816; b=MtHORk7NQX2RCHovAGcl//RONjj85VNe7tX4X2c4wX2cLF41J9bHlnpqNZe3/AzV1h eoCoz16MQoi2AbNDxpKxbxDS2YpgMpLicdQYzk9Ek9UgC6Z2hB5ANavPlN4LN28EFWhu +NUNa7QW2KHhS0cDgNLX+S/GmUJQ4z0+fSoTBXVm0Q85hBMM/aa+lQyqJO4Bif61SnDP rykzD8VkqzkNs5SJLbvILg6Y5mcQYbqYdmRmr33vrnTwxVCrMsPqB0I0FoFLsieFZE8H AyBQfMe+JBsCednD5jwoXhtt8JCsM74D5oE8s/KdI0bi1cr/WMgTjEmPOH+rIszm3qwm CbVw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:subject:to:from:delivered-to:sender:list-help :list-post:list-archive:list-subscribe:list-unsubscribe:list-id :precedence:mailing-list:dkim-signature:domainkey-signature :arc-authentication-results; bh=QAG+oxLvH0xgJJKRN+mv0s9Lc5UsLn6tEahLxsTSW3g=; b=ae5x060QN32KI97MkEkOrN9XtnBP1umqGj0UPJKe9ldQpY0czhmMRjI6CnODgT+bck l2jNvP9g+dCRESOwTdxv7VorG2BnAEdYZsroO8/Ft+RENiOvC/1K/Tt2a6QgGn1nMLCW 04hLYK5hbE9QdO1SV1J/Kl8P1Z5GDegvfKWdJqgdoIJdy0K1m5qm/zXAgwv8UVuZW0vd 5JElFXB8c7Axu7yfP9og9vlXjPHluznKJurrFoHCNGe3o4HkDQNnhlY2HGmuV3op0RDD RTzlC7xszY0/v7mMOo5w6+evTria469ug6WpoOYeu6yDNqEEPYXiFlQzhRcirjFHXWFG BOkA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=xzQ3pHel; spf=pass (google.com: domain of libc-alpha-return-88337-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-88337-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id w70si11292272pfk.109.2017.12.19.05.40.49 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Dec 2017 05:40:49 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-88337-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=xzQ3pHel; spf=pass (google.com: domain of libc-alpha-return-88337-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-88337-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; q=dns; s= default; b=PDqxvqazq8vBQGf3UsOMIPn14V0bWbPtrkaW+ZoDKdXcHkHawth3Z tIgFc2LGLFp15pMolcqPTFnrd4NbLGKTxRLsRBZRa7uMQtfGQCkOh1b5dR1/CMQZ iC/pGnvEbrmLnNyCmM4vCq0zAdHHvGcSUCwbC60S306fnD4CTGvOdY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; s=default; bh=/j9usu0BfsHEBBxi/rpQjz60EQs=; b=xzQ3pHelRG/y2OOoL7gXQ7pM7k4p egoZ3fPYeN7kR7O/B/zkysdk5YGb5iRxh2pv6mTE3N2o2Y+GwN1uXV+I9zGurl8Z T99eIyMrm7l2GX1THaw3c/RKpHsTK+Fpvynu2KIdELC73juF1JUiD2VNsENpIgFY uBMysmQghdljtss= 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-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. --- 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(-) -- 2.7.4 Reviewed-by: Carlos O'Donell 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)