> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com]
> Sent: Wednesday, 21 May 2025 21.39
>
> > > From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com]
> > > Sent: Wednesday, 21 May 2025 14.35
> > >
> > > > > From: Konstantin Ananyev [mailto:konstantin.anan...@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.anan...@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;
> > + }
> >
> > >
>
> 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?
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.)
> Do you have any comments for other patches in the series?
> Thanks
> Konstantin
>
>
> > > > 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.
> >
