[0/5] use rte atomic thread fence

  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.

Tyler Retzlaff (5):
  distributor: use rte atomic thread fence
  eal: use rte atomic thread fence
  hash: use rte atomic thread fence
  ring: use rte atomic thread fence
  stack: use rte atomic thread fence

 lib/distributor/rte_distributor.c |  2 +-
 lib/eal/common/eal_common_trace.c |  2 +-
 lib/hash/rte_cuckoo_hash.c        | 10 +++++-----
 lib/ring/rte_ring_c11_pvt.h       |  4 ++--
 lib/stack/rte_stack_lf_c11.h      |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)
  

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 2 November 2023 04.04
> 
> 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.
> 
> Tyler Retzlaff (5):
>   distributor: use rte atomic thread fence
>   eal: use rte atomic thread fence
>   hash: use rte atomic thread fence
>   ring: use rte atomic thread fence
>   stack: use rte atomic thread fence
> 
>  lib/distributor/rte_distributor.c |  2 +-
>  lib/eal/common/eal_common_trace.c |  2 +-
>  lib/hash/rte_cuckoo_hash.c        | 10 +++++-----
>  lib/ring/rte_ring_c11_pvt.h       |  4 ++--
>  lib/stack/rte_stack_lf_c11.h      |  2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> --
> 1.8.3.1

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
  

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.
  

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.

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

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

ty
  

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