Re: [PATCH v3 1/2] net/idpf: enable AVX2 for split queue Rx

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Sep 26, 2025 at 02:24:03PM +0530, Shaiq Wani wrote:
> In case some CPUs don't support AVX512. Enable AVX2 for them to
> get better per-core performance.
> 
> In the single queue model, the same descriptor queue is used by SW
> to post descriptors to the device and used by device to report completed
> descriptors to SW. While as the split queue model separates them into
> different queues for parallel processing and improved performance.
> 
> Signed-off-by: Shaiq Wani <shaiq.wani@intel.com>

Hi Shaiq,

more review comments inline below.

/Bruce

> ---
>  drivers/net/intel/idpf/idpf_common_device.h   |   3 +-
>  drivers/net/intel/idpf/idpf_common_rxtx.c     |   9 +-
>  drivers/net/intel/idpf/idpf_common_rxtx.h     |   3 +
>  .../net/intel/idpf/idpf_common_rxtx_avx2.c    | 242 ++++++++++++++++++
>  4 files changed, 255 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/intel/idpf/idpf_common_device.h b/drivers/net/intel/idpf/idpf_common_device.h
> index 3b95d519c6..982849dafd 100644
> --- a/drivers/net/intel/idpf/idpf_common_device.h
> +++ b/drivers/net/intel/idpf/idpf_common_device.h
> @@ -49,8 +49,9 @@ enum idpf_rx_func_type {
>  	IDPF_RX_SINGLEQ,
>  	IDPF_RX_SINGLEQ_SCATTERED,
>  	IDPF_RX_SINGLEQ_AVX2,
> +	IDPF_RX_AVX2,
>  	IDPF_RX_AVX512,
> -	IDPF_RX_SINGLQ_AVX512,
> +	IDPF_RX_SINGLEQ_AVX512,
>  	IDPF_RX_MAX
>  };
>  
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.c b/drivers/net/intel/idpf/idpf_common_rxtx.c
> index a2b8c372d6..57753180a2 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx.c
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx.c
> @@ -1656,6 +1656,13 @@ const struct ci_rx_path_info idpf_rx_path_infos[] = {
>  			.rx_offloads = IDPF_RX_VECTOR_OFFLOADS,
>  			.simd_width = RTE_VECT_SIMD_256,
>  			.extra.single_queue = true}},
> +	[IDPF_RX_AVX2] = {
> +	       .pkt_burst = idpf_dp_splitq_recv_pkts_avx2,
> +	       .info = "Split AVX2 Vector",
> +	       .features = {
> +		       .rx_offloads = IDPF_RX_VECTOR_OFFLOADS,
> +		       .simd_width = RTE_VECT_SIMD_256,
> +	       }},
>  #ifdef CC_AVX512_SUPPORT
>  	[IDPF_RX_AVX512] = {
>  		.pkt_burst = idpf_dp_splitq_recv_pkts_avx512,
> @@ -1663,7 +1670,7 @@ const struct ci_rx_path_info idpf_rx_path_infos[] = {
>  		.features = {
>  			.rx_offloads = IDPF_RX_VECTOR_OFFLOADS,
>  			.simd_width = RTE_VECT_SIMD_512}},
> -	[IDPF_RX_SINGLQ_AVX512] = {
> +	[IDPF_RX_SINGLEQ_AVX512] = {

This renaming is good, but should really be in a separate patch as it's not
part of the AVX2 changes. Can you put it in a new small patch 1 in this
set.

>  		.pkt_burst = idpf_dp_singleq_recv_pkts_avx512,
>  		.info = "Single AVX512 Vector",
>  		.features = {
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.h b/drivers/net/intel/idpf/idpf_common_rxtx.h
> index 3bc3323af4..3a9af06c86 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx.h
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx.h
> @@ -252,6 +252,9 @@ __rte_internal
>  uint16_t idpf_dp_splitq_xmit_pkts_avx512(void *tx_queue, struct rte_mbuf **tx_pkts,
>  					 uint16_t nb_pkts);
>  __rte_internal
> +uint16_t idpf_dp_splitq_recv_pkts_avx2(void *rxq, struct rte_mbuf **rx_pkts,
> +				     uint16_t nb_pkts);
> +__rte_internal
>  uint16_t idpf_dp_singleq_recv_scatter_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>  			  uint16_t nb_pkts);
>  __rte_internal
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx_avx2.c b/drivers/net/intel/idpf/idpf_common_rxtx_avx2.c
> index 21c8f79254..b00f85ce78 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx_avx2.c
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx_avx2.c
> @@ -482,6 +482,248 @@ idpf_dp_singleq_recv_pkts_avx2(void *rx_queue, struct rte_mbuf **rx_pkts, uint16
>  	return _idpf_singleq_recv_raw_pkts_vec_avx2(rx_queue, rx_pkts, nb_pkts);
>  }
>  
> +static __rte_always_inline void
> +idpf_splitq_rearm_common(struct idpf_rx_queue *rx_bufq)
> +{
> +	int i;
> +	uint16_t rx_id;
> +	volatile union virtchnl2_rx_buf_desc *rxdp = rx_bufq->rx_ring;
> +	struct rte_mbuf **rxep = &rx_bufq->sw_ring[rx_bufq->rxrearm_start];
> +
> +	rxdp += rx_bufq->rxrearm_start;
> +
> +	/* Try to bulk allocate mbufs from mempool */
> +	if (rte_mbuf_raw_alloc_bulk(rx_bufq->mp,
> +				rxep,
> +				IDPF_RXQ_REARM_THRESH) < 0) {
> +		if (rx_bufq->rxrearm_nb + IDPF_RXQ_REARM_THRESH >= rx_bufq->nb_rx_desc) {
> +			__m128i zero_dma = _mm_setzero_si128();
> +
> +			for (i = 0; i < IDPF_VPMD_DESCS_PER_LOOP; i++) {
> +				rxep[i] = &rx_bufq->fake_mbuf;
> +				_mm_storeu_si128((__m128i *)(uintptr_t)&rxdp[i], zero_dma);
> +			}
> +		}
> +			rte_atomic_fetch_add_explicit(&rx_bufq->rx_stats.mbuf_alloc_failed,
> +							IDPF_RXQ_REARM_THRESH,
> +							rte_memory_order_relaxed);
> +		return;
> +	}
> +
> +	__m128i headroom = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM, RTE_PKTMBUF_HEADROOM);
> +
> +	for (i = 0; i < IDPF_RXQ_REARM_THRESH; i += 2, rxep += 2, rxdp += 2) {
> +		struct rte_mbuf *mb0 = rxep[0];
> +		struct rte_mbuf *mb1 = rxep[1];
> +
> +		__m128i buf_addr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> +		__m128i buf_addr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> +		__m128i dma_addr0 = _mm_unpackhi_epi64(buf_addr0, buf_addr0);
> +		__m128i dma_addr1 = _mm_unpackhi_epi64(buf_addr1, buf_addr1);
> +
> +		dma_addr0 = _mm_add_epi64(dma_addr0, headroom);
> +		dma_addr1 = _mm_add_epi64(dma_addr1, headroom);
> +
> +		rxdp[0].split_rd.pkt_addr = _mm_cvtsi128_si64(dma_addr0);
> +		rxdp[1].split_rd.pkt_addr = _mm_cvtsi128_si64(dma_addr1);
> +	}
> +
> +	rx_bufq->rxrearm_start += IDPF_RXQ_REARM_THRESH;
> +	if (rx_bufq->rxrearm_start >= rx_bufq->nb_rx_desc)
> +		rx_bufq->rxrearm_start = 0;
> +
> +	rx_bufq->rxrearm_nb -= IDPF_RXQ_REARM_THRESH;
> +
> +	rx_id = (uint16_t)((rx_bufq->rxrearm_start == 0) ?
> +			(rx_bufq->nb_rx_desc - 1) : (rx_bufq->rxrearm_start - 1));
> +
> +	IDPF_PCI_REG_WRITE(rx_bufq->qrx_tail, rx_id);
> +}

Missed this on last review.

This code is almost, almost identical to the function with the exact same
name in idpf_common_rxtx_avx512.c - and the differences don't seem to be
due to avx2/avx512. Rather than duplicating code, put this in a common
location and use it from both avx2 and avx512 files.

> +
> +static __rte_always_inline void
> +idpf_splitq_rearm_avx2(struct idpf_rx_queue *rx_bufq)
> +{
> +	int i;
> +	uint16_t rx_id;
> +	volatile union virtchnl2_rx_buf_desc *rxdp = rx_bufq->rx_ring;
> +	struct rte_mempool_cache *cache =
> +		rte_mempool_default_cache(rx_bufq->mp, rte_lcore_id());
> +	struct rte_mbuf **rxp = &rx_bufq->sw_ring[rx_bufq->rxrearm_start];
> +
> +	rxdp += rx_bufq->rxrearm_start;
> +
> +	if (unlikely(!cache)) {
> +		idpf_splitq_rearm_common(rx_bufq);
> +		return;
> +	}
> +
> +	if (cache->len < IDPF_RXQ_REARM_THRESH) {
> +		uint32_t req = IDPF_RXQ_REARM_THRESH + (cache->size - cache->len);
> +		int ret = rte_mempool_ops_dequeue_bulk(rx_bufq->mp,
> +						&cache->objs[cache->len], req);
> +		if (ret == 0) {
> +			cache->len += req;
> +		} else {
> +			if (rx_bufq->rxrearm_nb + IDPF_RXQ_REARM_THRESH >=
> +				rx_bufq->nb_rx_desc) {
> +				__m128i dma_addr0 = _mm_setzero_si128();
> +				for (i = 0; i < IDPF_VPMD_DESCS_PER_LOOP; i++) {
> +					rxp[i] = &rx_bufq->fake_mbuf;
> +					_mm_storeu_si128(RTE_CAST_PTR(__m128i *, &rxdp[i]),
> +					dma_addr0);
> +				}
> +			}
> +			rte_atomic_fetch_add_explicit(&rx_bufq->rx_stats.mbuf_alloc_failed,
> +				IDPF_RXQ_REARM_THRESH, rte_memory_order_relaxed);
> +			return;
> +		}
> +	}
> +	__m128i headroom = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM, RTE_PKTMBUF_HEADROOM);
> +	const int step = 2;
> +
> +	for (i = 0; i < IDPF_RXQ_REARM_THRESH; i += step, rxp += step, rxdp += step) {
> +		struct rte_mbuf *mb0 = (struct rte_mbuf *)cache->objs[--cache->len];
> +		struct rte_mbuf *mb1 = (struct rte_mbuf *)cache->objs[--cache->len];
> +		rxp[0] = mb0;
> +		rxp[1] = mb1;
> +
> +		__m128i buf_addr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> +		__m128i buf_addr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> +		__m128i dma_addr0 = _mm_unpackhi_epi64(buf_addr0, buf_addr0);
> +		__m128i dma_addr1 = _mm_unpackhi_epi64(buf_addr1, buf_addr1);
> +
> +		dma_addr0 = _mm_add_epi64(dma_addr0, headroom);
> +		dma_addr1 = _mm_add_epi64(dma_addr1, headroom);
> +
> +		rxdp[0].split_rd.pkt_addr = _mm_cvtsi128_si64(dma_addr0);
> +		rxdp[1].split_rd.pkt_addr = _mm_cvtsi128_si64(dma_addr1);
> +	}
> +

And this code is very much the same as the "common" function above, in fact
the main block looks copy-pasted. Please rework to cut down on duplication?
How much perf benefit is got from this avx2-specific function vs the more
generic "common" one above?

> +	rx_bufq->rxrearm_start += IDPF_RXQ_REARM_THRESH;
> +	if (rx_bufq->rxrearm_start >= rx_bufq->nb_rx_desc)
> +		rx_bufq->rxrearm_start = 0;
> +
> +	rx_bufq->rxrearm_nb -= IDPF_RXQ_REARM_THRESH;
> +
> +	rx_id = (uint16_t)((rx_bufq->rxrearm_start == 0) ?
> +				(rx_bufq->nb_rx_desc - 1) : (rx_bufq->rxrearm_start - 1));
> +
> +	IDPF_PCI_REG_WRITE(rx_bufq->qrx_tail, rx_id);
> +}
> +uint16_t
> +idpf_dp_splitq_recv_pkts_avx2(void *rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
> +{
> +	struct idpf_rx_queue *queue = (struct idpf_rx_queue *)rxq;
> +	const uint32_t *ptype_tbl = queue->adapter->ptype_tbl;
> +	struct rte_mbuf **sw_ring = &queue->bufq2->sw_ring[queue->rx_tail];
> +	volatile union virtchnl2_rx_desc *rxdp =
> +		(volatile union virtchnl2_rx_desc *)queue->rx_ring + queue->rx_tail;
> +
> +	rte_prefetch0(rxdp);
> +	nb_pkts = RTE_ALIGN_FLOOR(nb_pkts, 4); /* 4 desc per AVX2 iteration */
> +
> +	if (queue->bufq2->rxrearm_nb > IDPF_RXQ_REARM_THRESH)
> +		idpf_splitq_rearm_avx2(queue->bufq2);
> +
> +	uint64_t head_gen = rxdp->flex_adv_nic_3_wb.pktlen_gen_bufq_id;
> +	if (((head_gen >> VIRTCHNL2_RX_FLEX_DESC_ADV_GEN_S) &
> +		 VIRTCHNL2_RX_FLEX_DESC_ADV_GEN_M) != queue->expected_gen_id)
> +		return 0;
> +
> +	const __m128i gen_mask =
> +		_mm_set1_epi64x(((uint64_t)queue->expected_gen_id) << 46);
> +
> +	uint16_t received = 0;
> +	for (uint16_t i = 0; i < nb_pkts; i += 4, rxdp += 4) {
> +		/* Step 1: pull mbufs */
> +		__m128i ptrs = _mm_loadu_si128((__m128i *)&sw_ring[i]);
> +		_mm_storeu_si128((__m128i *)&rx_pkts[i], ptrs);
> +

How does this work on 64-bit? An SSE load/store is 16 bytes, which is only
2 pointers 64-bit (4 on 32-bit). Am I missing somewhere where you
load/store the other two pointers per iteration?

> +		/* Step 2: load descriptors */
> +		__m128i d0 = _mm_load_si128(RTE_CAST_PTR(const __m128i *, &rxdp[0]));
> +		rte_compiler_barrier();
> +		__m128i d1 = _mm_load_si128(RTE_CAST_PTR(const __m128i *, &rxdp[1]));
> +		rte_compiler_barrier();
> +		__m128i d2 = _mm_load_si128(RTE_CAST_PTR(const __m128i *, &rxdp[2]));
> +		rte_compiler_barrier();
> +		__m128i d3 = _mm_load_si128(RTE_CAST_PTR(const __m128i *, &rxdp[3]));
> +
> +		/* Step 3: shuffle out pkt_len, data_len, vlan, rss */
> +		const __m256i shuf = _mm256_set_epi8(
> +			/* descriptor 3 */
> +			0xFF, 0xFF, 0xFF, 0xFF, 11, 10, 5, 4,
> +			0xFF, 0xFF, 5, 4, 0xFF, 0xFF, 0xFF, 0xFF,
> +			/* descriptor 2 */

By descriptor 3 and descriptor 2 do you maybe mean descriptors 1 and 0?

> +			0xFF, 0xFF, 0xFF, 0xFF, 11, 10, 5, 4,
> +			0xFF, 0xFF, 5, 4, 0xFF, 0xFF, 0xFF, 0xFF
> +		);
> +		__m128i d01_lo = d0, d01_hi = d1;
> +		__m128i d23_lo = d2, d23_hi = d3;

These variable assignments seem rather pointless.

> +
> +		__m256i m23 = _mm256_shuffle_epi8(_mm256_set_m128i(d23_hi, d23_lo), shuf);
> +		__m256i m01 = _mm256_shuffle_epi8(_mm256_set_m128i(d01_hi, d01_lo), shuf);
> +
> +		/* Step 4: extract ptypes */
> +		const __m256i ptype_mask = _mm256_set1_epi16(VIRTCHNL2_RX_FLEX_DESC_PTYPE_M);
> +		__m256i pt23 = _mm256_and_si256(_mm256_set_m128i(d23_hi, d23_lo), ptype_mask);
> +		__m256i pt01 = _mm256_and_si256(_mm256_set_m128i(d01_hi, d01_lo), ptype_mask);

I imagine the compiler is smart enough to realise it and optimize it away,
but you are still merging the descriptor pairs twice here, ones with the
shuffle and a second time here when doing masking. Rather than renaming the
variables as hi and lo 128bit values, why not merge them there into 256-bit
values.

> +
> +		uint16_t ptype2 = _mm256_extract_epi16(pt23, 1);
> +		uint16_t ptype3 = _mm256_extract_epi16(pt23, 9);
> +		uint16_t ptype0 = _mm256_extract_epi16(pt01, 1);
> +		uint16_t ptype1 = _mm256_extract_epi16(pt01, 9);
> +
> +		m23 = _mm256_insert_epi32(m23, ptype_tbl[ptype3], 2);
> +		m23 = _mm256_insert_epi32(m23, ptype_tbl[ptype2], 0);
> +		m01 = _mm256_insert_epi32(m01, ptype_tbl[ptype1], 2);
> +		m01 = _mm256_insert_epi32(m01, ptype_tbl[ptype0], 0);
> +
> +		/* Step 5: extract gen bits */
> +		__m128i sts0 = _mm_srli_epi64(d0, 46);
> +		__m128i sts1 = _mm_srli_epi64(d1, 46);
> +		__m128i sts2 = _mm_srli_epi64(d2, 46);
> +		__m128i sts3 = _mm_srli_epi64(d3, 46);
> +
> +		__m128i merged_lo = _mm_unpacklo_epi64(sts0, sts2);
> +		__m128i merged_hi = _mm_unpacklo_epi64(sts1, sts3);
> +		__m128i valid = _mm_and_si128(_mm_and_si128(merged_lo, merged_hi),
> +						  _mm_unpacklo_epi64(gen_mask, gen_mask));
> +		__m128i cmp = _mm_cmpeq_epi64(valid, _mm_unpacklo_epi64(gen_mask, gen_mask));
> +		int burst = _mm_movemask_pd(_mm_castsi128_pd(cmp));
> +
> +		/* Step 6: write rearm_data safely */
> +		__m128i m01_lo = _mm256_castsi256_si128(m01);
> +		__m128i m23_lo = _mm256_castsi256_si128(m23);
> +
> +		uint64_t tmp01[2], tmp23[2];
> +		_mm_storeu_si128((__m128i *)tmp01, m01_lo);
> +		_mm_storeu_si128((__m128i *)tmp23, m23_lo);
> +		*(uint64_t *)&rx_pkts[i]->rearm_data = tmp01[0];
> +		*(uint64_t *)&rx_pkts[i + 1]->rearm_data = tmp01[1];
> +		*(uint64_t *)&rx_pkts[i + 2]->rearm_data = tmp23[0];
> +		*(uint64_t *)&rx_pkts[i + 3]->rearm_data = tmp23[1];

Doing additional stores tends to be bad for performance. Extract the data
to do proper stores.

However, I only see 64-bits being written to each mbuf here, covering the
data_off, ref_cnt, nb_segs and port fields, which all can be set to
constant values read from the per-queue or per-port data. The "ice" driver
writes to the rearm-data in the avx2 path because it's doing a 256-bit
store covering the rearm data, the flags and the descriptor metadata. I
think here you are writing the descriptor metdata data to the rearm data
instead. Please check this.

> +
> +		received += burst;
> +		if (burst != 4)
> +			break;
> +	}
> +
> +	queue->rx_tail += received;
> +	if (received & 1) {
> +		queue->rx_tail &= ~(uint16_t)1;
> +		received--;
> +	}
> +	queue->rx_tail &= (queue->nb_rx_desc - 1);
> +	queue->expected_gen_id ^= ((queue->rx_tail & queue->nb_rx_desc) != 0);
> +	queue->bufq2->rxrearm_nb += received;
> +
> +	return received;
> +}
> +
> +RTE_EXPORT_INTERNAL_SYMBOL(idpf_dp_splitq_recv_pkts_avx2)
> +
>  static inline void
>  idpf_singleq_vtx1(volatile struct idpf_base_tx_desc *txdp,
>  		  struct rte_mbuf *pkt, uint64_t flags)
> -- 
> 2.34.1
>