Re: [PATCH 3/3] nptl: Move cancel type out of cancelhandling

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

Ping.

On 01/04/2020 11:24, Adhemerval Zanella wrote:
> The thread cancellation type is not accessed concurrently internally
> neither its pthread interface allows changing the state of a different
> thread than its own.
> 
> It allows simplifing the cancellation wrappers and the
> CANCEL_CANCELED_AND_ASYNCHRONOUS is removed.
> 
> Checked on x86_64-linux-gnu.
> ---
>  nptl/allocatestack.c         |  1 +
>  nptl/cancellation.c          | 59 ++++++++++--------------------------
>  nptl/cleanup_defer.c         | 46 +++-------------------------
>  nptl/cleanup_defer_compat.c  | 46 +++-------------------------
>  nptl/descr.h                 | 13 ++------
>  nptl/nptl-init.c             |  2 +-
>  nptl/pthread_cancel.c        |  5 ++-
>  nptl/pthread_setcanceltype.c | 42 +++----------------------
>  nptl/pthread_testcancel.c    |  2 +-
>  9 files changed, 41 insertions(+), 175 deletions(-)
> 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index d9174274c2..481487bbb4 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -233,6 +233,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->canceltype = PTHREAD_CANCEL_DEFERRED;
>    result->cleanup = NULL;
>  
>    /* No pending event.  */
> diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> index 7e8cbe9fe1..7127f9ae91 100644
> --- a/nptl/cancellation.c
> +++ b/nptl/cancellation.c
> @@ -32,31 +32,18 @@ attribute_hidden
>  __pthread_enable_asynccancel (void)
>  {
>    struct pthread *self = THREAD_SELF;
> -  int oldval = THREAD_GETMEM (self, cancelhandling);
>  
> -  while (1)
> -    {
> -      int newval = oldval | CANCELTYPE_BITMASK;
> -
> -      if (newval == oldval)
> -	break;
> -
> -      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> -					      oldval);
> -      if (__glibc_likely (curval == oldval))
> -	{
> -	  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> -	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> -	    {
> -	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> -	      __do_cancel ();
> -	    }
> +  int oldval = THREAD_GETMEM (self, canceltype);
> +  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
>  
> -	  break;
> -	}
> +  int ch = THREAD_GETMEM (self, cancelhandling);
>  
> -      /* Prepare the next round.  */
> -      oldval = curval;
> +  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> +      && (ch & (CANCELED_BITMASK | EXITING_BITMASK | TERMINATED_BITMASK))
> +	  == CANCELED_BITMASK)
> +    {
> +      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> +      __do_cancel ();
>      }
>  
>    return oldval;
> @@ -70,36 +57,22 @@ __pthread_disable_asynccancel (int oldtype)
>  {
>    /* If asynchronous cancellation was enabled before we do not have
>       anything to do.  */
> -  if (oldtype & CANCELTYPE_BITMASK)
> +  if (oldtype == PTHREAD_CANCEL_ASYNCHRONOUS)
>      return;
>  
>    struct pthread *self = THREAD_SELF;
> -  int newval;
> -
> -  int oldval = THREAD_GETMEM (self, cancelhandling);
> -
> -  while (1)
> -    {
> -      newval = oldval & ~CANCELTYPE_BITMASK;
> -
> -      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> -					      oldval);
> -      if (__glibc_likely (curval == oldval))
> -	break;
> -
> -      /* Prepare the next round.  */
> -      oldval = curval;
> -    }
> +  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>  
>    /* 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))
> +  int ch = THREAD_GETMEM (self, cancelhandling);
> +  while (__glibc_unlikely ((ch & (CANCELING_BITMASK | CANCELED_BITMASK))
> +			    == CANCELING_BITMASK))
>      {
> -      futex_wait_simple ((unsigned int *) &self->cancelhandling, newval,
> +      futex_wait_simple ((unsigned int *) &self->cancelhandling, ch,
>  			 FUTEX_PRIVATE);
> -      newval = THREAD_GETMEM (self, cancelhandling);
> +      ch = THREAD_GETMEM (self, cancelhandling);
>      }
>  }
> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> index 33d4ea6eef..3300b4df46 100644
> --- a/nptl/cleanup_defer.c
> +++ b/nptl/cleanup_defer.c
> @@ -31,27 +31,9 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
>    ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
>    ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
>  
> -  int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> -
>    /* Disable asynchronous cancellation for now.  */
> -  if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> -    while (1)
> -      {
> -	int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> -						cancelhandling
> -						& ~CANCELTYPE_BITMASK,
> -						cancelhandling);
> -	if (__glibc_likely (curval == cancelhandling))
> -	  /* Successfully replaced the value.  */
> -	  break;
> -
> -	/* Prepare for the next round.  */
> -	cancelhandling = curval;
> -      }
> -
> -  ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
> -				? PTHREAD_CANCEL_ASYNCHRONOUS
> -				: PTHREAD_CANCEL_DEFERRED);
> +  ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
> +  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>  
>    /* Store the new cleanup handler info.  */
>    THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
> @@ -67,25 +49,7 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
>  
>    THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
>  
> -  int cancelhandling;
> -  if (ibuf->priv.data.canceltype != PTHREAD_CANCEL_DEFERRED
> -      && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
> -	  & CANCELTYPE_BITMASK) == 0)
> -    {
> -      while (1)
> -	{
> -	  int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> -						  cancelhandling
> -						  | CANCELTYPE_BITMASK,
> -						  cancelhandling);
> -	  if (__glibc_likely (curval == cancelhandling))
> -	    /* Successfully replaced the value.  */
> -	    break;
> -
> -	  /* Prepare for the next round.  */
> -	  cancelhandling = curval;
> -	}
> -
> -      __pthread_testcancel ();
> -    }
> +  THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
> +  if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> +    __pthread_testcancel ();
>  }
> diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
> index a1ad291fcc..77655c1b0a 100644
> --- a/nptl/cleanup_defer_compat.c
> +++ b/nptl/cleanup_defer_compat.c
> @@ -29,27 +29,9 @@ _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
>    buffer->__arg = arg;
>    buffer->__prev = THREAD_GETMEM (self, cleanup);
>  
> -  int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> -
>    /* Disable asynchronous cancellation for now.  */
> -  if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> -    while (1)
> -      {
> -	int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> -						cancelhandling
> -						& ~CANCELTYPE_BITMASK,
> -						cancelhandling);
> -	if (__glibc_likely (curval == cancelhandling))
> -	  /* Successfully replaced the value.  */
> -	  break;
> -
> -	/* Prepare for the next round.  */
> -	cancelhandling = curval;
> -      }
> -
> -  buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> -			  ? PTHREAD_CANCEL_ASYNCHRONOUS
> -			  : PTHREAD_CANCEL_DEFERRED);
> +  buffer->__canceltype = THREAD_GETMEM (self, canceltype);
> +  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>  
>    THREAD_SETMEM (self, cleanup, buffer);
>  }
> @@ -64,27 +46,9 @@ _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
>  
>    THREAD_SETMEM (self, cleanup, buffer->__prev);
>  
> -  int cancelhandling;
> -  if (__builtin_expect (buffer->__canceltype != PTHREAD_CANCEL_DEFERRED, 0)
> -      && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
> -	  & CANCELTYPE_BITMASK) == 0)
> -    {
> -      while (1)
> -	{
> -	  int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
> -						  cancelhandling
> -						  | CANCELTYPE_BITMASK,
> -						  cancelhandling);
> -	  if (__glibc_likely (curval == cancelhandling))
> -	    /* Successfully replaced the value.  */
> -	    break;
> -
> -	  /* Prepare for the next round.  */
> -	  cancelhandling = curval;
> -	}
> -
> -      __pthread_testcancel ();
> -    }
> +  THREAD_SETMEM (self, canceltype, buffer->__canceltype);
> +  if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> +    __pthread_testcancel ();
>  
>    /* If necessary call the cleanup routine after we removed the
>       current cleanup block from the list.  */
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 61665bf859..bae9457e33 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 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)
> @@ -287,13 +284,6 @@ struct pthread
>    /* Bit set if thread is supposed to change XID.  */
>  #define SETXID_BIT		6
>  #define SETXID_BITMASK		(0x01 << SETXID_BIT)
> -  /* Mask for the rest.  Helps the compiler to optimize.  */
> -#define CANCEL_RESTMASK		0xffffff80
> -
> -#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \
> -  (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK    \
> -	       | EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK))     \
> -   == (CANCELTYPE_BITMASK | CANCELED_BITMASK))
>  
>    /* Flags.  Including those copied from the thread attribute.  */
>    int flags;
> @@ -391,6 +381,9 @@ struct pthread
>    /* Thread cancel state (enable, disable).  */
>    unsigned char cancelstate;
>  
> +  /* Thread cancel type (deferred, asynchronous).  */
> +  unsigned char canceltype;
> +
>    /* This member must be last.  */
>    char end_padding[];
>  
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 96b1444a01..89f58d2e5e 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -157,7 +157,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
>  	  THREAD_SETMEM (self, result, PTHREAD_CANCELED);
>  
>  	  /* Make sure asynchronous cancellation is still enabled.  */
> -	  if ((newval & CANCELTYPE_BITMASK) != 0)
> +	  if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
>  	    /* Run the registered destructors and terminate the thread.  */
>  	    __do_cancel ();
>  
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 5b2789d620..7518c0b8bf 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -56,7 +56,10 @@ __pthread_cancel (pthread_t th)
>  	 signal.  We avoid this if possible since it's more
>  	 expensive.  */
>        if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> -	  && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> +	  && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS
> +	  && (newval & (CANCELED_BITMASK | EXITING_BITMASK
> +			| TERMINATED_BITMASK))
> +	      == CANCELED_BITMASK)
>  	{
>  	  /* Mark the cancellation as "in progress".  */
>  	  if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index cc0507ae04..d8cb54736d 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -29,43 +29,11 @@ __pthread_setcanceltype (int type, int *oldtype)
>  
>    volatile struct pthread *self = THREAD_SELF;
>  
> -  int oldval = THREAD_GETMEM (self, cancelhandling);
> -  while (1)
> -    {
> -      int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS
> -		    ? oldval | CANCELTYPE_BITMASK
> -		    : oldval & ~CANCELTYPE_BITMASK);
> -
> -      /* Store the old value.  */
> -      if (oldtype != NULL)
> -	*oldtype = ((oldval & CANCELTYPE_BITMASK)
> -		    ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
> -
> -      /* 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 (self->cancelstate == PTHREAD_CANCEL_ENABLE
> -	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> -	    {
> -	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> -	      __do_cancel ();
> -	    }
> -
> -	  break;
> -	}
> -
> -      /* Prepare for the next round.  */
> -      oldval = curval;
> -    }
> +  if (oldtype != NULL)
> +    *oldtype = self->canceltype;
> +  self->canceltype = type;
> +  if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
> +    __pthread_testcancel ();
>  
>    return 0;
>  }
> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 3ffff4ebef..026c20f82e 100644
> --- a/nptl/pthread_testcancel.c
> +++ b/nptl/pthread_testcancel.c
> @@ -34,5 +34,5 @@ __pthread_testcancel (void)
>        __do_cancel ();
>      }
>  }
> -strong_alias (__pthread_testcancel, pthread_testcancel)
> +weak_alias (__pthread_testcancel, pthread_testcancel)
>  hidden_def (__pthread_testcancel)
>