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

[Adhemerval Zanella]

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.