[08/11] nptl: Move cancel state out of cancelhandling
Checks
Commit Message
Now that thread cancellation state is not accessed concurrently anymore,
it is possible to move it out the 'cancelhandling'.
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 and aarch64-linux-gnu.
---
manual/pattern.texi | 1 -
manual/process.texi | 3 +--
nptl/allocatestack.c | 1 +
nptl/cancellation.c | 3 ++-
nptl/cleanup_defer.c | 2 +-
nptl/descr.h | 14 ++++++--------
nptl/libc-cleanup.c | 2 +-
nptl/pthreadP.h | 12 ------------
nptl/pthread_cancel.c | 2 +-
nptl/pthread_join_common.c | 5 ++++-
nptl/pthread_setcancelstate.c | 36 +++--------------------------------
nptl/pthread_setcanceltype.c | 3 ++-
nptl/pthread_testcancel.c | 11 ++++++++++-
13 files changed, 32 insertions(+), 63 deletions(-)
Comments
* Adhemerval Zanella via Libc-alpha:
> diff --git a/nptl/descr.h b/nptl/descr.h
> index a120365f88..a3084fdf60 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -407,6 +401,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;
You could move this into the padding after the c11 flag, I think.
I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
__tls_init_tp and somewhere in pthread_create. Maybe add a static
assert for PTHREAD_CANCEL_ENABLE == 0?
Thanks,
Florian
On 26/05/2021 15:20, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index a120365f88..a3084fdf60 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>
>> @@ -407,6 +401,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;
>
> You could move this into the padding after the c11 flag, I think.
Right, I moved to below c11. What kind of constraint we have for the
'struct pthread' regarding its internal member layout? We have a couple
of unused fields and might be good to clean them up.
>
> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
> __tls_init_tp and somewhere in pthread_create. Maybe add a static
> assert for PTHREAD_CANCEL_ENABLE == 0?
I think it would be better to add an initialization on __tls_init_tp,
similar to get_cached_stack. It would be better if we consolidate
the 'struct pthread' initialization in a common place.
* Adhemerval Zanella:
> On 26/05/2021 15:20, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> diff --git a/nptl/descr.h b/nptl/descr.h
>>> index a120365f88..a3084fdf60 100644
>>> --- a/nptl/descr.h
>>> +++ b/nptl/descr.h
>>
>>> @@ -407,6 +401,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;
>>
>> You could move this into the padding after the c11 flag, I think.
>
> Right, I moved to below c11. What kind of constraint we have for the
> 'struct pthread' regarding its internal member layout? We have a couple
> of unused fields and might be good to clean them up.
The sanitizers depend on the size to derive the TLS memory area from the
thread pointer.
>> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
>> __tls_init_tp and somewhere in pthread_create. Maybe add a static
>> assert for PTHREAD_CANCEL_ENABLE == 0?
>
> I think it would be better to add an initialization on __tls_init_tp,
> similar to get_cached_stack. It would be better if we consolidate
> the 'struct pthread' initialization in a common place.
We depend a lot on zero initialization, though. Maybe it would be
better to use memset in get_cached_stack. Unrelated change, of course.
Thanks,
Florian
On 27/05/2021 13:48, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 26/05/2021 15:20, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> diff --git a/nptl/descr.h b/nptl/descr.h
>>>> index a120365f88..a3084fdf60 100644
>>>> --- a/nptl/descr.h
>>>> +++ b/nptl/descr.h
>>>
>>>> @@ -407,6 +401,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;
>>>
>>> You could move this into the padding after the c11 flag, I think.
>>
>> Right, I moved to below c11. What kind of constraint we have for the
>> 'struct pthread' regarding its internal member layout? We have a couple
>> of unused fields and might be good to clean them up.
>
> The sanitizers depend on the size to derive the TLS memory area from the
> thread pointer.
Yeah, but they use a map to version and size to get the correct size.
If we change the size, sanitizer might fix with a new entry in the
map.
>
>>> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
>>> __tls_init_tp and somewhere in pthread_create. Maybe add a static
>>> assert for PTHREAD_CANCEL_ENABLE == 0?
>>
>> I think it would be better to add an initialization on __tls_init_tp,
>> similar to get_cached_stack. It would be better if we consolidate
>> the 'struct pthread' initialization in a common place.
>
> We depend a lot on zero initialization, though. Maybe it would be
> better to use memset in get_cached_stack. Unrelated change, of course.
Indeed, but implicit initialization is usually harder to understand
specially in this case where it is done in multiple places.
@@ -1820,7 +1820,6 @@ the beginning of the vector.
@c (disable cancellation around exec_comm; it may do_cancel the
@c second time, if async cancel is enabled)
@c THREAD_ATOMIC_CMPXCHG_VAL dup ok
-@c CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS dup ok
@c do_cancel @ascuplugin @ascuheap @acsmem
@c THREAD_ATOMIC_BIT_SET dup ok
@c pthread_unwind @ascuplugin @ascuheap @acsmem
@@ -68,7 +68,7 @@ until the subprogram terminates before you can do anything else.
@c CLEANUP_HANDLER @ascuplugin @ascuheap @acsmem
@c libc_cleanup_region_start @ascuplugin @ascuheap @acsmem
@c pthread_cleanup_push_defer @ascuplugin @ascuheap @acsmem
-@c CANCELLATION_P @ascuplugin @ascuheap @acsmem
+@c __pthread_testcancel @ascuplugin @ascuheap @acsmem
@c CANCEL_ENABLED_AND_CANCELED ok
@c do_cancel @ascuplugin @ascuheap @acsmem
@c cancel_handler ok
@@ -86,7 +86,6 @@ until the subprogram terminates before you can do anything else.
@c SINGLE_THREAD_P ok
@c LIBC_CANCEL_ASYNC @ascuplugin @ascuheap @acsmem
@c libc_enable_asynccancel @ascuplugin @ascuheap @acsmem
-@c CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS dup ok
@c do_cancel dup @ascuplugin @ascuheap @acsmem
@c LIBC_CANCEL_RESET ok
@c libc_disable_asynccancel ok
@@ -160,6 +160,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;
result->setup_failed = 0;
@@ -44,7 +44,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 ();
@@ -92,7 +92,7 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
cancelhandling = curval;
}
- CANCELLATION_P (self);
+ __pthread_testcancel ();
}
}
versioned_symbol (libc, ___pthread_unregister_cancel_restore,
@@ -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)
@@ -298,11 +295,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))
@@ -407,6 +401,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[];
@@ -79,7 +79,7 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
cancelhandling = curval;
}
- CANCELLATION_P (self);
+ __pthread_testcancel ();
}
}
libc_hidden_def (__libc_cleanup_pop_restore)
@@ -245,18 +245,6 @@ libc_hidden_proto (__pthread_current_priority)
#define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
#define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)
-/* 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)
@@ -45,7 +45,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
int ch = atomic_load_relaxed (&self->cancelhandling);
/* Cancelation not enabled, not cancelled, or already exitting. */
- if ((ch & CANCELSTATE_BITMASK) != 0
+ if (self->cancelstate == PTHREAD_CANCEL_DISABLE
|| (ch & CANCELED_BITMASK) == 0
|| (ch & EXITING_BITMASK) != 0)
return;
@@ -59,7 +59,10 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
&& (pd->cancelhandling
& (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
@@ -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;
}
@@ -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 ();
@@ -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 ();
+ }
}
versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
versioned_symbol (libc, ___pthread_testcancel, __pthread_testcancel,