|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 modifies the soring (staged ordered ring) finalization logic
to have each stage finalize only its own state rather than being called
from multiple contexts. This simplifies the code and improves performance
by reducing 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);
> -	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;
> -	}

Does this code change the behavior for callers using RTE_RING_QUEUE_FIXED?
Previously when n was 0, the code would attempt finalization and retry the
move_cons_head call with 'behavior'. Now the initial call passes 'behavior'
directly. If the first call fails with FIXED behavior, won't it return 0
without the retry that previously happened after finalization?

> @@ -382,7 +366,7 @@ soring_acquire(struct rte_soring *r, void *objs, void *meta,
>  	uint32_t stage, uint32_t num, enum rte_ring_queue_behavior behavior,
>  	uint32_t *ftoken, uint32_t *available)
>  {
> -	uint32_t avail, head, idx, n, next, reqn;
> +	uint32_t avail, head, idx, n, next;
>  	struct soring_stage *pstg;
>  	struct soring_stage_headtail *cons;
>  
> @@ -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 removing the finalize-and-retry logic
from acquire() break the forward progress guarantee for FIXED mode? If
stage-1 hasn't finalized yet but has completed elements, won't acquire()
now fail where it previously would have succeeded after forcing
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);

Does calling finalize() unconditionally on every release increase overhead?
The previous code only called it when tail==pos, presumably to avoid
unnecessary work. Does finalize() have sufficient early-exit logic to make
this unconditional call acceptable performance-wise for the case where
tail!=pos?