Re: [lttng-dev] [PATCH 3/7] Use __atomic_thread_fence() for cmm_barrier()

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

On 2023-03-20 14:06, Mathieu Desnoyers via lttng-dev wrote:
> On 2023-03-17 17:37, Ondřej Surý via lttng-dev wrote:
>> Use __atomic_thread_fence(__ATOMIC_ACQ_REL) for cmm_barrier(), so
>> ThreadSanitizer can understand the memory synchronization.
> 
> You should update the patch subject and commit message to replace 
> "thread" by "signal".
> 
>>
>> FIXME: What should be the correct memory ordering here?
> 
> ACQ_REL is what we want here, I think this is fine. We want to prevent
> the compiler from reordering loads/stores across the fence, but don't
> want any barrier instructions issued.

We should probably make it SEQ_CST here as well, even though I doubt it 
changes anything in this very particular case of atomic_signal_fence.

Thanks,

Mathieu

> 
> Thanks,
> 
> Mathieu
> 
>>
>> Signed-off-by: Ondřej Surý <ondrej@sury.org>
>> ---
>>   include/urcu/compiler.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/urcu/compiler.h b/include/urcu/compiler.h
>> index 2f32b38..ede909f 100644
>> --- a/include/urcu/compiler.h
>> +++ b/include/urcu/compiler.h
>> @@ -28,7 +28,8 @@
>>   #define caa_likely(x)    __builtin_expect(!!(x), 1)
>>   #define caa_unlikely(x)    __builtin_expect(!!(x), 0)
>> -#define    cmm_barrier()    __asm__ __volatile__ ("" : : : "memory")
>> +/* FIXME: What would be a correct memory ordering here? */
>> +#define    cmm_barrier()    __atomic_signal_fence(__ATOMIC_ACQ_REL)
>>   /*
>>    * Instruct the compiler to perform only a single access to a variable
> 

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