Re: [PATCH 0/5] use rte atomic thread fence

[Thomas Monjalon <thomas@monjalon.net>]

08/11/2023 19:49, Tyler Retzlaff:
> On Wed, Nov 08, 2023 at 06:04:47PM +0100, Thomas Monjalon wrote:
> > 02/11/2023 04:04, Tyler Retzlaff:
> > > Replace use of __atomic_thread_fence with __rte_atomic_thread_fence.
> > > 
> > > It may be appropriate to use rte_atomic_thread_fence instead but it
> > > will be up to maintainers to evaluate and make the change if appropriate.
> > 
> > I don't understand the use of __rte_atomic_thread_fence
> > which is supposed to be EAL-internal only, isn't it?
> > 
> > On x86, we have this:
> > static __rte_always_inline void
> > rte_atomic_thread_fence(rte_memory_order memorder)
> > {
> >     if (memorder == rte_memory_order_seq_cst)
> >         rte_smp_mb();
> >     else
> >         __rte_atomic_thread_fence(memorder);
> > }
> > 
> > This is skipped if you use __rte_atomic_thread_fence() directly.
> 
> correct. that is on purpose because the original code was skipping
> condition by using __atomic_thread_fence directly.

There is chance that it was not skipping on purpose.

> this series intends no functional change which is why the replacements
> are __rte_atomic_thread_fence instead of to rte_atomic_thread_fence.

We should take this opportunity to simplify the code,
I mean we should avoid having 3 functions for the same thing.

> i would encourage the maintainers to evaluate whether the code can use
> rte_atomic_thread_fence directly without functional regression.

Let's do the change so the maintainers can review what is changed.
Note it should have no impact if not using rte_memory_order_seq_cst.
If we discover later that something is broken, we have time to fix it.

Note: lib/eal/include/rte_mcslock.h should not use __rte_atomic_thread_fence()
and lib/lpm/rte_lpm.c should not use __atomic_thread_fence().

Can we replace and discourage all occurrences of
__atomic_thread_fence() and __rte_atomic_thread_fence()?
It would be simpler to recommend only rte_atomic_thread_fence().

Also in another patch, it would be nice to replace __ATOMIC_*
with rte_memory_order_*, at least when used in rte_atomic_thread_fence().