|SUCCESS| [v3,10/10] dts: add selective Rx tests

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164538

_AI Code Review_

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

Looking at this DPDK patch series for selective Rx support, I'll provide a focused technical review based on the AGENTS.md guidelines.

commit 20260528124758.1984528-2-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>

app/testpmd: print Rx split capabilities

[ ... ]

> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index c950793aaf..55d1c6d696 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -790,6 +790,12 @@ rss_offload_types_display(uint64_t offload_types, uint16_t char_num_per_line)
>  	printf("\n");
>  }
>  
> +static void
> +print_bool_capa(const char *label, int value)
> +{
> +	printf("%s: %s\n", label, value ? "supported" : "not supported");
> +}

Does this code have an issue with implicit boolean comparisons? The
coding style guide states that boolean expressions should not be
compared against TRUE or FALSE, but rather use explicit comparisons for
integers. Should this be "value != 0" instead of "value ?"

> @@ -911,6 +917,16 @@ port_infos_display(portid_t port_id)
>  		dev_info.max_rx_pktlen);
>  	printf("Maximum configurable size of LRO aggregated packet: %u\n",
>  		dev_info.max_lro_pkt_size);
> +
> +	printf("Rx split:\n");
> +	printf("\tMax segments: %hu\n", dev_info.rx_seg_capa.max_nseg);
> +	if (dev_info.rx_seg_capa.max_nseg > 0) {
> +		print_bool_capa("\tMulti-pool", dev_info.rx_seg_capa.multi_pools);
> +		print_bool_capa("\tBuffer offset", dev_info.rx_seg_capa.offset_allowed);
> +		printf("\tOffset alignment: %u\n",
> +				RTE_BIT32(dev_info.rx_seg_capa.offset_align_log2));
> +	}
> +
>  	if (dev_info.max_vfs)

---

commit 20260528124758.1984528-3-thomas at monjalon.net
Author: Gregory Etelson <getelson at nvidia.com>

ethdev: introduce selective Rx

[ ... ]

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d0273e3f7b..67d609820c 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -2129,7 +2129,7 @@ rte_eth_rx_queue_check_split(uint16_t port_id,
>  			const struct rte_eth_dev_info *dev_info)
>  {
>  	const struct rte_eth_rxseg_capa *seg_capa = &dev_info->rx_seg_capa;
> -	struct rte_mempool *mp_first;
> +	struct rte_mempool *mp_first = NULL;

Does this initialization matter? The code below continues to check
segments until finding the first pool in rte_eth_rx_queue_check_split.
If mp_first stays NULL through the loop, does this code properly handle
that case at line 2239 where it checks "if (mp_first == NULL)"?

> @@ -2148,13 +2148,17 @@ rte_eth_rx_queue_check_split(uint16_t port_id,
>  		uint32_t offset = rx_seg[seg_idx].offset;
>  		uint32_t proto_hdr = rx_seg[seg_idx].proto_hdr;
>  
> -		if (mpl == NULL) {
> -			RTE_ETHDEV_LOG_LINE(ERR, "null mempool pointer");
> -			ret = -EINVAL;
> -			goto out;
> +		if (mpl == NULL) { /* discarded segment */
> +			if (seg_capa->selective_rx == 0) { /* not supported */
> +				RTE_ETHDEV_LOG_LINE(ERR, "null mempool pointer");
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			continue; /* next checks are not relevant if no mempool */
>  		}

Does this code properly validate the segment configuration? When
mpl == NULL, the code skips all remaining validation for that segment,
but are there consistency checks needed even for discard segments? For
example, should offset or length constraints still be validated?

> @@ -2233,6 +2236,11 @@ rte_eth_rx_queue_check_split(uint16_t port_id,
>  		if (ret != 0)
>  			goto out;
>  	}
> +	if (mp_first == NULL) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "At least one Rx segment must have a mempool");
> +		ret = -EINVAL;
> +		goto out;
> +	}

---

commit 20260528124758.1984528-4-thomas at monjalon.net
Author: Gregory Etelson <getelson at nvidia.com>

app/testpmd: support selective Rx

[ ... ]

> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index a9b35f530a..d14341d3ff 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2731,6 +2731,16 @@ port_is_started(portid_t port_id)
>  	return 1;
>  }
>  
> +static struct rte_eth_rxseg_split *
> +next_rx_seg(union rte_eth_rxseg *segs, uint16_t *idx)
> +{
> +	if (*idx >= MAX_SEGS_BUFFER_SPLIT) {
> +		fprintf(stderr, "Too many segments (max %u)\n", MAX_SEGS_BUFFER_SPLIT);
> +		return NULL;
> +	}
> +	return &segs[(*idx)++].split;
> +}

Does this function handle overflow correctly? The post-increment of
*idx happens after the array access. If idx is exactly
MAX_SEGS_BUFFER_SPLIT-1, it returns the last valid segment but then
increments idx to MAX_SEGS_BUFFER_SPLIT. Does the caller always check
the return value before the next call?

> @@ -2744,31 +2754,70 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>  	uint32_t prev_hdrs = 0;
>  	int ret;
>  
> -	if ((rx_pkt_nb_segs > 1) &&
> -	    (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT)) {
> +	if (rx_pkt_nb_segs > 1 || rx_pkt_nb_offs > 0) {

[ ... ]

> +		/* multi-segment configuration */
> +		for (i = 0; i < rx_pkt_nb_segs; i++) {
> +			struct rte_eth_rxseg_split *rx_seg;
> +			uint16_t seg_offset;
> +
> +			if (i < rx_pkt_nb_offs)
> +				seg_offset = rx_pkt_seg_offsets[i];
> +			else
> +				seg_offset = rx_pkt_nb_offs > 0 ? next_offset : 0;
> +
> +			/* Insert selective Rx discard segment if there's a gap */
> +			if (seg_offset > next_offset) {
> +				rx_seg = next_rx_seg(rx_useg, &seg_idx);
> +				if (rx_seg == NULL)
> +					return -EINVAL;
> +				rx_seg->offset = next_offset;
> +				rx_seg->length = seg_offset - next_offset;
> +				rx_seg->mp = NULL;
> +				next_offset = seg_offset;
> +			}

Does this code overflow seg_idx? Multiple gap-filling insertions could
accumulate more segments than MAX_SEGS_BUFFER_SPLIT. The next_rx_seg
call checks the limit, but does returning -EINVAL here leak the
rx_useg array or leave the queue in an inconsistent state?

---

commit 20260528124758.1984528-7-thomas at monjalon.net
Author: Gregory Etelson <getelson at nvidia.com>

net/mlx5: support selective Rx

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c
> index 185bfd4fff..586bf6c935 100644
> --- a/drivers/net/mlx5/mlx5_rx.c
> +++ b/drivers/net/mlx5/mlx5_rx.c
> @@ -486,7 +486,7 @@ mlx5_rxq_initialize(struct mlx5_rxq_data *rxq)
>  					rxq->wqes)[i];
>  			addr = rte_pktmbuf_mtod(buf, uintptr_t);
>  			byte_count = DATA_LEN(buf);
> -			lkey = mlx5_rx_mb2mr(rxq, buf);
> +			lkey = buf->pool ? mlx5_rx_mb2mr(rxq, buf) : rxq->sh->null_mr->lkey;

Does this code check null_mr before dereferencing? If
rxq->sh->null_mr is NULL (allocation failed), does
mlx5_rxq_initialize() crash here accessing null_mr->lkey?

> @@ -1044,11 +1044,13 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  	const unsigned int sges_n = rxq->sges_n;
>  	struct rte_mbuf *pkt = NULL;
>  	struct rte_mbuf *seg = NULL;
> +	struct rte_mbuf *tail = NULL;

[ ... ]

> -		if (pkt)
> -			NEXT(seg) = rep;
> +		if (pkt) {
> +			if (rep->pool)
> +				NEXT(tail) = rep;
> +			else
> +				--NB_SEGS(pkt);
> +		}

Does this code correctly maintain mbuf chain integrity? When rep->pool
is NULL (discard segment), the code decrements NB_SEGS but doesn't link
the segment. If there are multiple consecutive discard segments, does
tail stay pointing to the last valid segment, or does it become stale?

> @@ -1103,7 +1103,7 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  			if (unlikely(len & MLX5_ERROR_CQE_MASK)) {
>  				/* We drop packets with non-critical errors */
>  				if (seg->pool)
> -				rte_mbuf_raw_free(rep);
> +					rte_mbuf_raw_free(rep);

The indentation fix is good, but does this leak rep when seg->pool is
NULL? The comment says "drop packets with non-critical errors", but if
rep was allocated but seg is a null segment, does rep get freed?

---

commit 20260528124758.1984528-6-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>

net/mlx5: fix Rx split segment counter type

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
> index dffab3955b..01b563d981 100644
> --- a/drivers/net/mlx5/mlx5_rx.h
> +++ b/drivers/net/mlx5/mlx5_rx.h
> @@ -164,9 +164,9 @@ struct __rte_cache_aligned mlx5_rxq_data {
>  	uint64_t flow_meta_mask;
>  	int32_t flow_meta_offset;
>  	uint32_t flow_meta_port_mask;
> -	uint32_t rxseg_n; /* Number of split segment descriptions. */
>  	struct mlx5_eth_rxseg rxseg[MLX5_MAX_RXQ_NSEG];
>  	/* Buffer split segment descriptions - sizes, offsets, pools. */
> +	uint16_t rxseg_n; /* Number of split segment descriptions. */

Does this struct reordering create ABI issues? Moving rxseg_n after the
rxseg array changes field offsets. Is this change safe within the same
release, or does it need ABI versioning?

---

commit 20260528124758.1984528-10-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>

dts: fix topology capability comparison

[ ... ]

> diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py
> index 960370fc72..96e1cd449f 100644
> --- a/dts/framework/testbed_model/capability.py
> +++ b/dts/framework/testbed_model/capability.py
> @@ -574,7 +574,9 @@ def __gt__(self, other: Any) -> bool:
>          Returns:
>              :data:`True` if the instance's topology type is more complex than the compared object's.
>          """
> -        return other < self
> +        if not self.is_comparable_with(other):
> +            return False
> +        return self.topology_type > other.topology_type

Does this fix handle all comparison edge cases consistently? The
is_comparable_with() check returns False for non-TopologyCapability
types, but does that match Python's rich comparison semantics where
NotImplemented should be returned instead of False for incomparable
types?