From patchwork Wed May 26 16:57:24 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 43595 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 1A2A93945C30; Wed, 26 May 2021 16:57:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1A2A93945C30 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1622048266; bh=u98OM6DsDp3rqugcyn91ND8u7aMGLkiuWD0MQ9n6/1I=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=vkgOWJihR6y7FS0AU+rwQRLkAeCgCEorifNYENwlpJyxXCZdOUF7H1L7NpxQSbk93 5+Rwr7rsu3h78OKtRTLWpEvRx0Iq3S0Zia2isjaIPTYrPaf2LexCJkbMimkU96qPUO xNkNCrfRCxhNMCVwmEAN9my0uBNgBkx1JKC2yIo0= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) by sourceware.org (Postfix) with ESMTPS id 310533945C31 for ; Wed, 26 May 2021 16:57:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 310533945C31 Received: by mail-qk1-x72b.google.com with SMTP id i5so1641536qkf.12 for ; Wed, 26 May 2021 09:57:43 -0700 (PDT) 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=u98OM6DsDp3rqugcyn91ND8u7aMGLkiuWD0MQ9n6/1I=; b=UYZZQEsFTN4S+00gYwBlXWuqCmVwC6oXo8/dG9RJgUBJqEPvueCDduAxYkj7JR1lFm /AjWCkkiPXGTIxRVDWoBgPabZqflO7Ku+BCPU+lVFZYyx833GUO4PG9yfgJqJ+171Am+ 5uPXqeQ/4lvr2C/CSPdkk/BNhgZvAU0PKIKF5pi/M6D0lTxWf8ofAmfUp4OT+8MzakJG 0vshtzRM6/QKxVUVoF7G2mvL2jDx0b6KbveT+/s6wz8Y9kQHgu+qbC2Z33rVrv1qNCSJ acFdzdy+We8NlaitmKGwB/mThIzDBhbqDLuCxBUPqFphAHn+5VLyHACQza2xawRWttrG DSqg== X-Gm-Message-State: AOAM533Lggmns2kwdE4FE0zCSdQGFneLMdTe8x8HDFbEFdEm2kwLPfEe 6YLe+WVaBUNMUebTDa5Z2BlTQKNU2vvI9A== X-Google-Smtp-Source: ABdhPJyvOS+9BGhZZdKGN4cHePbU6CaNQOmjLdIE0nCisNcSvbV3HaCNUffOczsOXuHu/HpBAgLzsA== X-Received: by 2002:a05:620a:15e3:: with SMTP id p3mr41933272qkm.115.1622048262304; Wed, 26 May 2021 09:57:42 -0700 (PDT) Received: from birita.. ([177.194.37.86]) by smtp.googlemail.com with ESMTPSA id k125sm1859761qkf.53.2021.05.26.09.57.41 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 May 2021 09:57:42 -0700 (PDT) To: libc-alpha@sourceware.org Subject: [PATCH 07/11] nptl: Remove CANCELING_BITMASK Date: Wed, 26 May 2021 13:57:24 -0300 Message-Id: <20210526165728.1772546-8-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210526165728.1772546-1-adhemerval.zanella@linaro.org> References: <20210526165728.1772546-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 X-Spam-Status: No, score=-12.6 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 Netto Reply-To: Adhemerval Zanella Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" The CANCELING_BITMASK is used as an optimization to avoid sending the signal when pthread_cancel is called in a concurrent manner. This requires then to put both the cancellation state and type on a shared state (cancelhandling), since 'pthread_cancel' checks whether cancellation is enabled and asynchrnous to either cancel itself of sending the signal. It also requires handle the CANCELING_BITMASK on __pthread_disable_asynccancel, however this is incurs in the same issues described on BZ#12683: the cancellation is acting even *after* the syscalls returns with user visible side-effects. This patch removes this optimization and simplifies the pthread cancellation implementation: pthread_cancel now first check if cancellation is already pending and if not always send a signal if the target is not itself. The SIGCANCEL handler is also simpified since there is not need to setup a CAS loop. It also alows to move both the cancellation state and mode out of 'cancelhadling' (it is done in subsequent patches). Checked on x86_64-linux-gnu and aarch64-linux-gnu. --- nptl/cancellation.c | 12 ---- nptl/descr.h | 3 - nptl/pthread_cancel.c | 124 ++++++++++--------------------------- nptl/pthread_join_common.c | 2 +- 4 files changed, 35 insertions(+), 106 deletions(-) diff --git a/nptl/cancellation.c b/nptl/cancellation.c index c20845adc0..b15f25d8f6 100644 --- a/nptl/cancellation.c +++ b/nptl/cancellation.c @@ -88,17 +88,5 @@ __pthread_disable_asynccancel (int oldtype) /* Prepare the next round. */ oldval = curval; } - - /* We cannot return when we are being canceled. Upon return the - thread might be things which would have to be undone. The - following loop should loop until the cancellation signal is - delivered. */ - while (__builtin_expect ((newval & (CANCELING_BITMASK | CANCELED_BITMASK)) - == CANCELING_BITMASK, 0)) - { - futex_wait_simple ((unsigned int *) &self->cancelhandling, newval, - FUTEX_PRIVATE); - newval = THREAD_GETMEM (self, cancelhandling); - } } libc_hidden_def (__pthread_disable_asynccancel) diff --git a/nptl/descr.h b/nptl/descr.h index 9d8297b45f..a120365f88 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -283,9 +283,6 @@ struct pthread /* Bit set if asynchronous cancellation mode is selected. */ #define CANCELTYPE_BIT 1 #define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT) - /* Bit set if canceling has been initiated. */ -#define CANCELING_BIT 2 -#define CANCELING_BITMASK (0x01 << CANCELING_BIT) /* Bit set if canceled. */ #define CANCELED_BIT 3 #define CANCELED_BITMASK (0x01 << CANCELED_BIT) diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c index f264a4096a..c11a2b4551 100644 --- a/nptl/pthread_cancel.c +++ b/nptl/pthread_cancel.c @@ -43,35 +43,18 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx) struct pthread *self = THREAD_SELF; - int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) - { - /* We are canceled now. When canceled by another thread this flag - is already set but if the signal is directly send (internally or - from another process) is has to be done here. */ - int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; - - if (oldval == newval || (oldval & EXITING_BITMASK) != 0) - /* Already canceled or exiting. */ - break; - - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (curval == oldval) - { - /* Set the return value. */ - THREAD_SETMEM (self, result, PTHREAD_CANCELED); - - /* Make sure asynchronous cancellation is still enabled. */ - if ((newval & CANCELTYPE_BITMASK) != 0) - /* Run the registered destructors and terminate the thread. */ - __do_cancel (); - - break; - } - - oldval = curval; - } + int ch = atomic_load_relaxed (&self->cancelhandling); + /* Cancelation not enabled, not cancelled, or already exitting. */ + if ((ch & CANCELSTATE_BITMASK) != 0 + || (ch & CANCELED_BITMASK) == 0 + || (ch & EXITING_BITMASK) != 0) + return; + + /* Set the return value. */ + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + /* Make sure asynchronous cancellation is still enabled. */ + if ((ch & CANCELTYPE_BITMASK) != 0) + __do_cancel (); } int @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th) " must be installed for pthread_cancel to work\n"); } #endif - int result = 0; - int oldval; - int newval; - do + + int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK); + if ((oldch & CANCELED_BITMASK) != 0) + return 0; + + if (pd == THREAD_SELF) { - again: - oldval = pd->cancelhandling; - newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; - - /* Avoid doing unnecessary work. The atomic operation can - potentially be expensive if the bug has to be locked and - remote cache lines have to be invalidated. */ - if (oldval == newval) - break; - - /* 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)) - { - /* Mark the cancellation as "in progress". */ - if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, - oldval | CANCELING_BITMASK, - oldval)) - goto again; - - if (pd == THREAD_SELF) - /* This is not merely an optimization: An application may - call pthread_cancel (pthread_self ()) without calling - pthread_create, so the signal handler may not have been - set up for a self-cancel. */ - { - THREAD_SETMEM (pd, result, PTHREAD_CANCELED); - if ((newval & CANCELTYPE_BITMASK) != 0) - __do_cancel (); - } - else - { - /* The cancellation handler will take care of marking the - thread as canceled. */ - pid_t pid = __getpid (); - - int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, - SIGCANCEL); - if (INTERNAL_SYSCALL_ERROR_P (val)) - result = INTERNAL_SYSCALL_ERRNO (val); - } - - break; - } - - /* A single-threaded process should be able to kill itself, since - there is nothing in the POSIX specification that says that it - cannot. So we set multiple_threads to true so that cancellation - points get executed. */ - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1); + /* A single-threaded process should be able to kill itself, since there + is nothing in the POSIX specification that says that it cannot. So + we set multiple_threads to true so that cancellation points get + executed. */ + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1); #ifndef TLS_MULTIPLE_THREADS_IN_TCB - __libc_multiple_threads = 1; + __libc_multiple_threads = 1; #endif + + THREAD_SETMEM (pd, result, PTHREAD_CANCELED); + if ((oldch & CANCELTYPE_BITMASK) != 0) + __do_cancel (); + return 0; } - /* Mark the thread as canceled. This has to be done - atomically since other bits could be modified as well. */ - while (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, newval, - oldval)); - return result; + pid_t pid = __getpid (); + int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL); + return INTERNAL_SYSCALL_ERROR_P (val) + ? INTERNAL_SYSCALL_ERRNO (val) + : 0; } versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34); diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c index e87801b5a3..f842c91a08 100644 --- a/nptl/pthread_join_common.c +++ b/nptl/pthread_join_common.c @@ -57,7 +57,7 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return, if ((pd == self || (self->joinid == pd && (pd->cancelhandling - & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_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