From 26cfbb7162ad364d53d69f6d482f2d87b5950524 Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella Date: Tue, 25 May 2021 14:31:30 -0300 Subject: [PATCH] nptl: Remove CANCELING_BITMASK 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 incurs in the same issues described on BZ#12683: the cancellation is acted upon even *after* syscall returns with user visible side-effects. This patch removes this optimization and simplifies the pthread cancellation implementation: pthread_cancel now first checks if cancellation is already pending and if not always, sends 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 allows 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 | 125 +++++++++++-------------------------- nptl/pthread_join_common.c | 2 +- 4 files changed, 36 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 deb404600c..33e82c3717 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; + 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; - 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; - } + /* 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,34 @@ __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 - } - /* 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; + THREAD_SETMEM (pd, result, PTHREAD_CANCELED); + if ((oldch & CANCELSTATE_BITMASK) == 0 + && (oldch & CANCELTYPE_BITMASK) != 0) + __do_cancel (); + return 0; + } + + 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