Re: [lttng-dev] userspace-rcu and ThreadSanitizer

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

On 2023-03-17 13:02, Ondřej Surý wrote:
>> On 17. 3. 2023, at 14:44, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>> I would indeed like to remove all the custom atomics assembly code from liburcu now that there are good atomics support in the major compilers (gcc and clang).
> 
> Here's very preliminary implementation:
> 
> https://gitlab.isc.org/isc-projects/userspace-rcu/-/merge_requests/2
> 
> I just did something wrong somewhere along the path and it doesn't compile now,
> but it did for me locally.
> 
> I am submitting this now as it's 18:00 Friday evening and my kids are starting to
> be angry at me :).
> 
> This will need some more work - I think some of the cmm_ macros might be dropped
> now, and somebody who does that more often than I should take a look at the memory
> orderings.

A few comments:

cmm_barrier() should rather be __atomic_signal_fence().

Also I notice this macro pattern (coding style):

#define uatomic_set(addr, v) __atomic_store_n((addr), (v), __ATOMIC_RELEASE)

The extra parentheses for parameters are not needed, because the comma is pretty
much the last operator in terms of priority. The following would be preferred
specifically because those are separated by comma:

#define uatomic_set(addr, v) __atomic_store_n(addr, v, __ATOMIC_RELEASE)

Our memory barrier semantic are similar to the Linux kernel, where the following
imply ACQ_REL because they return something: cmpxchg, add_return, sub_return, xchg.

The rest (add, sub, and, or, inc, dec) are __ATOMIC_RELAXED. Note that
cmm_smp_mb__before/after_uatomic_*() need to be implemented as
__atomic_thread_fence(__ATOMIC_ACQ_REL).

There are some architectures where we will want to keep a specialized version
of those add, sub, and, or, inc, dec operations which include the ACQ_REL semantic,
e.g. x86, where this is implied by the LOCK prefix. For those the cmm_smp_mb__before/after_uatomic_*()
will be no-ops.

The CMM_STORE_SHARED is not meant to have a RELEASE semantic. It is meant to
update variables that don't need the release ordering. The ATOMIC_CONSUME was
not the intent at the CMM_LOAD_SHARED level neither.

(this is just from looking around at the patches, it would be better if we can have the
patches posted to the mailing list for further discussion)

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