From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107423 invoked by alias); 20 Dec 2017 05:44:09 -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 107413 invoked by uid 89); 20 Dec 2017 05:44:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-qk0-f172.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:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=4sozT8jIxByGbMKe5YDt+LyPoNLSGZt5j1reV/Hskjc=; b=hGXYIykWz51lV4KVn/v14w0IBCUv0JXbrHgjWB+1c2zANR+MfSd2UgQt9XQPcJkQbn T3asDEaVs39AbeyDfBuFwgfvdUYk8opRGCpGpXPNbUtz6koBb4s6OVh1cq4waBclUYgg cD6vMF0tk7Jf7NfX3Nr044fCYqDNlmCh+mxW1lB2wHIuIvGLL1T82h1wdAS4s+oIMbKY Suf4TZcaF7mSOXliUfZDxxDQ8IAmPZVbgJYREdPIQhixSwF7JfrIN67rIE6IuSdBs60P aAdtjXNYIO/dFYdiNcrWs6ge8I2iPNtR/JdeU2253wQC59extebINv8JZVCZGdxVxg5L hjMA== X-Gm-Message-State: AKGB3mJU0PEpylO8A7Df3nZgcExFl+/El8oQGUNfiqnPq7VNcaQBrjs2 yqHLhqecv7C0A9uuqJ9b2nuTRV6KPqg= X-Google-Smtp-Source: ACJfBouQw3opBAsazQD2AUvgm1Hah35vJI+EzrUNuSVQ2AieYFAQghs/aARdTA4x/egI2+8ZOA0Wuw== X-Received: by 10.55.103.198 with SMTP id b189mr5390481qkc.50.1513748643386; Tue, 19 Dec 2017 21:44:03 -0800 (PST) Subject: Re: [PATCH] nptl: Consolidate pthread_{timed,try}join{_np} To: Adhemerval Zanella , libc-alpha@sourceware.org References: <1513690827-11941-1-git-send-email-adhemerval.zanella@linaro.org> From: Carlos O'Donell Message-ID: <4dab5eb1-33ee-bff2-d5b0-0f8300343e72@redhat.com> Date: Wed, 20 Dec 2017 05:44: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: <1513690827-11941-1-git-send-email-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00702.txt.bz2 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 > --- > 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 > . */ > > -#include > -#include > - > -#include OK. > #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); 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 > #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); > } 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.