|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-16

I'll review this patch series focusing on correctness, C coding style, and API compliance.

```
commit 9e1a2f3b4c5d6e7f8a9b0c1d2e3f4a5b6c7d8e9f
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>

[PATCH v2 1/2] ring: make soring to finalize its own stage only

While finalize() is MT-safe and can be called from multiple places:
from 'acquire()' for next stage or even from consumer's 'dequeue(),
doing so usually creates extra un-necessary contention and might
slow-down ring operations, especially for the cases when we have
multiple threads doing acquire/release for some stage.
Things become worse when we have single producer/consumer and multiple
workers doing acquire/release for the same ring.
So instead of calling finalize() from different places, just make
release() for given stage to always try to perform it.

> 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

[ ... ]

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

The change from RTE_RING_QUEUE_FIXED to behavior in the first call appears
inconsistent with the rest of the function logic. After this call, if n is
zero, does this code still correctly handle the RTE_RING_QUEUE_FIXED case?
Previously there was a second attempt to grab elements after finalization,
but now only one attempt is made with the behavior parameter. Does this
preserve the original semantics for the bulk/fixed case?

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

By removing this retry logic with finalization, does this change introduce
a behavior regression where dequeue operations might fail unnecessarily
when finalization could have made elements available?

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

Similar to the dequeue case, does removing this retry-after-finalize logic
introduce a behavior change where acquire operations can spuriously fail?

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

The removal of the tail equals pos check means finalize is now always
called unconditionally. Does __rte_soring_stage_finalize internally
handle the case where no finalization is possible? The original code
had this optimization to avoid unnecessary calls. Does removing it
potentially increase overhead in cases where finalization is not yet
possible?

[PATCH v2 2/2] ring: introduce peek API for soring

> 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

[ ... ]

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

The meta NULL check follows coding style guideline for explicit
comparisons, which is correct.

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

The default case initializes free and n to 0, which is good defensive
programming even though RTE_ASSERT(0) will trigger. However, does this
provide adequate error signaling to callers when an unsupported sync type
is used? In production builds with assertions disabled, does this silently
return 0 without logging the error condition?

> +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)
> +{
> +	return soring_enqueue_finish(r, objs, meta, n);
> +}

This function has a return statement for a void function. While technically
legal in C, does this follow DPDK coding style conventions? This pattern
appears in several other wrapper functions as well.

> 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 n
> + *   The number of objects to add in the soring from the 'objs'.
> + */
> +__rte_experimental
> +void
> +rte_soring_enqueue_finish(struct rte_soring *r, const void *objs, uint32_t n);

The documentation states results are undefined if n exceeds the previous
enqueue_start return value. Does the implementation validate this
constraint, or does it rely purely on correct caller behavior? Should there
be defensive checks or at least a debug assertion?

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Complete to enqueue several objects plus metadata on the soring.

There's a typo: enqueux_finish should likely be enqueue_finish or
enqueuex_finish for consistency. Is this intentional to distinguish the
metadata variant, or is it a typo?

> +__rte_experimental
> +void
> +rte_soring_enqueux_finish(struct rte_soring *r, const void *objs,
> +	const void *meta, uint32_t n);

[ ... ]

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

Does the API properly handle the case where a thread calls dequeue_start
but never calls dequeue_finish? What are the consequences for subsequent
operations? Does this create resource leaks or deadlocks?

```