Re: [lttng-dev] [PATCH 2/7] Use gcc __atomic builtis for <urcu/uatomic.h> implementation

[Mathieu Desnoyers via lttng-dev <lttng-dev@lists.lttng.org>]

On 2023-03-20 14:38, Mathieu Desnoyers via lttng-dev wrote:
> On 2023-03-20 14:28, Ondřej Surý wrote:
>>
>>> On 20. 3. 2023, at 19:03, Mathieu Desnoyers 
>>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> In doc/uatomic-api.md, we document:
>>>
>>> "```c
>>> type uatomic_cmpxchg(type *addr, type old, type new);
>>> ```
>>>
>>> An atomic read-modify-write operation that performs this
>>> sequence of operations atomically: check if `addr` contains `old`.
>>> If true, then replace the content of `addr` by `new`. Return the
>>> value previously contained by `addr`. This function implies a full
>>> memory barrier before and after the atomic operation."
>>>
>>> This would map to a "__ATOMIC_ACQ_REL" semantic on cmpxchg failure
>>> rather than __ATOMIC_CONSUME".
>>
>>
>> From: https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>>
>>> If desired is written into *ptr then true is returned and memory is 
>>> affected according to the memory order specified by success_memorder. 
>>> There are no restrictions on what memory order can be used here.
>>>
>>> Otherwise, false is returned and memory is affected according to 
>>> failure_memorder. This memory order cannot be __ATOMIC_RELEASE nor 
>>> __ATOMIC_ACQ_REL. It also cannot be a stronger order than that 
>>> specified by success_memorder.
>>
>> I think it makes sense that the failure_memorder has the same memorder 
>> as uatomic_read(), but it definitelly cannot be __ATOMIC_ACQ_REL - 
>> it's same as with __atomic_load_n, only following are permitted:
>>
>>> The valid memory order variants are __ATOMIC_RELAXED, 
>>> __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE, and __ATOMIC_CONSUME.
> 
> Based on my other reply, we want "SEQ_CST" rather than ACQ_REL everywhere.

And it _would_ make sense to use the same memorder on cmpxchg failure as 
uatomic_read if we were exposing a new API, but we are modifying an 
already exposed documented API, so I would stick to SEQ_CST for both 
cmpxchg success/failure.

If we want to expose a new cmpxchg_relaxed_failure with a relaxed 
memorder on failure that would be fine, but we cannot change the 
semantic that is already documented.

Thanks,

Mathieu

> 
> Thanks,
> 
> Mathieu
> 
>>
>> Ondrej
>> -- 
>> Ondřej Surý (He/Him)
>> ondrej@sury.org
>>
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev