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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

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

---

**Review of PATCH v6 series: selective Rx feature for DPDK**

**Commit:** (git sha from series not provided in isolation, assuming v6 iteration)

**Author:** Thomas Monjalon <thomas at monjalon.net>, Gregory Etelson <getelson at nvidia.com>

**Subject:** Multiple patches introducing selective Rx feature

---

### PATCH v6 01/10: app/testpmd: print Rx split capabilities

**Summary:** Adds printing of `rte_eth_rxseg_capa` fields to `show port info`.

> 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 match the existing style for boolean capability prints? The existing code might use different patterns. This is acceptable but should be consistent with similar code.

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

Does this code handle the case where max_nseg is 0? It looks fine, but the alignment calculation RTE_BIT32 with offset_align_log2 could overflow with bad data. Consider bounds checking offset_align_log2 before the bit shift.

---

### PATCH v6 02/10: ethdev: introduce selective Rx

**Summary:** Introduces selective Rx capability allowing NULL mempool segments.

> +/**
> + * Memory pool to allocate segment from.
> + *
> + * NULL means discarded segment.
> + * Length of discarded segment is not reflected in mbuf packet length
> + * and not accounted in ibytes statistics.
> + * @see rte_eth_rxseg_capa::selective_rx
> + */
> +struct rte_mempool *mp;

Good documentation of NULL semantics.

> @@ -2148,7 +2148,6 @@ rte_eth_rx_queue_check_split(uint16_t port_id,
>  	 * Check the sizes and offsets against buffer sizes
>  	 * for each segment specified in extended configuration.
>  	 */
> -	mp_first = rx_seg[0].mp;
>  	offset_mask = RTE_BIT32(seg_capa->offset_align_log2) - 1;

Does this create a potential shift overflow? If offset_align_log2 is 32, RTE_BIT32(32) is undefined behavior. Should validate offset_align_log2 < 32.

> @@ -2160,13 +2159,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 skip validation for NULL segments even when selective_rx is supported? The offset and length checks below are skipped. Could a driver accept invalid offset/length in a NULL segment that causes issues? Might need to at least validate offset + length doesn't exceed max_rx_pktlen.

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

Good catch requiring at least one real segment.

---

### PATCH v6 03/10: app/testpmd: support selective Rx

**Summary:** Adds selective Rx support in testpmd using rxoffs and rxpkts.

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

Good bounds checking.

> @@ -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) {
> +		struct rte_eth_dev_info dev_info;
> +		uint16_t seg_idx = 0;
> +		uint16_t next_offset = 0;
> +
> +		ret = rte_eth_dev_info_get(port_id, &dev_info);
> +		if (ret != 0)
> +			return ret;

Does this handle dev_info uninitialized on error? rte_eth_dev_info_get might not clear dev_info on failure. The immediate return is safe, but later code uses dev_info.

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

Does this handle seg_offset < next_offset (going backwards)? This would indicate a configuration error. Should validate 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 seg_offset - next_offset overflow? If seg_offset wraps, this could be very large. Already checked above, so safe if validation added.

> +		/* Add trailing selective Rx discard segment up to max packet length */
> +		if (rx_pkt_nb_offs > 0 && next_offset < dev_info.max_rx_pktlen) {
> +			struct rte_eth_rxseg_split *rx_seg = next_rx_seg(rx_useg, &seg_idx);
> +			if (rx_seg == NULL)
> +				return -EINVAL;
> +			rx_seg->offset = next_offset;
> +			rx_seg->length = dev_info.max_rx_pktlen - next_offset;
> +			rx_seg->mp = NULL;
> +		}

Does this correctly handle the case where dev_info.max_rx_pktlen is 0? That would be an invalid device, but defensive coding suggests checking.

---

### PATCH v6 04/10: common/mlx5: add null MR functions

**Summary:** Adds functions to allocate/free null Memory Region.

> +RTE_EXPORT_INTERNAL_SYMBOL(mlx5_os_alloc_null_mr)
> +struct mlx5_pmd_mr *
> +mlx5_os_alloc_null_mr(struct rte_device *dev, void *pd)
> +{
> +	struct ibv_mr *ibv_mr;
> +	struct mlx5_pmd_mr *null_mr;
> +
> +	null_mr = mlx5_malloc(MLX5_MEM_ZERO, sizeof(*null_mr), 0, dev->numa_node);
> +	if (!null_mr)
> +		return NULL;

Does mlx5_malloc log errors? Missing error log here might make debugging harder.

> +	ibv_mr = mlx5_glue->alloc_null_mr(pd);
> +	if (!ibv_mr) {
> +		mlx5_free(null_mr);
> +		return NULL;
> +	}

Same here, no error logging. Should add DRV_LOG(ERR).

---

### PATCH v6 05/10: net/mlx5: fix Rx split segment counter type

**Summary:** Changes rxseg_n from uint32_t to uint16_t.

Looks good. Removes struct padding.

---

### PATCH v6 06/10: net/mlx5: support selective Rx

**Summary:** Implements selective Rx in mlx5_rx_burst().

> +static struct rte_mbuf *
> +mlx5_alloc_null_mbuf(uint32_t data_len)
> +{
> +	size_t alloc_size = sizeof(struct rte_mbuf) + RTE_PKTMBUF_HEADROOM +
> +		rte_align32pow2(data_len);
> +	struct rte_mbuf *m;
> +
> +	m = mlx5_malloc(MLX5_MEM_ZERO, alloc_size, 0, SOCKET_ID_ANY);
> +	if (m == NULL)
> +		return NULL;

Does rte_align32pow2(data_len) handle data_len == 0? Might return 1, which is fine.

> +	m->buf_addr = RTE_PTR_ADD(m, sizeof(*m));
> +	m->buf_len = alloc_size - sizeof(*m);
> +	rte_mbuf_iova_set(m, rte_mem_virt2iova(m->buf_addr));

Does rte_mem_virt2iova fail for mlx5_malloc memory? If it returns RTE_BAD_IOVA, the driver might crash. Should validate.

> +	m->data_off = RTE_PKTMBUF_HEADROOM;
> +	m->refcnt = 1;
> +	m->nb_segs = 1;
> +	m->port = RTE_MBUF_PORT_INVALID;
> +	m->pool = NULL;
> +	return m;
> +}

Reasonable fake mbuf construction.

> @@ -1044,11 +1044,14 @@ 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;

Does tail need initialization? It's set before use if pkt exists, but clarity would help.

> @@ -1056,14 +1059,18 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  		struct rte_mbuf *rep = (*rxq->elts)[idx];
> -		volatile struct mlx5_mini_cqe8 *mcqe = NULL;
>  
> -		if (pkt)
> -			NEXT(seg) = rep;
> +		if (pkt) {
> +			if (rep->pool)
> +				NEXT(tail) = rep;
> +			else
> +				--NB_SEGS(pkt);
> +		}

Does this correctly handle NB_SEGS underflow? If pkt starts with NB_SEGS == 1 and we decrement for the first discard segment, it becomes 0. Then if we try to decrement again, it wraps to 65535. Should check NB_SEGS(pkt) > 0 before decrementing.

[ ... ]

> @@ -1167,20 +1178,37 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  		/* If there's only one MR, no need to replace LKey in WQE. */
>  		if (unlikely(mlx5_mr_btree_len(&rxq->mr_ctrl.cache_bh) > 1))
>  			wqe->lkey = mlx5_rx_mb2mr(rxq, rep);
> +		}
>  		if (len > DATA_LEN(seg)) {
> +			if (seg->pool)
> +				data_seg_len += DATA_LEN(seg);
>  			len -= DATA_LEN(seg);
> +			if (pkt)
>  			++NB_SEGS(pkt);
>  			++rq_ci;
>  			continue;
>  		}
> +		if (seg->pool) { /* last segment */
>  		DATA_LEN(seg) = len;
> +		data_seg_len += len;
> +		}
> +		if (unlikely(!pkt)) { /* no real segment found, skip packet */

Does this leak rep if pkt is NULL? The rep was allocated earlier if seg->pool was set. Need to check the control flow more carefully.

Actually, looking