> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com]
> Sent: Wednesday, 21 May 2025 13.15
>
> This patch aims several purposes:
> - provide an alternative (and I think a better) way to fix the
> issue discussed in previous patch:
> "ring/soring: fix synchronization issue between head and tail values"
> - make sure that such problem wouldn’t happen within other usages of
> __rte_ring_headtail_move_head() – both current rte_ring
> implementation and possible future use-cases.
> - step towards unification of move_head() implementations and
> removing rte_ring_generic_pvt.h
> It uses Acquire-Release memory ordering for CAS operation in
> move_head().
> That guarantees that corresponding ‘tail’ updates will be visible
> before current ‘head’ is updated.
> As I said before: I think that in theory the problem described in
> previous patch might happen with our conventional rte_ring too
> (when RTE_USE_C11_MEM_MODEL enabled).
> But, so far I didn’t manage to reproduce it in reality.
Overall, I think the code becomes more elegant and much easier to understand
with this patch, where the atomic operations are performed explicitly on the
head/tail, eliminating the need for the broad-reaching
rte_atomic_thread_fence().
The detailed inline code comments are also a good improvement.
> For that reason and also because it touches a critical rte_ring code-
> path,
> I put these changes into a separate patch. Expect all interested
> stakeholders to come-up with their comments and observations.
> Regarding performance impact – on my boxes both ring_perf_autotest and
> ring_stress_autotest – show a mixed set of results: some of them become
> few cycles faster, another few cycles slower.
> But so far, I didn’t notice any real degradations with that patch.
Maybe it was the broad-reaching rte_atomic_thread_fence() that made the C11
variant slow on other architectures.
This makes me curious about performance results on other architectures with
this patch.
>
> Fixes: b5458e2cc483 ("ring: introduce staged ordered ring")
> Fixes: 1cc363b8ce06 ("ring: introduce HTS ring mode")
> Fixes: e6ba4731c0f3 ("ring: introduce RTS ring mode")
> Fixes: 49594a63147a ("ring/c11: relax ordering for load and store of
> the head")
>
> Signed-off-by: Konstantin Ananyev <konstantin.anan...@huawei.com>
> ---
> lib/ring/rte_ring_c11_pvt.h | 27 +++++++++++++++++----------
> lib/ring/rte_ring_hts_elem_pvt.h | 6 ++++--
> lib/ring/rte_ring_rts_elem_pvt.h | 6 ++++--
> lib/ring/soring.c | 5 -----
> 4 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/lib/ring/rte_ring_c11_pvt.h b/lib/ring/rte_ring_c11_pvt.h
> index 0845cd6dcf..6d1c46df9a 100644
> --- a/lib/ring/rte_ring_c11_pvt.h
> +++ b/lib/ring/rte_ring_c11_pvt.h
> @@ -77,20 +77,19 @@ __rte_ring_headtail_move_head(struct
> rte_ring_headtail *d,
> int success;
> unsigned int max = n;
>
> + /* Ensure the head is read before tail */
Maybe "d->head" and "s->tail" instead of "head" and "tail".
> *old_head = rte_atomic_load_explicit(&d->head,
> - rte_memory_order_relaxed);
> + rte_memory_order_acquire);
> do {
> /* Reset n to the initial burst count */
> n = max;
>
> - /* Ensure the head is read before tail */
> - rte_atomic_thread_fence(rte_memory_order_acquire);
> -
> - /* load-acquire synchronize with store-release of ht->tail
> - * in update_tail.
> + /*
> + * Read s->tail value. Note that it will be loaded after
> + * d->head load, but before CAS operation for the d->head.
> */
> stail = rte_atomic_load_explicit(&s->tail,
> - rte_memory_order_acquire);
> + rte_memory_order_relaxed);
>
> /* The subtraction is done between two unsigned 32bits
> value
> * (the result is always modulo 32 bits even if we have
> @@ -112,11 +111,19 @@ __rte_ring_headtail_move_head(struct
> rte_ring_headtail *d,
> d->head = *new_head;
> success = 1;
> } else
> - /* on failure, *old_head is updated */
> + /*
> + * on failure, *old_head is updated.
> + * this CAS(ACQ_REL, ACQUIRE) serves as a hoist
> + * barrier to prevent:
> + * - OOO reads of cons tail value
> + * - OOO copy of elems from the ring
It's not really the ACQ_REL that does this. It's the AQUIRE.
So this comment needs some adjustment.
Also maybe "s->tail" instead of "cons tail".
> + * Also RELEASE guarantees that latest tail value
Maybe "latest s->tail" instead of "latest tail".
> + * will become visible before the new head value.
Maybe "new d->head value" instead of "new head value".
> + */
> success =
> rte_atomic_compare_exchange_strong_explicit(
> &d->head, old_head, *new_head,
> - rte_memory_order_relaxed,
> - rte_memory_order_relaxed);
> + rte_memory_order_acq_rel,
> + rte_memory_order_acquire);
> } while (unlikely(success == 0));
> return n;
> }
I haven't reviewed the remaining changes in detail, but I think my feedback
that the comments should mention ACQUIRE instead of ACQ_REL also apply to the
other files.
> diff --git a/lib/ring/rte_ring_hts_elem_pvt.h
> b/lib/ring/rte_ring_hts_elem_pvt.h
> diff --git a/lib/ring/rte_ring_rts_elem_pvt.h
> b/lib/ring/rte_ring_rts_elem_pvt.h
> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
