From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22423 invoked by alias); 20 Dec 2017 13:41:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 22413 invoked by uid 89); 20 Dec 2017 13:41:56 -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= X-HELO: mail-qt0-f195.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Bm+G7gxag8wkbXIUhCcrBQDsW5veVTEYmtiPAvGsFyM=; b=FHDiwT3sH+wVIS2DrJN1mqvGnKtpJXUSNo3pxYLJ9K3++W2ymZVha3qJLgMsjscrie zfqgCXV0ExW9WTK/Q9JGywwQZ1ZqHjnlo272mWhLy+f+AkKkBTtSewV/5sJru4hxKwLH pwKbHNr9KflIlk3FtL/h8nLCCX92Xqtwm5UinGUH1qz2B5RAL2vBEZeyhU0zdet8DI96 AF27EF5r6ZAF1tGETsOW/qyJz11OJxz4SjUyBQjZTIki7zYYJlR514Vy855fVszsN5Ct BYpSP71/XwlObNIMIwNwpxenpM3/Qc1upboNa/5uRljBVC50u5opTfQB4fzDDxWh0rNl c1Sw== X-Gm-Message-State: AKGB3mL1ZKDs/jdhBxciEmPq8/aGpB2RJ9o6l0wMt//tMF2seOlIxa+z j8TLVCYjG2qDSGFmeLKmb3azeyAMygY= X-Google-Smtp-Source: ACJfBovmpqBVCcToreV/53OCXvgNi6MTSQ7SAMcmr4cXI25FcUFpLHYq5CCVph5dWgbZyv94zyVUIg== X-Received: by 10.200.46.167 with SMTP id h36mr10266058qta.267.1513777311163; Wed, 20 Dec 2017 05:41:51 -0800 (PST) Subject: Re: [PATCH] nptl: Consolidate pthread_{timed,try}join{_np} To: Carlos O'Donell , libc-alpha@sourceware.org References: <1513690827-11941-1-git-send-email-adhemerval.zanella@linaro.org> <4dab5eb1-33ee-bff2-d5b0-0f8300343e72@redhat.com> From: Adhemerval Zanella Message-ID: <7fd489f4-d35d-e86a-09d0-42240edfc557@linaro.org> Date: Wed, 20 Dec 2017 13:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <4dab5eb1-33ee-bff2-d5b0-0f8300343e72@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00730.txt.bz2 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 + + * 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 * 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 . */ -#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_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 + . */ + +#include "pthreadP.h" +#include +#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. */ + 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 . */ -#include -#include -#include #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 . */ -#include -#include - -#include #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