|SUCCESS| [v2,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/163161

_AI Code Review_

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

# Review of DPDK SORING patch series

commit 1c849ee2a3f4 (based on patch identifiers)
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>
Subject: [PATCH v2 1/2] ring: make soring to finalize its own stage only

This patch modifies the soring finalize behavior to have each stage's
release() always attempt finalization instead of 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 @@

[ ... ]

>  static inline uint32_t
>  soring_dequeue(struct rte_soring *r, void *objs, void *meta,
>  	uint32_t num, enum rte_ring_queue_behavior behavior,
> @@ -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 semantic behavior when RTE_RING_QUEUE_VARIABLE
is passed? Previously, in the first call to __rte_soring_move_cons_head(),
the behavior was always forced to RTE_RING_QUEUE_FIXED. After this change,
if behavior is RTE_RING_QUEUE_VARIABLE, it's passed directly to
__rte_soring_move_cons_head(). Does this alter the dequeue behavior in a
way that could affect existing users?

[ ... ]

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

Same question here: does changing from forcing RTE_RING_QUEUE_FIXED to
passing the caller's behavior directly alter the semantics in a way that
could break existing code paths?

[ ... ]

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

The check for tail == pos has been removed. Does
__rte_soring_stage_finalize() handle the case where tail != pos correctly,
or could this cause unnecessary overhead or incorrect behavior when
attempting finalization?


commit 6a5b8c7d9e12 (based on patch identifiers)
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>
Subject: [PATCH v2 2/2] ring: introduce peek API for soring

This patch adds peek-style API for soring rings with ST and HTS sync
modes, allowing split enqueue/dequeue operations.

> diff --git a/app/test/test_soring_mt_stress.c b/app/test/test_soring_mt_stress.c

[ ... ]

> +static const struct test_case tests[] = {
> +	{
> +		.name = "MT_DEQENQ-MT_STG1-PRCS",
> +		.func = test_sym_mt1,
> +		.wfunc = test_worker_prcs,
> +	},

[ ... ]

> diff --git a/app/test/test_soring_peek_stress.c b/app/test/test_soring_peek_stress.c
> new file mode 100644
> index 0000000000..6ac638f8d7
> --- /dev/null
> +++ b/app/test/test_soring_peek_stress.c
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Huawei Technologies Co., Ltd
> + */
> +
> +#include "test_soring_stress_impl.h"
> +
> +static inline uint32_t
> +_st_ring_dequeue_burst(struct rte_soring *r, void **obj, uint32_t n,
> +	uint32_t *avail)
> +{
> +	uint32_t m;
> +
> +	m = rte_soring_dequeue_burst_start(r, obj, n, avail);
> +	if (m != 0)
> +		rte_soring_dequeue_finish(r, m);
> +	return m;
> +}

In _st_ring_dequeue_burst(), if rte_soring_dequeue_burst_start() returns
a non-zero value but then rte_soring_dequeue_finish() is not called (say,
due to an early return path), would this leave the ring in an inconsistent
state?

> +
> +static inline uint32_t
> +_st_ring_enqueue_bulk(struct rte_soring *r, void * const *obj, uint32_t n,
> +	uint32_t *free)
> +{
> +	uint32_t m;
> +
> +	m = rte_soring_enqueue_bulk_start(r, n, free);
> +	if (m != 0)
> +		rte_soring_enqueue_finish(r, obj, m);
> +	return m;
> +}

Similarly in _st_ring_enqueue_bulk(), if rte_soring_enqueue_bulk_start()
succeeds but rte_soring_enqueue_finish() is not called, does this leak
the reserved space in the ring?

[ ... ]

> 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

[ ... ]

> +/**
> + * @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);

The function name rte_soring_enqueux_finish appears to have a typo.
Should this be rte_soring_enqueue_finish or is the 'x' intentional to
differentiate the version that accepts metadata?

[ ... ]

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Start to dequeue several objects plus metadata from the soring.
> + * Dequeues exactly requested number of objects or none.
> + * Note that user has to call appropriate dequeue_finish()
> + * to complete given dequeue operation and actually remove objects from
> + * the soring.
> + *
> + * @param r
> + *   A pointer to the soring structure.
> + * @param objs
> + *   A pointer to an array of objects to dequeue.
> + *   Size of objects to enqueue must be the same value as 'elem_size' parameter
> + *   used while creating the soring. Otherwise the results are undefined.

In the documentation for rte_soring_dequeux_bulk_start(), the param
description for objs says "Size of objects to enqueue" but should this
say "dequeue" instead?

[ ... ]

> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
> index fc7fc55a21..de5b67071f 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);
> +}

In __enqueue_elems(), could the compiler optimize the check for meta !=
NULL differently based on optimization level? Should this use
rte_likely() or rte_unlikely() to provide a hint?

[ ... ]

> +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;
> +	}

In soring_enqueue_start() and similar functions, when the sync_type falls
into the default case, does RTE_ASSERT(0) provide adequate error handling
in production builds where assertions may be disabled?

[ ... ]

> +static inline void
> +soring_enqueue_finish(struct rte_soring *r, const void *objs, const void *meta,
> +	uint32_t num)
> +{
> +	enum rte_ring_sync_type st;
> +	uint32_t n