|SUCCESS| [v4,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/163261

_AI Code Review_

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

I'll review this patch for DPDK compliance, focusing on correctness, coding style, and API requirements.

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

This patch modifies the SORING internal finalize() function to be called
always from release() for the current stage, reducing contention and
improving performance for multi-worker scenarios.

> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
> index 3b90521bdb..4bc2321fb5 100644
> --- a/lib/ring/soring.c
> +++ b/lib/ring/soring.c

[ ... ]

> @@ -442,7 +442,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,12 +479,9 @@ 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,
> -				r->capacity);
> +	/* now, try to do finalize() */
> +	__rte_soring_stage_finalize(&stg->sht, stage, r->state, r->mask,
> +			r->capacity);
>  }

The patch removes the conditional check before calling finalize(). While
the comment states this is MT-safe and performance is improved, does this
create any race conditions where finalize() is called with inconsistent
state? The commit message mentions finalize() acts as a try-lock scenario,
but could there be edge cases where removing the tail check causes
spurious finalize attempts that could affect correctness?


---

commit 3f7b8d9a (based on patch metadata)  
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>
Subject: [PATCH v4 2/2] ring: introduce peek API for soring

This patch introduces peek-style API for soring, allowing split
enqueue/dequeue operations for RTE_RING_SYNC_ST and RTE_RING_SYNC_MT_HTS
modes.

> diff --git a/app/test/test_soring_peek_stress.c b/app/test/test_soring_peek_stress.c
> new file mode 100644
> index 0000000000..cbcea51c64
> --- /dev/null
> +++ b/app/test/test_soring_peek_stress.c

[ ... ]

> diff --git a/lib/ring/rte_soring.h b/lib/ring/rte_soring.h
> index 95c3cc4080..5d214f85a0 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);

Does this function name have a typo? The function is named
rte_soring_enqueux_finish but should it be rte_soring_enqueue_finish to
match the naming pattern? The corresponding dequeue functions use
rte_soring_dequeux_* which appears intentional for metadata variants, but
this seems inconsistent with rte_soring_enqueue_finish which already
exists.

> diff --git a/lib/ring/soring.c b/lib/ring/soring.c
> index 4bc2321fb5..37f2db2557 100644
> --- a/lib/ring/soring.c
> +++ b/lib/ring/soring.c

[ ... ]

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

Does this code properly handle the case where the user calls
enqueue_start() with an unsupported sync mode in production builds? The
RTE_ASSERT(0) will be compiled out in release builds, leaving free and n
uninitialized in the default case. Should this return an error code or
handle the unsupported mode more gracefully?

> +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, tail;
> +
> +	RTE_ASSERT(r != NULL && r->nb_stage > 0);
> +	RTE_ASSERT(meta == NULL || r->meta != NULL);
> +
> +	st = r->prod.ht.sync_type;
> +
> +	switch (st) {
> +	case RTE_RING_SYNC_ST:
> +		n = __rte_ring_st_get_tail(&r->prod.ht, &tail, num);
> +		if (n != 0)
> +			__enqueue_elems(r, objs, meta, tail, n);
> +		__rte_ring_st_set_head_tail(&r->prod.ht, tail, n, 1);
> +		break;
> +	case RTE_RING_SYNC_MT_HTS:
> +		n = __rte_ring_hts_get_tail(&r->prod.hts, &tail, num);
> +		if (n != 0)
> +			__enqueue_elems(r, objs, meta, tail, n);
> +		__rte_ring_hts_set_head_tail(&r->prod.hts, tail, n, 1);
> +		break;
> +	default:
> +		/* unsupported mode, shouldn't be here */
> +		RTE_ASSERT(0);
> +	}
> +}

Does this code have the same issue as soring_enqueue_start()? In release
builds with RTE_ASSERT compiled out, if an unsupported sync mode is used,
does the function silently do nothing or corrupt state?

> +static inline uint32_t
> +soring_dequeue_start(struct rte_soring *r, void *objs, void *meta,
> +	uint32_t num, enum rte_ring_queue_behavior behavior,
> +	uint32_t *available)
> +{
> +	enum rte_ring_sync_type st;
> +	uint32_t avail, head, next, n, ns;
> +
> +	RTE_ASSERT(r != NULL && r->nb_stage > 0);
> +	RTE_ASSERT(meta == NULL || r->meta != NULL);
> +
> +	ns = r->nb_stage - 1;
> +	st = r->cons.ht.sync_type;
> +
> +	switch (st) {
> +	case RTE_RING_SYNC_ST:
> +		n = __rte_ring_headtail_move_head(&r->cons.ht, &r->stage[ns].ht,
> +			0, RTE_RING_SYNC_ST, num, behavior, &head, &next,
> +			&avail);
> +		break;
> +	case RTE_RING_SYNC_MT_HTS:
> +		n = __rte_ring_hts_move_head(&r->cons.hts, &r->stage[ns].ht,
> +			0, num, behavior, &head, &avail);
> +		break;
> +	default:
> +		/* unsupported mode, shouldn't be here */
> +		RTE_ASSERT(0);
> +		avail = 0;
> +		n = 0;
> +	}

Same pattern here - does this code properly handle unsupported sync modes
in production builds where RTE_ASSERT is compiled out?

> +static inline void
> +soring_dequeue_finish(struct rte_soring *r, uint32_t num)
> +{
> +	uint32_t n, tail;
> +
> +	RTE_ASSERT(r != NULL && r->nb_stage > 0);
> +
> +	switch (r->cons.ht.sync_type) {
> +	case RTE_RING_SYNC_ST:
> +		n = __rte_ring_st_get_tail(&r->cons.ht, &tail, num);
> +		__rte_ring_st_set_head_tail(&r->cons.ht, tail, n, 0);
> +		break;
> +	case RTE_RING_SYNC_MT_HTS:
> +		n = __rte_ring_hts_get_tail(&r->cons.hts, &tail, num);
> +		__rte_ring_hts_set_head_tail(&r->cons.hts, tail, n, 0);
> +		break;
> +	default:
> +		/* unsupported mode, shouldn't be here */
> +		RTE_ASSERT(0);
> +	}
> +}

Same concern about unsupported mode handling.

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

This exports the potentially misspelled function name. Should this be
reviewed for naming consistency with the rest of the API?