From patchwork Mon Dec 7 21:27:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 41335 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7CBF03945C0A; Mon, 7 Dec 2020 21:28:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7CBF03945C0A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1607376495; bh=AkqbdLPEvnHcaf9WDEH4fOfx8FldJnLcB04IwL7bfV0=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=k9U/oSoPa93w5/5baVLJh+BlXhozv1vIRqPodN3KEFEeOjMXHV/Fct7w4T14uCyg1 S4oUiA0Kz8XJTVQjjx39Cg8FExpiFewavuac9HUGB4UvAfF74asXiuykqFCkyMp6YM ZVvCnvrNGH87in5gqvvbpUL7LPymw2vp+OzTD68k= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qk1-x741.google.com (mail-qk1-x741.google.com [IPv6:2607:f8b0:4864:20::741]) by sourceware.org (Postfix) with ESMTPS id 0A4073943540 for ; Mon, 7 Dec 2020 21:28:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0A4073943540 Received: by mail-qk1-x741.google.com with SMTP id u4so14139494qkk.10 for ; Mon, 07 Dec 2020 13:28:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=AkqbdLPEvnHcaf9WDEH4fOfx8FldJnLcB04IwL7bfV0=; b=l1imMeKxzaeoWB7v+6d5CaxMLlbonVXRwZdT6YCpYBTi4W9K3wNL3PqMCvf46OX4hI 4Sm7wAgzBdOcUhGk98IL9uSItxJFK4fF5Ucc3wf4NhnzNdo9+MSGIwZkjUKr7kUU0aZH G/1GYyVYz/+Nwu3ujWrGrtkINaqQ65xeE/y3v2oBPSY2/a7VkeOOad6elTEz7tbJHi6V F9yScYRWtp77D71GChJgleQEXNY/+8OLcycK5kD4zHkTGJlKg+6Gh/VyR7ka/V8g1otV yK5MeAffF6MWEocaHw3SEopLxIJdOIjjOp8+9aKWdLeHVNHcRzneObg3YgUnbZURAe6C 0u3Q== X-Gm-Message-State: AOAM531UtS4AMLxnsARU5eFTkTFB+nIksDgkCIEwHzuBaPXVe32Shrm7 EJ+7fF0wLeIBIQyiqqeQoOPBoZtFvcx/7w== X-Google-Smtp-Source: ABdhPJzXj4ayogrUeTfba8UBhpfZgzGMhUMB4GuchvM980klb4BZeBBcPtNRcApz1DjU5A7ycnjc7A== X-Received: by 2002:a37:4f12:: with SMTP id d18mr7141137qkb.378.1607376492153; Mon, 07 Dec 2020 13:28:12 -0800 (PST) Received: from localhost.localdomain ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id o29sm12832317qtl.7.2020.12.07.13.28.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Dec 2020 13:28:11 -0800 (PST) To: libc-alpha@sourceware.org Subject: [PATCH v2 6/8] nptl: Move cancel state out of cancelhandling Date: Mon, 7 Dec 2020 18:27:55 -0300 Message-Id: <20201207212757.3948164-6-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201207212757.3948164-1-adhemerval.zanella@linaro.org> References: <20201207212757.3948164-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Adhemerval Zanella via Libc-alpha From: Adhemerval Zanella Reply-To: Adhemerval Zanella Cc: Florian Weimer Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" Changes from previous version [1] * Rebased against master. --- The thread cancellation state is not accessed concurrently internally neither the pthread interface allows changing the state of a different thread than its own. By removing the cancel state out of the internal thread cancel handling state there is no need to check if cancelled bit was set in CAS operation. The code is also simplified: the CANCELLATION_P is replaced with a internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is removed. The second part of this patchset also keeps the pthread_setcanceltype as cancellation entrypoint by calling pthread_testcancel if the new type is PTHREAD_CANCEL_ASYNCHRONOUS. With this behavior pthread_setcancelstate does not require to act on cancellation if cancel type is asynchronous (is already handled either by pthread_setcanceltype or by the signal handler). Checked on x86_64-linux-gnu. [1] https://patchwork.sourceware.org/project/glibc/patch/20200520174819.1138124-1-adhemerval.zanella@linaro.org/ --- nptl/allocatestack.c | 1 + nptl/cancellation.c | 3 ++- nptl/cleanup_defer.c | 2 +- nptl/cleanup_defer_compat.c | 2 +- nptl/descr.h | 14 ++++++-------- nptl/pthreadP.h | 12 ------------ nptl/pthread_cancel.c | 3 ++- nptl/pthread_join_common.c | 5 ++++- nptl/pthread_setcancelstate.c | 36 +++-------------------------------- nptl/pthread_setcanceltype.c | 3 ++- nptl/pthread_testcancel.c | 11 ++++++++++- 11 files changed, 32 insertions(+), 60 deletions(-) diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index b7f9eeebf6..169ed41b9e 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -220,6 +220,7 @@ get_cached_stack (size_t *sizep, void **memp) /* Cancellation handling is back to the default. */ result->cancelhandling = 0; + result->cancelstate = PTHREAD_CANCEL_ENABLE; result->cleanup = NULL; /* No pending event. */ diff --git a/nptl/cancellation.c b/nptl/cancellation.c index 826071321e..7e8cbe9fe1 100644 --- a/nptl/cancellation.c +++ b/nptl/cancellation.c @@ -45,7 +45,8 @@ __pthread_enable_asynccancel (void) oldval); if (__glibc_likely (curval == oldval)) { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) + if (self->cancelstate == PTHREAD_CANCEL_ENABLE + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval)) { THREAD_SETMEM (self, result, PTHREAD_CANCELED); __do_cancel (); diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c index 8ad9a90c50..33d4ea6eef 100644 --- a/nptl/cleanup_defer.c +++ b/nptl/cleanup_defer.c @@ -86,6 +86,6 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf) cancelhandling = curval; } - CANCELLATION_P (self); + __pthread_testcancel (); } } diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c index 33e47888f2..a1ad291fcc 100644 --- a/nptl/cleanup_defer_compat.c +++ b/nptl/cleanup_defer_compat.c @@ -83,7 +83,7 @@ _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer, cancelhandling = curval; } - CANCELLATION_P (self); + __pthread_testcancel (); } /* If necessary call the cleanup routine after we removed the diff --git a/nptl/descr.h b/nptl/descr.h index b172ee408b..258c485ef1 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -277,9 +277,6 @@ struct pthread /* Flags determining processing of cancellation. */ int cancelhandling; - /* Bit set if cancellation is disabled. */ -#define CANCELSTATE_BIT 0 -#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT) /* Bit set if asynchronous cancellation mode is selected. */ #define CANCELTYPE_BIT 1 #define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT) @@ -301,11 +298,8 @@ struct pthread /* Mask for the rest. Helps the compiler to optimize. */ #define CANCEL_RESTMASK 0xffffff80 -#define CANCEL_ENABLED_AND_CANCELED(value) \ - (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK \ - | CANCEL_RESTMASK | TERMINATED_BITMASK)) == CANCELED_BITMASK) -#define CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS(value) \ - (((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK \ +#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \ + (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK \ | EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK)) \ == (CANCELTYPE_BITMASK | CANCELED_BITMASK)) @@ -409,6 +403,10 @@ struct pthread /* Used on strsignal. */ struct tls_internal_t tls_state; + /* Thread cancel state (PTHREAD_CANCEL_ENABLE or + PTHREAD_CANCEL_DISABLE). */ + unsigned char cancelstate; + /* This member must be last. */ char end_padding[]; diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 968adcdb6a..1a8518ef23 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -253,18 +253,6 @@ extern int __pthread_debug attribute_hidden; #endif -/* Cancellation test. */ -#define CANCELLATION_P(self) \ - do { \ - int cancelhandling = THREAD_GETMEM (self, cancelhandling); \ - if (CANCEL_ENABLED_AND_CANCELED (cancelhandling)) \ - { \ - THREAD_SETMEM (self, result, PTHREAD_CANCELED); \ - __do_cancel (); \ - } \ - } while (0) - - extern void __pthread_unwind (__pthread_unwind_buf_t *__buf) __cleanup_fct_attribute __attribute ((__noreturn__)) #if !defined SHARED && !IS_IN (libpthread) diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c index 88c1ab8f6a..5b2789d620 100644 --- a/nptl/pthread_cancel.c +++ b/nptl/pthread_cancel.c @@ -55,7 +55,8 @@ __pthread_cancel (pthread_t th) /* If the cancellation is handled asynchronously just send a signal. We avoid this if possible since it's more expensive. */ - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) + if (pd->cancelstate == PTHREAD_CANCEL_ENABLE + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval)) { /* Mark the cancellation as "in progress". */ if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c index 4293b072c8..2d23e83ced 100644 --- a/nptl/pthread_join_common.c +++ b/nptl/pthread_join_common.c @@ -59,7 +59,10 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return, && (pd->cancelhandling & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK | TERMINATED_BITMASK)) == 0)) - && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling)) + && !(self->cancelstate == PTHREAD_CANCEL_ENABLE + && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK + | TERMINATED_BITMASK)) + == CANCELED_BITMASK)) /* 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 diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c index 4d7f413e19..aa1c8073a8 100644 --- a/nptl/pthread_setcancelstate.c +++ b/nptl/pthread_setcancelstate.c @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate) self = THREAD_SELF; - int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) - { - int newval = (state == PTHREAD_CANCEL_DISABLE - ? oldval | CANCELSTATE_BITMASK - : oldval & ~CANCELSTATE_BITMASK); - - /* Store the old value. */ - if (oldstate != NULL) - *oldstate = ((oldval & CANCELSTATE_BITMASK) - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE); - - /* Avoid doing unnecessary work. The atomic operation can - potentially be expensive if the memory has to be locked and - remote cache lines have to be invalidated. */ - if (oldval == newval) - break; - - /* Update the cancel handling word. This has to be done - atomically since other bits could be modified as well. */ - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (__glibc_likely (curval == oldval)) - { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) - __do_cancel (); - - break; - } - - /* Prepare for the next round. */ - oldval = curval; - } + if (oldstate != NULL) + *oldstate = self->cancelstate; + self->cancelstate = state; return 0; } diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c index fcaae8abc7..cc0507ae04 100644 --- a/nptl/pthread_setcanceltype.c +++ b/nptl/pthread_setcanceltype.c @@ -53,7 +53,8 @@ __pthread_setcanceltype (int type, int *oldtype) oldval); if (__glibc_likely (curval == oldval)) { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) + if (self->cancelstate == PTHREAD_CANCEL_ENABLE + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval)) { THREAD_SETMEM (self, result, PTHREAD_CANCELED); __do_cancel (); diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c index 30408c2008..cd937bb3a3 100644 --- a/nptl/pthread_testcancel.c +++ b/nptl/pthread_testcancel.c @@ -23,7 +23,16 @@ void __pthread_testcancel (void) { - CANCELLATION_P (THREAD_SELF); + struct pthread *self = THREAD_SELF; + int cancelhandling = THREAD_GETMEM (self, cancelhandling); + if (self->cancelstate == PTHREAD_CANCEL_ENABLE + && (cancelhandling & CANCELED_BITMASK) + && !(cancelhandling & EXITING_BITMASK) + && !(cancelhandling & TERMINATED_BITMASK)) + { + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + __do_cancel (); + } } strong_alias (__pthread_testcancel, pthread_testcancel) hidden_def (__pthread_testcancel)