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