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

[Florian Weimer <fw@deneb.enyo.de>]

* 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.