[dpdk-dev] [PATCH v4 4/4] eal/atomic: add wrapper for c11 atomics
Ananyev, Konstantin
konstantin.ananyev at intel.com
Wed May 13 13:53:58 CEST 2020
> <snip>
>
> Subject: Re: [PATCH v4 4/4] eal/atomic: add wrapper for c11 atomics
>
> On Tue, May 12, 2020 at 4:03 pm, Phil Yang <mailto:phil.yang at arm.com> wrote:
>
> parameter. Signed-off-by: Phil Yang <mailto:phil.yang at arm.com>
>
>
> What is the purpose of having rte_atomic at all?
> Is this level of indirection really helping?
> [HONNAPPA] (not sure why this email has html format, converted to text format)
> I believe you meant, why not use the __atomic_xxx built-ins directly? The only reason for now is handling of
> __atomic_thread_fence(__ATOMIC_SEQ_CST) for x86. This is equivalent to rte_smp_mb which has an optimized implementation for x86.
> According to Konstantin, the compiler does not generate optimal code. Wrapping that built-in alone is going to be confusing.
>
> The wrappers also allow us to have our own implementation using inline assembly for compilers versions that do not support C11 atomic
> built-ins. But, I do not know if there is a need to support those versions.
Few thoughts from my side about that patch:
Yes, for __atomic_thread_fence(__ATOMIC_SEQ_CST) generates full 'mfence' which is quite expensive,
and can be a avoided for SMP case.
Though I don't see why we need to create our own wrappers for *all* __atomic buitins.
>From my perspective it would be sufficient to just introduce few of them:
rte_thread_fence_XXX (where XXX - supported memory-orders: RELEASE, ACUIQRE, SEQ_CST, etc.).
For all other __atomic built-ins I don't see any problem to use them directly,
without introducing any wrappers around.
As a side note, this patch implements rte_atomic_thread_fence() as a simple wrapper around
__atomic_thread_fence(), so concern mentioned above is not addressed.
More information about the dev
mailing list