|SUCCESS| [v1,2/2] ring: introduce peek API for soring

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163101

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-15

git sha: 20260415171626.161370-2-konstantin.ananyev at huawei.com
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>
Subject: [PATCH v1 1/2] ring: make soring to finalize its own stage only

This patch removes the finalization call from acquire and dequeue paths,
making release() responsible for finalizing its own stage only. The
change aims to reduce contention in multi-worker scenarios.

> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
> index 3b90521bdb..fc7fc55a21 100644
> --- a/lib/ring/soring.c
> +++ b/lib/ring/soring.c
> @@ -50,10 +50,8 @@
>   * from current stage tail up to its head, check state[] and move stage tail
>   * through elements that already are in SORING_ST_FINISH state.
>   * Along with that, corresponding state[] values are reset to zero.
> - * Note that 'finalize()' for given stage can be done from multiple places:
> - * 'release()' for that stage or from 'acquire()' for next stage
> - * even from consumer's 'dequeue()' - in case given stage is the last one.
> - * So 'finalize()' has to be MT-safe and inside it we have to
> + * Note that 'finalize()' for given stage can be called from multiple threads
> + * in parallel, so 'finalize()' has to be MT-safe and inside it we have to
>   * guarantee that only one thread will update state[] and stage's tail values.
>   */

[ ... ]

> @@ -293,22 +291,8 @@ soring_dequeue(struct rte_soring *r, void *objs, void *meta,
>  	st = r->cons.ht.sync_type;
>  
>  	/* try to grab exactly @num elems first */
> -	n = __rte_soring_move_cons_head(r, ns, num, RTE_RING_QUEUE_FIXED, st,
> +	n = __rte_soring_move_cons_head(r, ns, num, behavior, st,
>  			&cons_head, &cons_next, &entries);

Does this change the behavior of soring_dequeue() when it's called with
RTE_RING_QUEUE_FIXED? Previously it would try to finalize and retry if
the initial grab failed, but now it immediately changes to using the
'behavior' parameter. Was this change intentional, or should the first
attempt still use RTE_RING_QUEUE_FIXED?

> -	if (n == 0) {
> -		/* try to finalize some elems from previous stage */
> -		n = __rte_soring_stage_finalize(&r->stage[ns].sht, ns,
> -			r->state, r->mask, 2 * num);
> -		entries += n;
> -
> -		/* repeat attempt to grab elems */
> -		reqn = (behavior == RTE_RING_QUEUE_FIXED) ? num : 0;
> -		if (entries >= reqn)
> -			n = __rte_soring_move_cons_head(r, ns, num, behavior,
> -				st, &cons_head, &cons_next, &entries);
> -		else
> -			n = 0;
> -	}

[ ... ]

> @@ -399,22 +383,7 @@ soring_acquire(struct rte_soring *r, void *objs, void *meta,
>  
>  		/* try to grab exactly @num elems */
>  		n = __rte_soring_stage_move_head(cons, &pstg->ht, 0, num,
> -			RTE_RING_QUEUE_FIXED, &head, &next, &avail);
> -		if (n == 0) {
> -			/* try to finalize some elems from previous stage */
> -			n = __rte_soring_stage_finalize(&pstg->sht, stage - 1,
> -				r->state, r->mask, 2 * num);
> -			avail += n;
> -
> -			/* repeat attempt to grab elems */
> -			reqn = (behavior == RTE_RING_QUEUE_FIXED) ? num : 0;
> -			if (avail >= reqn)
> -				n = __rte_soring_stage_move_head(cons,
> -					&pstg->ht, 0, num, behavior, &head,
> -					&next, &avail);
> -			else
> -				n = 0;
> -		}
> +			behavior, &head, &next, &avail);

The same question applies here: does this remove the retry behavior
with finalization?

> @@ -442,7 +411,7 @@ static __rte_always_inline void
>  soring_release(struct rte_soring *r, const void *objs,
>  	const void *meta, uint32_t stage, uint32_t n, uint32_t ftoken)
>  {
> -	uint32_t idx, pos, tail;
> +	uint32_t idx, pos;
>  	struct soring_stage *stg;
>  	union soring_state st;
>  
> @@ -479,11 +448,8 @@ soring_release(struct rte_soring *r, const void *objs,
>  	rte_atomic_store_explicit(&r->state[idx].raw, st.raw,
>  			rte_memory_order_relaxed);
>  
> -	/* try to do finalize(), if appropriate */
> -	tail = rte_atomic_load_explicit(&stg->sht.tail.pos,
> -			rte_memory_order_relaxed);
> -	if (tail == pos)
> -		__rte_soring_stage_finalize(&stg->sht, stage, r->state, r->mask,
> +	/* try to do finalize(), if possible */
> +	__rte_soring_stage_finalize(&stg->sht, stage, r->state, r->mask,
>  				r->capacity);
>  }

Previously, finalize was only called when tail == pos. Now it's always
called. Does __rte_soring_stage_finalize() handle the case where tail !=
pos correctly? Could this cause unnecessary work or contention in cases
where the condition was previously false?