> > > > 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; > > > + } 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. > > > >
