RE: [PATCH v1 1/4] ring: introduce extra run-time checks

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> > > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> > > > Sent: Wednesday, 21 May 2025 14.35
> > > >
> > > > > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> > > > > > Sent: Wednesday, 21 May 2025 13.14
> > > > > >
> > > > > > Add RTE_ASSERT() to check that different move_tail() flavors
> > > > > > return meaningful  *entries value.
> > > > > > It also helps to ensure that inside move_tail(), it uses
> > correct
> > > > > > head/tail values.
> > > > > >
> > > > > > Signed-off-by: Konstantin Ananyev
> > <konstantin.ananyev@huawei.com>
> > > > > > ---
> > > > > >  lib/ring/rte_ring_c11_pvt.h      | 2 +-
> > > > > >  lib/ring/rte_ring_elem_pvt.h     | 8 ++++++--
> > > > > >  lib/ring/rte_ring_hts_elem_pvt.h | 8 ++++++--
> > > > > >  lib/ring/rte_ring_rts_elem_pvt.h | 8 ++++++--
> > > > > >  lib/ring/soring.c                | 2 ++
> > > > > >  5 files changed, 21 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/ring/rte_ring_c11_pvt.h
> > > > b/lib/ring/rte_ring_c11_pvt.h
> > > > > > index b9388af0da..0845cd6dcf 100644
> > > > > > --- a/lib/ring/rte_ring_c11_pvt.h
> > > > > > +++ b/lib/ring/rte_ring_c11_pvt.h
> > > > > > @@ -104,10 +104,10 @@ __rte_ring_headtail_move_head(struct
> > > > > > rte_ring_headtail *d,
> > > > > >  			n = (behavior == RTE_RING_QUEUE_FIXED) ?
> > > > > >  					0 : *entries;
> > > > > >
> > > > > > +		*new_head = *old_head + n;
> > > > > >  		if (n == 0)
> > > > > >  			return 0;
> > > > > >
> > > > > > -		*new_head = *old_head + n;
> > > > > >  		if (is_st) {
> > > > > >  			d->head = *new_head;
> > > > > >  			success = 1;
> > > > >
> > > > > Is there a need to assign a value to *new_head if n==0?
> > > >
> > > > Not really, main reason I just moved this line up - to keep
> > compiler
> > > > happy.
> > > > Otherwise it complained that *new_head might be left uninitialized.
> > >
> > > Your change might give the impression that *new_head is used by a
> > caller. (Like I asked about.)
> > > To please the compiler, you could mark new_head __rte_unused, or:
> > >
> > > -		if (n == 0)
> > > +		if (n == 0) {
> > > +			RTE_SET_USED(new_head);
> > > 			return 0;
> > > +		}

Actually, that wouldn't help.
By some reason, after introducing RTE_ASSERT()  gcc13 believes that now cons_next can
be used (stored) unfinalized here:

        n = __rte_ring_move_cons_head(r, (int)is_sc, n, behavior,
                        &cons_head, &cons_next, &entries);
        if (n == 0)
                goto end;

        __rte_ring_dequeue_elems(r, cons_head, obj_table, esize, n);

        __rte_ring_update_tail(&r->cons, cons_head, cons_next, is_sc, 0);

end:
   ...

For me it is a false positive, somehow it missed that if (n==0) then update_table()
wouldn't be called  at all. Full error message below.
So making new_head always initialized, even if we are not going to use, seems
like the simplest and cleanest way to fix it.

est-pipeline_runtime.c.o -c ../app/test-pipeline/runtime.c
In file included from ../lib/eal/include/rte_bitops.h:24,
                 from ../lib/eal/include/rte_memory.h:18,
                 from ../app/test-pipeline/runtime.c:19:
In function '__rte_ring_update_tail',
    inlined from '__rte_ring_do_dequeue_elem' at ../lib/ring/rte_ring_elem_pvt.h:472:2,
    inlined from 'rte_ring_sc_dequeue_bulk_elem' at ../lib/ring/rte_ring_elem.h:344:9,
    inlined from 'rte_ring_sc_dequeue_bulk' at ../lib/ring/rte_ring.h:402:9,
    inlined from 'app_main_loop_worker' at ../app/test-pipeline/runtime.c:91:10:
../lib/eal/include/rte_stdatomic.h:139:9: error: 'cons_next' may be used uninitialized [-Werror=maybe-uninitialized]
  139 |         __atomic_store_n(ptr, val, memorder)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/ring/rte_ring_c11_pvt.h:39:9: note: in expansion of macro 'rte_atomic_store_explicit'
   39 |         rte_atomic_store_explicit(&ht->tail, new_val, rte_memory_order_release);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../lib/ring/rte_ring_elem.h:20,
                 from ../lib/ring/rte_ring.h:38,
                 from ../lib/mempool/rte_mempool.h:49,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ether.h:20,
                 from ../app/test-pipeline/runtime.c:31:
../lib/ring/rte_ring_elem_pvt.h: In function 'app_main_loop_worker':
../lib/ring/rte_ring_elem_pvt.h:462:29: note: 'cons_next' was declared here
  462 |         uint32_t cons_head, cons_next;
      |                             ^~~~~~~~~
In function '__rte_ring_update_tail',
    inlined from '__rte_ring_do_enqueue_elem' at ../lib/ring/rte_ring_elem_pvt.h:425:2,
    inlined from 'rte_ring_sp_enqueue_bulk_elem' at ../lib/ring/rte_ring_elem.h:159:9,
    inlined from 'rte_ring_sp_enqueue_bulk' at ../lib/ring/rte_ring.h:267:9,
    inlined from 'app_main_loop_worker' at ../app/test-pipeline/runtime.c:101:11 

> > >
> > > >
> >
> > Makes sense, will re-spin.


> I'm having second thoughts about treating this compiler warning as a false positive!
> 
> Are you 100 % sure that no caller uses *new_head?
 
Yes, I believe so. If not, then we do have a severe bug in our rt_ring,

> I suppose you are, because it wasn't set before this patch either, so the existing code would have a bug if some caller uses it.
> But the documentation does not mention that *new_head is not set if the function returns 0.
> So, some future caller might use *new_head, thus introducing a bug when n==0.
> 
> But most importantly for the performance discussion, the costly CAS is only pointless when n==0.
> So, if n==0 is very unlikely, we could accept this pointless cost.
> And it would save us the cost of "if (n==0) return 0;" in the hot code path.
> 
> And, as a consequence, some of the callers of this function could also remove their special handing of
> __rte_ring_headtail_move_head() returning 0. (Likewise, only if a return value of 0 is unlikely, and the special handling has a cost in
> the hot cod path for non-zero return values.)

I don't think it is a good idea.
First of all about the cost - I suppose that situation when 'n==0' is not so uncommon:
There is a contention on the ring (many threads try to enqueue/dequeue) to/from it.
Doing unnecessary CAS() (when n==0) we introduce extra cache-snooping to already busy memory sub-system,
i.e. we slowing down not only current threads, but all other producers/consumers of the ring.
About removing extra branches: I don't think there would be many to remove.
Usually after move_head() finishes we have to do two operations:
__rte_ring_dequeue_elems() ;
__rte_ring_update_tail();
Both have to be performed only when 'n != 0'.
Regarding the doc for the move_head() function - sure it can be updated to note explicitly
that both *old_head and *new_head will contain up-to-date values only when return value
is not zero. 

> > > > > I don't think your suggestion is multi-thread safe.
> > > > > If d->head moves, the value in *new_head will be incorrect.
> > > >
> > > > If d->head moves, then *old_head will also be incorrect.
> > > > For that case we do have CAS() below, it will return zero if (d-
> > >head
> > > > != *old_head)
> > > > and we shall go to the next iteration (attempt).
> > >
> > > Exactly.
> > > And with my suggestion the same will happen if n==0, and the next
> > attempt will update them both, until they are both correct.
> > >
> > > > Basically - if n == 0, your *old_head and *new_head might be
> > invalid
> > > > and should not be used
> > > > (and they are not used).
> > > >
> > > > > Instead, suggest:
> > > > >
> > > > > -		if (n == 0)
> > > > > -			return 0;
> > > > >
> > > > > 		*new_head = *old_head + n;
> > > > > 		if (is_st) {
> > > > > 			d->head = *new_head;
> > > > > 			success = 1;
> > > > > 		} else
> > > > > 			/* on failure, *old_head is updated */
> > > > > 			success =
> > > > rte_atomic_compare_exchange_strong_explicit(
> > > > > 					&d->head, old_head, *new_head,
> > > > > 					rte_memory_order_relaxed,
> > > > > 					rte_memory_order_relaxed);
> > > > > 	} while (unlikely(success == 0));
> > > >
> > > > That's possible, but if (n ==0) we probably don't want to update
> > the
> > > > head -
> > > > as we are not moving head - it is pointless, while still expensive.
> > >
> > > Agree. Let's avoid unnecessary cost!
> > > My suggestion was only relevant if *new_head needed to be updated
> > when n==0.
> > >
>