| Message ID | 20200401142439.1906574-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] nptl: Remove x86_64 cancellation assembly implementations | expand |
Ping. On 01/04/2020 11:24, Adhemerval Zanella wrote: > The thread cancellation state is not accessed concurrently internally > neither the pthread interface allows changing the state of a different > thread than its own. > > The code is also simplified: the CANCELLATION_P is replaced with a > internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is > removed. > > Checked on x86_64-linux-gnu. > --- > nptl/allocatestack.c | 1 + > nptl/cancellation.c | 3 ++- > nptl/cleanup_defer.c | 2 +- > nptl/cleanup_defer_compat.c | 2 +- > nptl/descr.h | 13 +++++-------- > 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, 31 insertions(+), 60 deletions(-) > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index c94980c21c..d9174274c2 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -232,6 +232,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 9dcf480bdf..61665bf859 100644 > --- a/nptl/descr.h > +++ b/nptl/descr.h > @@ -269,9 +269,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) > @@ -293,11 +290,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)) > > @@ -394,6 +388,9 @@ struct pthread > /* Indicates whether is a C11 thread created by thrd_creat. */ > bool c11; > > + /* Thread cancel state (enable, disable). */ > + unsigned char cancelstate; > + > /* This member must be last. */ > char end_padding[]; > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 06fb0d74c5..d55c3b26a4 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -258,18 +258,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 a96ceafde4..03e202136f 100644 > --- a/nptl/pthread_join_common.c > +++ b/nptl/pthread_join_common.c > @@ -105,7 +105,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..3ffff4ebef 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 | EXITING_BITMASK > + | TERMINATED_BITMASK)) > + == CANCELED_BITMASK) > + { > + THREAD_SETMEM (self, result, PTHREAD_CANCELED); > + __do_cancel (); > + } > } > strong_alias (__pthread_testcancel, pthread_testcancel) > hidden_def (__pthread_testcancel) >
* Adhemerval Zanella via Libc-alpha: > 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; > } Why isn't this needed anymore? I think this should be mentioned explicitly in the commit message.
On 22/04/2020 11:11, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> 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; >> } > > Why isn't this needed anymore? I think this should be mentioned > explicitly in the commit message. > Do you mean the pthread_setcancelstate not being a cancellation entrypoint anymore? If it were the question, because POSIX does not state neither if it should be or may be a cancellation entrypoint [1]. Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the thread will be cancelled by the signal handler in asynchronous mode or in the next cancellation entrypoint for deferred mode. [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
* Adhemerval Zanella: > On 22/04/2020 11:11, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> 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; >>> } >> >> Why isn't this needed anymore? I think this should be mentioned >> explicitly in the commit message. >> > > Do you mean the pthread_setcancelstate not being a cancellation entrypoint > anymore? If it were the question, because POSIX does not state neither > if it should be or may be a cancellation entrypoint [1]. > > Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the > thread will be cancelled by the signal handler in asynchronous mode > or in the next cancellation entrypoint for deferred mode. > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html We can have a conversation about this change, but has it to be part of this patchset? I would like to keep it separate if possible, so that we can make some progress here. (I have not reviewed this patch, I raised this question because this particular change stood out on my first pass through the patch.)
On 24/04/2020 10:21, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 22/04/2020 11:11, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> 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; >>>> } >>> >>> Why isn't this needed anymore? I think this should be mentioned >>> explicitly in the commit message. >>> >> >> Do you mean the pthread_setcancelstate not being a cancellation entrypoint >> anymore? If it were the question, because POSIX does not state neither >> if it should be or may be a cancellation entrypoint [1]. >> >> Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the >> thread will be cancelled by the signal handler in asynchronous mode >> or in the next cancellation entrypoint for deferred mode. >> >> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html > > We can have a conversation about this change, but has it to be part of > this patchset? I would like to keep it separate if possible, so that > we can make some progress here. > > (I have not reviewed this patch, I raised this question because this > particular change stood out on my first pass through the patch.) Alright, what about this commit message then: 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. Checked on x86_64-linux-gnu.
On 30/04/2020 08:11, Adhemerval Zanella wrote: > > > On 24/04/2020 10:21, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 22/04/2020 11:11, Florian Weimer wrote: >>>> * Adhemerval Zanella via Libc-alpha: >>>> >>>>> 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; >>>>> } >>>> >>>> Why isn't this needed anymore? I think this should be mentioned >>>> explicitly in the commit message. >>>> >>> >>> Do you mean the pthread_setcancelstate not being a cancellation entrypoint >>> anymore? If it were the question, because POSIX does not state neither >>> if it should be or may be a cancellation entrypoint [1]. >>> >>> Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the >>> thread will be cancelled by the signal handler in asynchronous mode >>> or in the next cancellation entrypoint for deferred mode. >>> >>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html >> >> We can have a conversation about this change, but has it to be part of >> this patchset? I would like to keep it separate if possible, so that >> we can make some progress here. >> >> (I have not reviewed this patch, I raised this question because this >> particular change stood out on my first pass through the patch.) > > Alright, what about this commit message then: > > 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. > > Checked on x86_64-linux-gnu. > Florian, is the commit message change enough to move forward or do you think this patch requires some work/change? (this is a pre-requisite to BZ#12678 which I would like to progress as well).
* Adhemerval Zanella: >>> (I have not reviewed this patch, I raised this question because this >>> particular change stood out on my first pass through the patch.) >> >> Alright, what about this commit message then: >> >> 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. >> >> Checked on x86_64-linux-gnu. >> > > Florian, is the commit message change enough to move forward or do you > think this patch requires some work/change? > > (this is a pre-requisite to BZ#12678 which I would like to progress > as well). As I said, I have not yet reviewed the patch. It is on my to-do list, though.
* Adhemerval Zanella via Libc-alpha: > The thread cancellation state is not accessed concurrently internally > neither the pthread interface allows changing the state of a different > thread than its own. > > The code is also simplified: the CANCELLATION_P is replaced with a > internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is > removed. > > Checked on x86_64-linux-gnu. Please use the more elaborate commit message. It is helpful. > diff --git a/nptl/descr.h b/nptl/descr.h > index 9dcf480bdf..61665bf859 100644 > --- a/nptl/descr.h > +++ b/nptl/descr.h > @@ -394,6 +388,9 @@ struct pthread > /* Indicates whether is a C11 thread created by thrd_creat. */ > bool c11; > > + /* Thread cancel state (enable, disable). */ > + unsigned char cancelstate; > + Please document the permitted values in the comment. > 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; > } I've re-read the old code and I think this checks for cancellation even in the absence of a race (i.e., if the CAS succeeds). I still think we should preserve this behavior, also for symmetry with this code below: > 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..3ffff4ebef 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 | EXITING_BITMASK > + | TERMINATED_BITMASK)) > + == CANCELED_BITMASK) > + { > + THREAD_SETMEM (self, result, PTHREAD_CANCELED); > + __do_cancel (); > + } > } > strong_alias (__pthread_testcancel, pthread_testcancel) > hidden_def (__pthread_testcancel) I think you can write this as self->cancelstate == PTHREAD_CANCEL_ENABLE && (cancelhandling & CANCELED_BITMASK) && !(cancelhandling & EXITING_BITMASK) && !(cancelhandling & TERMINATED_BITMASK) and GCC will do the right thing. I find this variant easier to read, but I don't have a strong preference.
On 16/05/2020 15:38, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> The thread cancellation state is not accessed concurrently internally >> neither the pthread interface allows changing the state of a different >> thread than its own. >> >> The code is also simplified: the CANCELLATION_P is replaced with a >> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is >> removed. >> >> Checked on x86_64-linux-gnu. > > Please use the more elaborate commit message. It is helpful. Ack. > >> diff --git a/nptl/descr.h b/nptl/descr.h >> index 9dcf480bdf..61665bf859 100644 >> --- a/nptl/descr.h >> +++ b/nptl/descr.h > >> @@ -394,6 +388,9 @@ struct pthread >> /* Indicates whether is a C11 thread created by thrd_creat. */ >> bool c11; >> >> + /* Thread cancel state (enable, disable). */ >> + unsigned char cancelstate; >> + > > Please document the permitted values in the comment. Ack. > >> 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; >> } > > I've re-read the old code and I think this checks for cancellation > even in the absence of a race (i.e., if the CAS succeeds). I still > think we should preserve this behavior, also for symmetry with this > code below: The second part of this change keeps the pthread_setcanceltype as cancellation entrypoint by calling pthread_testcancel if the new type is PTHREAD_CANCEL_ASYNCHRONOUS. And with this behavior I don't see a clear rationale to keep pthread_setcancelstate as a cancellation entrypoint since it should act iff cancel type is PTHREAD_CANCEL_ASYNCHRONOUS (which is already handled by pthread_setcanceltype). > >> 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..3ffff4ebef 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 | EXITING_BITMASK >> + | TERMINATED_BITMASK)) >> + == CANCELED_BITMASK) >> + { >> + THREAD_SETMEM (self, result, PTHREAD_CANCELED); >> + __do_cancel (); >> + } >> } >> strong_alias (__pthread_testcancel, pthread_testcancel) >> hidden_def (__pthread_testcancel) > > I think you can write this as > > self->cancelstate == PTHREAD_CANCEL_ENABLE > && (cancelhandling & CANCELED_BITMASK) > && !(cancelhandling & EXITING_BITMASK) > && !(cancelhandling & TERMINATED_BITMASK) > > and GCC will do the right thing. I find this variant easier to read, > but I don't have a strong preference. > Ack, I think it is slight better as well.
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index c94980c21c..d9174274c2 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -232,6 +232,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 9dcf480bdf..61665bf859 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -269,9 +269,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) @@ -293,11 +290,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)) @@ -394,6 +388,9 @@ struct pthread /* Indicates whether is a C11 thread created by thrd_creat. */ bool c11; + /* Thread cancel state (enable, disable). */ + unsigned char cancelstate; + /* This member must be last. */ char end_padding[]; diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 06fb0d74c5..d55c3b26a4 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -258,18 +258,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 a96ceafde4..03e202136f 100644 --- a/nptl/pthread_join_common.c +++ b/nptl/pthread_join_common.c @@ -105,7 +105,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..3ffff4ebef 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 | EXITING_BITMASK + | TERMINATED_BITMASK)) + == CANCELED_BITMASK) + { + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + __do_cancel (); + } } strong_alias (__pthread_testcancel, pthread_testcancel) hidden_def (__pthread_testcancel)
The thread cancellation state is not accessed concurrently internally neither the pthread interface allows changing the state of a different thread than its own. The code is also simplified: the CANCELLATION_P is replaced with a internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is removed. Checked on x86_64-linux-gnu. --- nptl/allocatestack.c | 1 + nptl/cancellation.c | 3 ++- nptl/cleanup_defer.c | 2 +- nptl/cleanup_defer_compat.c | 2 +- nptl/descr.h | 13 +++++-------- 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, 31 insertions(+), 60 deletions(-)