|SUCCESS| [v5,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/163594

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-06

Commit: 4bc2321fb5 (based on patch context)
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>

Subject: [PATCH v5 1/2] ring: make soring to always finalize its own stage

This patch modifies the SORING finalize() behavior to ensure it is
always invoked from the release() function of the current stage, rather
than relying on tail-head position checks. This reduces contention when
multiple threads perform operations on the same stage.

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

Does removing the tail == pos check introduce any performance regressions
for the single-threaded case? The original code avoided calling
__rte_soring_stage_finalize() when tail != pos, which suggests there was
a performance consideration. Can this unconditional call to finalize()
negatively impact latency in scenarios with very low contention?

---

Commit: 4bc2321fb5 (continuation)
Author: Konstantin Ananyev <konstantin.ananyev at huawei.com>

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

This patch introduces peek-style APIs for SORING, similar to rte_ring.
The API allows splitting enqueue/dequeue operations into start and
finish phases, enabling MT-safe inspection of ring contents.

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

In function rte_soring_enqueux_finish(), the documentation states "the
number of objects to enqueue should not exceed previous enqueue_start
return value." Does the implementation validate this constraint, or can
passing n larger than the value returned by enqueue_start cause buffer
overflows or corruption of ring state?

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

[ ... ]

> @@ -249,6 +249,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 function __enqueue_elems(), should there be validation that r->meta is
not NULL when meta != NULL? Can this code dereference a NULL pointer in
r->meta if the soring was created with meta_size == 0 but the caller
incorrectly passes a non-NULL meta parameter?

[ ... ]

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

In function soring_enqueue_finish(), can calling this function with num
larger than what enqueue_start returned result in n being clamped or does
it lead to ring corruption? Does __rte_ring_st_get_tail() or
__rte_ring_hts_get_tail() properly handle this case?

[ ... ]

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

In function soring_dequeue_start(), when st is neither RTE_RING_SYNC_ST
nor RTE_RING_SYNC_MT_HTS in a non-debug build, does execution fall
through the RTE_ASSERT(0) and return potentially uninitialized values for
head and next? Can this cause undefined behavior in the subsequent call to
__dequeue_elems()?