Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK

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

On 01/06/2021 06:03, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The CANCELING_BITMASK is used as an optimization to avoid sending
>> the signal when pthread_cancel is called in a concurrent manner.
>>
>> This requires then to put both the cancellation state and type on
>> a shared state (cancelhandling), since 'pthread_cancel' checks
>> whether cancellation is enabled and asynchrnous to either cancel
>> itself of sending the signal.
>>
>> It also requires handle the CANCELING_BITMASK on
>> __pthread_disable_asynccancel, however this is incurs in the same
> 
> typo: “this is incurs”

Ack.

> 
>> issues described on BZ#12683: the cancellation is acting even *after*
> 
> “is acted upon even *after*”?

Ack.

> 
>> the syscalls returns with user visible side-effects.
>>
>> This patch removes this optimization and simplifies the pthread
>> cancellation implementation: pthread_cancel now first check if
> 
> typo: check → checks

Ack.

> 
>> cancellation is already pending and if not always send a signal
> 
> typo: send → sends

Ack.

> 
> Maybe add a comma before “always”.

Ack.

> 
>> if the target is not itself.  The SIGCANCEL handler is also simpified
>> since there is not need to setup a CAS loop.
>>
>> It also alows to move both the cancellation state and mode out of
>> 'cancelhadling' (it is done in subsequent patches).
> 
> typo: alows

Ack.

> 
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index deb404600c..8dfbcff8c3 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
> 
>> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
>>  		    " must be installed for pthread_cancel to work\n");
>>    }
>> +
>> +  int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
>> +  if ((oldch & CANCELED_BITMASK) != 0)
>> +    return 0;
>> +
>> +  if (pd == THREAD_SELF)
>>      {
>> +      /* A single-threaded process should be able to kill itself, since there
>> +	 is nothing in the POSIX specification that says that it cannot.  So
>> +	 we set multiple_threads to true so that cancellation points get
>> +	 executed.  */
>> +      THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>>  #ifndef TLS_MULTIPLE_THREADS_IN_TCB
>> -	__libc_multiple_threads = 1;
>> +      __libc_multiple_threads = 1;
>>  #endif
>> +
>> +      THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
>> +      if ((oldch & CANCELTYPE_BITMASK) != 0)
>> +	__do_cancel ();
>> +      return 0;
>>      }
> 
> The last part is for asynchronous self-cancel, right?  Don't we have to
> check that cancellation is enabled, too?

Indeed it should be:

      if ((oldch & CANCELSTATE_BIT)
          && (oldch & CANCELTYPE_BITMASK) != 0)          

I ended up fixing it later in the serie with the cancel type and state 
refactor.  I have fixed it locally.