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().
