Re: [PATCH] Use C11 atomics instead of atomic_increment(_val)

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

On 22/09/22 10:27, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
> In general you only need acquire/release for locks that synchronize access to
> shared data which does not use atomic accesses. If there is no such shared
> data, there is no need for acquire/release. Similarly only using acquire and never
> release (as the old atomics did) makes no sense - you need a release atomic
> to synchronize with.

Alright, I agree that default to acquire MO as the most atomic does today does
not make much sense.

> 
>> I am not sure if relaxed MO is correct here, shouldn't it synchronize with the
>> __nptl_setxid_sighandler ?
> 
> The counter cntr in nptl/nptl_setxid.c is just a simple atomic counter to keep
> track of the number of outstanding signals that were sent to threads. There is
> no data it is trying to synchronize with, so release/acquire semantics do not
> make sense here.

Ack.

> 
>> Not sure if this is correct for hurd, __pthread_total is used on __pthread_exit
>> to call exit although for i686 atomic_decrement_and_test will be used and it
>> has strong MO.
> 
> The same is true for pthread_total counter, it is used to call exit when the last
> thread stops. Note while the increment is done optimistically (thread creation
> can fail, and then it is decremented again), the decrement resulting in a call to
> exit can only happen once even if multiple threads are finishing concurrently
> (and that happen once is what is required). 

Ack.

> 
> Cheers,
> Wilco
> 

The patch looks ok then.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> 
>> diff --git a/htl/pt-create.c b/htl/pt-create.c
>> index ce52ed9f52210a4e4c7a049ebee817ec9ccfeeb1..14f02cd2b8a19e8581a170dfba2b948ef8304203 100644
>> --- a/htl/pt-create.c
>> +++ b/htl/pt-create.c
>> @@ -228,7 +228,7 @@ __pthread_create_internal (struct __pthread **thread,
>>        the number of threads from within the new thread isn't an option
>>        since this thread might return and call `pthread_exit' before the
>>        new thread runs.  */
>> -  atomic_increment (&__pthread_total);
>> +  atomic_fetch_add_relaxed (&__pthread_total, 1);
>>   
>>     /* Store a pointer to this thread in the thread ID lookup table.  We
>>        could use __thread_setid, however, we only lock for reading as no
> 
> Not sure if this is correct for hurd, __pthread_total is used on __pthread_exit
> to call exit although for i686 atomic_decrement_and_test will be used and it
> has strong MO.
> 
> 
>> diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
>> index aa863c7ea8122ea01d1aa4cffe101bbb7c11270c..3b7e2d434abe8a15145349d1a08a4e706061c74d 100644
>> --- a/nptl/nptl_setxid.c
>> +++ b/nptl/nptl_setxid.c
>> @@ -163,7 +163,7 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
>>     /* If this failed, it must have had not started yet or else exited.  */
>>     if (!INTERNAL_SYSCALL_ERROR_P (val))
>>       {
>> -      atomic_increment (&cmdp->cntr);
>> +      atomic_fetch_add_relaxed (&cmdp->cntr, 1);
>>         return 1;
>>       }
>>     else
> 
> I am not sure if relaxed MO is correct here, shouldn't it synchronize with the
> __nptl_setxid_sighandler ?