diff mbox series

[08/11] nptl: Move cancel state out of cancelhandling

Message ID 20210526165728.1772546-9-adhemerval.zanella@linaro.org
State New
Headers show
Series nptl: pthread cancellation refactor | expand

Commit Message

Adhemerval Zanella Netto May 26, 2021, 4:57 p.m. UTC 
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

Florian Weimer May 26, 2021, 6:20 p.m. UTC | #1 
* 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
Adhemerval Zanella Netto May 27, 2021, 4:40 p.m. UTC | #2 
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.
Florian Weimer May 27, 2021, 4:48 p.m. UTC | #3 
* 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
Adhemerval Zanella Netto May 27, 2021, 4:57 p.m. UTC | #4 
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.
diff mbox series

Patch

diff --git a/manual/pattern.texi b/manual/pattern.texi
index 39ae97a3c4..4fa4c25525 100644
--- a/manual/pattern.texi
+++ b/manual/pattern.texi
@@ -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
diff --git a/manual/process.texi b/manual/process.texi
index 54e65f76c6..134d5c6143 100644
--- a/manual/process.texi
+++ b/manual/process.texi
@@ -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
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 2114bd2e27..54e95baad7 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -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;
 
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index b15f25d8f6..ce00603109 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -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 ();
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 6d85359118..873ab007fd 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -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,
diff --git a/nptl/descr.h b/nptl/descr.h
index a120365f88..a3084fdf60 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)
@@ -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[];
 
diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
index 14ccfe9285..6286b8b525 100644
--- a/nptl/libc-cleanup.c
+++ b/nptl/libc-cleanup.c
@@ -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)
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 48d48e7008..3e7a4f52ab 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -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)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index c11a2b4551..0588f086a8 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -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;
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index f842c91a08..7303069316 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
 	       & (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 e3696ca348..7e2b6e4974 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 5f061d512b..ae5df1d591 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 a9e941ddb7..920374643a 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 ();
+    }
 }
 versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
 versioned_symbol (libc, ___pthread_testcancel, __pthread_testcancel,