|SUCCESS| [v3,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/163179

_AI Code Review_

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

Review of patch series 'series.patch'

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

This commit modifies the soring finalization logic to have each stage
finalize only its own work rather than calling finalize from multiple
locations.

> 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.
>   */
>  
> @@ -284,7 +282,7 @@ soring_dequeue(struct rte_soring *r, void *objs, void *meta,
>  	uint32_t *available)
>  {
>  	enum rte_ring_sync_type st;
> -	uint32_t entries, cons_head, cons_next, n, ns, reqn;
> +	uint32_t entries, cons_head, cons_next, n, ns;

[ ... ]

> @@ -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 behavior change affect correctness? The original code always
used RTE_RING_QUEUE_FIXED for the first attempt, but now it passes
behavior directly. Could this cause unexpected behavior when behavior is
RTE_RING_QUEUE_VARIABLE?

> -	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;
> -	}
>  
>  	/* we have some elems to consume */
>  	if (n != 0) {
> @@ -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;

[ ... ]

> @@ -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);
>  	}
>  
>  	if (n != 0) {

Similar concern about the behavior parameter. Does removing the two-phase
acquire logic (try with FIXED, then retry with behavior) cause any
functional regressions?

> @@ -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;

[ ... ]

> @@ -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 __rte_soring_stage_finalize() unconditionally increase
contention compared to the guarded approach? The old code checked if
tail == pos before calling finalize, which appears to be an optimization
to avoid unnecessary calls.


git sha: 20260417212358.212692-3-konstantin.ananyev at huawei.com
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>
Subject: [PATCH v3 2/2] ring: introduce peek API for soring

This commit adds peek-style API for soring, allowing split
enqueue/dequeue operations.

> diff --git a/lib/ring/rte_soring.h b/lib/ring/rte_soring.h
> index 95c3cc4080..16f042e612 100644
> --- a/lib/ring/rte_soring.h
> +++ b/lib/ring/rte_soring.h

[ ... ]

> @@ -607,6 +607,270 @@ void
>  rte_soring_releasx(struct rte_soring *r, const void *objs,
>  	const void *meta, uint32_t stage, uint32_t n, uint32_t ftoken);
>  
> +/**
> + * SORING Peek API
> + * Same as with rte_ring, for some sync modes it is possible to split
> + * public enqueue/dequeue API into two phases:
> + * - enqueue/dequeue start
> + * - enqueue/dequeue finish
> + * That allows user to inspect objects in the soring without removing them
> + * from it (aka MT safe peek).
> + * Note that right now this new API is available only for two sync modes:
> + * 1) Single Producer/Single Consumer (RTE_RING_SYNC_ST)
> + * 2) Serialized Producer/Serialized Consumer (RTE_RING_SYNC_MT_HTS).
> + * It is a user responsibility to create/init soring with appropriate sync
> + * modes selected for enqueue/dequeue.
> + * For more information, please refer to corresping rte_ring peek API.

Typo: "corresping" should be "corresponding".

[ ... ]

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Complete to enqueue several objects plus metadata on the soring.
> + * Note that number of objects to enqueue should not exceed previous
> + * enqueue_start return value.
> + *
> + * @param r
> + *   A pointer to the soring structure.
> + * @param objs
> + *   A pointer to an array of objects to enqueue.
> + *   Size of objects to enqueue must be the same value as 'elem_size' parameter
> + *   used while creating the soring. Otherwise the results are undefined.
> + * @param meta
> + *   A pointer to an array of metadata values for each object to enqueue.
> + *   Note that if user not using object metadata values, then this parameter
> + *   can be NULL.
> + *   Size of elements in this array must be the same value as 'meta_size'
> + *   parameter used while creating the soring. If user created the soring with
> + *   'meta_size' value equals zero, then 'meta' parameter should be NULL.
> + *   Otherwise the results are undefined.
> + * @param n
> + *   The number of objects to add in the soring from the 'objs'.
> + */
> +__rte_experimental
> +void
> +rte_soring_enqueux_finish(struct rte_soring *r, const void *objs,
> +	const void *meta, uint32_t n);

Function name has a typo: "enqueux_finish" should likely be
"enqueue_finish" to match the pattern. Is "enqueux" intentional to
distinguish metadata-aware operations, or is this a spelling error?

> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
> index fc7fc55a21..b7d0ad6da8 100644
> --- a/lib/ring/soring.c
> +++ b/lib/ring/soring.c

[ ... ]

> @@ -247,6 +247,28 @@ __rte_soring_stage_move_head(struct soring_stage_headtail *d,
>  	return n;
>  }
>  
> +static inline void
> +__enqueue_elems(struct rte_soring *r, const void *objs, const void *meta,
> +	uint32_t head, uint32_t n)
> +{
> +	__rte_ring_do_enqueue_elems(&r[1], objs, r->size, head & r->mask,
> +		r->esize, n);
> +	if (meta != NULL)
> +		__rte_ring_do_enqueue_elems(r->meta, meta, r->size,
> +			head & r->mask, r->msize, n);
> +}

Does this code properly handle the case where meta is NULL but r->msize
is non-zero? Could this lead to inconsistent state where objects are
enqueued but metadata slots are left uninitialized?

[ ... ]

> +static inline uint32_t
> +soring_enqueue_start(struct rte_soring *r, uint32_t num,
> +	enum rte_ring_queue_behavior behavior, uint32_t *free_space)
> +{
> +	enum rte_ring_sync_type st;
> +	uint32_t free, head, n, next;
> +
> +	RTE_ASSERT(r != NULL && r->nb_stage > 0);
> +
> +	st = r->prod.ht.sync_type;
> +
> +	switch (st) {
> +	case RTE_RING_SYNC_ST:
> +		n = __rte_ring_headtail_move_head(&r->prod.ht, &r->cons.ht,
> +			r->capacity, RTE_RING_SYNC_ST, num, behavior,
> +			&head, &next, &free);
> +		break;
> +	case RTE_RING_SYNC_MT_HTS:
> +		n = __rte_ring_hts_move_head(&r->prod.hts, &r->cons.ht,
> +			r->capacity, num, behavior, &head, &free);
> +		break;
> +	default:
> +		/* unsupported mode, shouldn't be here */
> +		RTE_ASSERT(0);
> +		free = 0;
> +		n = 0;
> +	}

If the code reaches the default case in a non-debug build, does returning
n = 0 and free = 0 cause any issues for callers expecting valid state?
Should there be runtime error handling instead of just RTE_ASSERT?

[ ... ]

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_soring_enqueux_finish, 26.07)
> +void
> +rte_soring_enqueux_finish(struct rte_soring *r, const void *objs,
> +	const void *meta, uint32_t n)
> +{
> +	soring_enqueue_finish(r, objs, meta, n);
> +}

Again, is "enqueux" intentional or a typo that should be "enqueue"?