RE: [PATCH v1 3/4] ring: fix potential sync issue between head and tail values

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@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.ananyev@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