On Mon, May 12, 2025 at 01:54:33PM +0100, Anatoly Burakov wrote:
> There is certain amount of duplication between various drivers when it
> comes to Rx ring rearm. This patch takes implementation from ice driver
> as a base because it has support for no IOVA in mbuf as well as all
> vector implementations, and moves them to a common file.
>
> The driver Rx rearm code used copious amounts of #ifdef-ery to
> discriminate between 16- and 32-byte descriptor support, but we cannot do
> that in the common code because we will not have access to those
> definitions. So, instead, we use copious amounts of compile-time constant
I was initially wondering why we don't have access to the definitions, but
then I realised it was because the common code doesn't know whether to use
the I40E, ICE or IAVF definition. :-)
However, this leads me to consider whether, if we need to keep these
definitions, we are better to just use a single one, rather than one per
driver.
> propagation and force-inlining to ensure that the compiler generates
> effectively the same code it generated back when it was in the driver. We
> also add a compile-time definition for vectorization levels for x86
> vector instructions to discriminate between different instruction sets.
> This too is constant-propagated, and thus should not affect performance.
>
> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
More comments inline below. While I realise this is mostly a copy-paste
transfer, I think we can do some cleanup in the process.
/Bruce
> ---
> drivers/net/intel/common/rx.h | 3 +
> drivers/net/intel/common/rx_vec_sse.h | 323 ++++++++++++++++++++
> drivers/net/intel/ice/ice_rxtx.h | 2 +-
> drivers/net/intel/ice/ice_rxtx_common_avx.h | 233 --------------
> drivers/net/intel/ice/ice_rxtx_vec_avx2.c | 5 +-
> drivers/net/intel/ice/ice_rxtx_vec_avx512.c | 5 +-
> drivers/net/intel/ice/ice_rxtx_vec_sse.c | 77 +----
> 7 files changed, 336 insertions(+), 312 deletions(-)
> create mode 100644 drivers/net/intel/common/rx_vec_sse.h
> delete mode 100644 drivers/net/intel/ice/ice_rxtx_common_avx.h
>
> diff --git a/drivers/net/intel/common/rx.h b/drivers/net/intel/common/rx.h
> index 2d9328ae89..65e920fdd1 100644
> --- a/drivers/net/intel/common/rx.h
> +++ b/drivers/net/intel/common/rx.h
> @@ -14,6 +14,8 @@
> #define CI_RX_BURST 32
> #define CI_RX_MAX_BURST 32
> #define CI_RX_MAX_NSEG 2
> +#define CI_VPMD_DESCS_PER_LOOP 4
Do all our vector PMDs use the same DESC_PER_LOOP value? Do the AVX2 and
AVX512 paths not do 8 at a time?
> +#define CI_VPMD_RX_REARM_THRESH 64
>
> struct ci_rx_queue;
>
> @@ -40,6 +42,7 @@ struct ci_rx_queue {
> volatile union ice_32b_rx_flex_desc *ice_rx_32b_ring;
> volatile union iavf_16byte_rx_desc *iavf_rx_16b_ring;
> volatile union iavf_32byte_rx_desc *iavf_rx_32b_ring;
> + volatile void *rx_ring; /**< Generic */
> };
> volatile uint8_t *qrx_tail; /**< register address of tail */
> struct ci_rx_entry *sw_ring; /**< address of RX software ring. */
> diff --git a/drivers/net/intel/common/rx_vec_sse.h
> b/drivers/net/intel/common/rx_vec_sse.h
> new file mode 100644
> index 0000000000..6fe0baf38b
> --- /dev/null
> +++ b/drivers/net/intel/common/rx_vec_sse.h
This file should be called "rx_vec_x86.h", I think, since it has got both
SSE and AVX code in it.
> @@ -0,0 +1,323 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Intel Corporation
Date -> 2025
> + */
> +
> +#ifndef _COMMON_INTEL_RX_VEC_SSE_H_
> +#define _COMMON_INTEL_RX_VEC_SSE_H_
> +
> +#include <stdint.h>
> +
> +#include <ethdev_driver.h>
> +#include <rte_io.h>
> +
> +#include "rx.h"
> +
> +enum ci_rx_vec_level {
> + CI_RX_VEC_LEVEL_SSE = 0,
> + CI_RX_VEC_LEVEL_AVX2,
> + CI_RX_VEC_LEVEL_AVX512,
> +};
> +
> +static inline int
> +_ci_rxq_rearm_get_bufs(struct ci_rx_queue *rxq, const size_t desc_len)
> +{
> + struct ci_rx_entry *rxp = &rxq->sw_ring[rxq->rxrearm_start];
> + const uint16_t rearm_thresh = CI_VPMD_RX_REARM_THRESH;
> + volatile void *rxdp;
> + int i;
> +
> + rxdp = RTE_PTR_ADD(rxq->rx_ring, rxq->rxrearm_start * desc_len);
> +
> + if (rte_mempool_get_bulk(rxq->mp,
> + (void **)rxp,
> + rearm_thresh) < 0) {
Since we are copying the code to a new place, maybe we can lengthen the
lines a bit to the 100 char limit. I suspect this can be all on one line,
increasing readability.
> + if (rxq->rxrearm_nb + rearm_thresh >= rxq->nb_rx_desc) {
> + __m128i dma_addr0;
> +
> + dma_addr0 = _mm_setzero_si128();
> + for (i = 0; i < CI_VPMD_DESCS_PER_LOOP; i++) {
> + rxp[i].mbuf = &rxq->fake_mbuf;
> + const void *ptr = RTE_PTR_ADD(rxdp, i *
> desc_len);
If we drop the const here, the cast should not be necessary at all in the
line below, I think.
> + _mm_store_si128(RTE_CAST_PTR(__m128i *, ptr),
> + dma_addr0);
> + }
> + }
> + rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed +=
> rearm_thresh;
> + return -1;
> + }
> + return 0;
> +}
> +
> +/*
> + * SSE code path can handle both 16-byte and 32-byte descriptors with one
> code
> + * path, as we only ever write 16 bytes at a time.
> + */
> +static __rte_always_inline void
> +_ci_rxq_rearm_sse(struct ci_rx_queue *rxq, const size_t desc_len)
> +{
> + const __m128i hdr_room = _mm_set1_epi64x(RTE_PKTMBUF_HEADROOM);
Minor nit, but we are referring to this as "headroom" not "header-room" so
the prefix should probably be "hd_room" (or hdroom), not "hdr_room" :-)
> + const __m128i zero = _mm_setzero_si128();
> + const uint16_t rearm_thresh = CI_VPMD_RX_REARM_THRESH;
> + struct ci_rx_entry *rxp = &rxq->sw_ring[rxq->rxrearm_start];
> + volatile void *rxdp;
> + int i;
> +
> + rxdp = RTE_PTR_ADD(rxq->rx_ring, rxq->rxrearm_start * desc_len);
> +
> + /* Initialize the mbufs in vector, process 2 mbufs in one loop */
> + for (i = 0; i < rearm_thresh; i += 2, rxp += 2, rxdp =
> RTE_PTR_ADD(rxdp, 2 * desc_len)) {
> + volatile void *ptr0 = RTE_PTR_ADD(rxdp, 0);
> + volatile void *ptr1 = RTE_PTR_ADD(rxdp, desc_len);
We don't need the volatile casts here, since we only ever cast them away
when used with store_si128. In fact, I suspect we don't ever need to have
rxdp be volatile either.
> + __m128i vaddr0, vaddr1;
> + __m128i dma_addr0, dma_addr1;
> + struct rte_mbuf *mb0, *mb1;
> +
> + mb0 = rxp[0].mbuf;
> + mb1 = rxp[1].mbuf;
> +
> +#if RTE_IOVA_IN_MBUF
> + /* load buf_addr(lo 64bit) and buf_iova(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> +#endif
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> + /* add headroom to address values */
> + vaddr0 = _mm_add_epi64(vaddr0, hdr_room);
> + vaddr1 = _mm_add_epi64(vaddr1, hdr_room);
> +
> +#if RTE_IOVA_IN_MBUF
> + /* move IOVA to Packet Buffer Address, erase Header Buffer
> Address */
> + dma_addr0 = _mm_unpackhi_epi64(vaddr0, zero);
> + dma_addr1 = _mm_unpackhi_epi64(vaddr1, zero);
> +#else
> + /* erase Header Buffer Address */
> + dma_addr0 = _mm_unpacklo_epi64(vaddr0, zero);
> + dma_addr1 = _mm_unpacklo_epi64(vaddr1, zero);
> +#endif
> +
> + /* flush desc with pa dma_addr */
> + _mm_store_si128(RTE_CAST_PTR(__m128i *, ptr0), dma_addr0);
> + _mm_store_si128(RTE_CAST_PTR(__m128i *, ptr1), dma_addr1);
> + }
> +}
> +
> +#ifdef __AVX2__
> +/* AVX2 version for 16-byte descriptors, handles 4 buffers at a time */
> +static __rte_always_inline void
> +_ci_rxq_rearm_avx2(struct ci_rx_queue *rxq)
> +{
> + struct ci_rx_entry *rxp = &rxq->sw_ring[rxq->rxrearm_start];
> + const uint16_t rearm_thresh = CI_VPMD_RX_REARM_THRESH;
> + const size_t desc_len = 16;
> + volatile void *rxdp;
> + const __m256i hdr_room = _mm256_set1_epi64x(RTE_PKTMBUF_HEADROOM);
> + const __m256i zero = _mm256_setzero_si256();
> + int i;
> +
> + rxdp = RTE_PTR_ADD(rxq->rx_ring, rxq->rxrearm_start * desc_len);
> +
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
> + for (i = 0; i < rearm_thresh; i += 4, rxp += 4, rxdp =
> RTE_PTR_ADD(rxdp, 4 * desc_len)) {
> + volatile void *ptr0 = RTE_PTR_ADD(rxdp, 0);
> + volatile void *ptr1 = RTE_PTR_ADD(rxdp, desc_len * 2);
Again, we can drop volatile, I think.
> + __m128i vaddr0, vaddr1, vaddr2, vaddr3;
> + __m256i vaddr0_1, vaddr2_3;
> + __m256i dma_addr0_1, dma_addr2_3;
> + struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
> +
> + mb0 = rxp[0].mbuf;
> + mb1 = rxp[1].mbuf;
> + mb2 = rxp[2].mbuf;
> + mb3 = rxp[3].mbuf;
> +
> +#if RTE_IOVA_IN_MBUF
> + /* load buf_addr(lo 64bit) and buf_iova(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> +#endif
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> + vaddr2 = _mm_loadu_si128((__m128i *)&mb2->buf_addr);
> + vaddr3 = _mm_loadu_si128((__m128i *)&mb3->buf_addr);
> +
> + /**
> + * merge 0 & 1, by casting 0 to 256-bit and inserting 1
> + * into the high lanes. Similarly for 2 & 3
> + */
> + vaddr0_1 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr0),
> + vaddr1, 1);
Can these statements now fit on a single 100-character line?
> + vaddr2_3 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr2),
> + vaddr3, 1);
> +
> + /* add headroom to address values */
> + vaddr0_1 = _mm256_add_epi64(vaddr0_1, hdr_room);
> + vaddr0_1 = _mm256_add_epi64(vaddr0_1, hdr_room);
> +
> +#if RTE_IOVA_IN_MBUF
> + /* extract IOVA addr into Packet Buffer Address, erase Header
> Buffer Address */
> + dma_addr0_1 = _mm256_unpackhi_epi64(vaddr0_1, zero);
> + dma_addr2_3 = _mm256_unpackhi_epi64(vaddr2_3, zero);
> +#else
> + /* erase Header Buffer Address */
> + dma_addr0_1 = _mm256_unpacklo_epi64(vaddr0_1, zero);
> + dma_addr2_3 = _mm256_unpacklo_epi64(vaddr2_3, zero);
> +#endif
> +
> + /* flush desc with pa dma_addr */
> + _mm256_store_si256(RTE_CAST_PTR(__m256i *, ptr0), dma_addr0_1);
> + _mm256_store_si256(RTE_CAST_PTR(__m256i *, ptr1), dma_addr2_3);
> + }
> +}
> +#endif /* __AVX2__ */
> +
> +#ifdef __AVX512VL__
> +/* AVX512 version for 16-byte descriptors, handles 8 buffers at a time */
> +static __rte_always_inline void
> +_ci_rxq_rearm_avx512(struct ci_rx_queue *rxq)
> +{
> + struct ci_rx_entry *rxp = &rxq->sw_ring[rxq->rxrearm_start];
> + const uint16_t rearm_thresh = CI_VPMD_RX_REARM_THRESH;
> + const size_t desc_len = 16;
> + volatile void *rxdp;
> + int i;
> + struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
> + struct rte_mbuf *mb4, *mb5, *mb6, *mb7;
> + __m512i dma_addr0_3, dma_addr4_7;
> + __m512i hdr_room = _mm512_set1_epi64(RTE_PKTMBUF_HEADROOM);
> + __m512i zero = _mm512_setzero_si512();
> +
> + rxdp = RTE_PTR_ADD(rxq->rx_ring, rxq->rxrearm_start * desc_len);
> +
> + /* Initialize the mbufs in vector, process 8 mbufs in one loop */
> + for (i = 0; i < rearm_thresh; i += 8, rxp += 8, rxdp =
> RTE_PTR_ADD(rxdp, 8 * desc_len)) {
> + volatile void *ptr0 = RTE_PTR_ADD(rxdp, 0);
> + volatile void *ptr1 = RTE_PTR_ADD(rxdp, desc_len * 4);
> + __m128i vaddr0, vaddr1, vaddr2, vaddr3;
> + __m128i vaddr4, vaddr5, vaddr6, vaddr7;
> + __m256i vaddr0_1, vaddr2_3;
> + __m256i vaddr4_5, vaddr6_7;
> + __m512i vaddr0_3, vaddr4_7;
Rather than defining all of these variables here, many of which are
throw-away, if we define them on first use the code will be shorter, just
as readable, and also the variables can be made "const".
> +
> + mb0 = rxp[0].mbuf;
> + mb1 = rxp[1].mbuf;
> + mb2 = rxp[2].mbuf;
> + mb3 = rxp[3].mbuf;
> + mb4 = rxp[4].mbuf;
> + mb5 = rxp[5].mbuf;
> + mb6 = rxp[6].mbuf;
> + mb7 = rxp[7].mbuf;
> +
> +#if RTE_IOVA_IN_MBUF
> + /* load buf_addr(lo 64bit) and buf_iova(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> +#endif
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> + vaddr2 = _mm_loadu_si128((__m128i *)&mb2->buf_addr);
> + vaddr3 = _mm_loadu_si128((__m128i *)&mb3->buf_addr);
> + vaddr4 = _mm_loadu_si128((__m128i *)&mb4->buf_addr);
> + vaddr5 = _mm_loadu_si128((__m128i *)&mb5->buf_addr);
> + vaddr6 = _mm_loadu_si128((__m128i *)&mb6->buf_addr);
> + vaddr7 = _mm_loadu_si128((__m128i *)&mb7->buf_addr);
> +
> + /**
> + * merge 0 & 1, by casting 0 to 256-bit and inserting 1
> + * into the high lanes. Similarly for 2 & 3, and so on.
> + */
> + vaddr0_1 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr0),
> + vaddr1, 1);
> + vaddr2_3 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr2),
> + vaddr3, 1);
> + vaddr4_5 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr4),
> + vaddr5, 1);
> + vaddr6_7 =
> + _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr6),
> + vaddr7, 1);
> + vaddr0_3 =
> + _mm512_inserti64x4(_mm512_castsi256_si512(vaddr0_1),
> + vaddr2_3, 1);
> + vaddr4_7 =
> + _mm512_inserti64x4(_mm512_castsi256_si512(vaddr4_5),
> + vaddr6_7, 1);
> +
> + /* add headroom to address values */
> + vaddr0_3 = _mm512_add_epi64(vaddr0_3, hdr_room);
> + dma_addr4_7 = _mm512_add_epi64(dma_addr4_7, hdr_room);
> +
> +#if RTE_IOVA_IN_MBUF
> + /* extract IOVA addr into Packet Buffer Address, erase Header
> Buffer Address */
> + dma_addr0_3 = _mm512_unpackhi_epi64(vaddr0_3, zero);
> + dma_addr4_7 = _mm512_unpackhi_epi64(vaddr4_7, zero);
> +#else
> + /* erase Header Buffer Address */
> + dma_addr0_3 = _mm512_unpacklo_epi64(vaddr0_3, zero);
> + dma_addr4_7 = _mm512_unpacklo_epi64(vaddr4_7, zero);
> +#endif
> +
> + /* flush desc with pa dma_addr */
> + _mm512_store_si512(RTE_CAST_PTR(__m512i *, ptr0), dma_addr0_3);
> + _mm512_store_si512(RTE_CAST_PTR(__m512i *, ptr1), dma_addr4_7);
> + }
> +}
> +#endif /* __AVX512VL__ */
> +
> +static __rte_always_inline void
> +ci_rxq_rearm(struct ci_rx_queue *rxq, const size_t desc_len,
> + const enum ci_rx_vec_level vec_level)
> +{
> + const uint16_t rearm_thresh = CI_VPMD_RX_REARM_THRESH;
> + uint16_t rx_id;
> +
> + /* Pull 'n' more MBUFs into the software ring */
> + if (_ci_rxq_rearm_get_bufs(rxq, desc_len) < 0)
> + return;
> +
> + if (desc_len == 16) {
> + switch (vec_level) {
> + case CI_RX_VEC_LEVEL_AVX512:
> +#ifdef __AVX512VL__
> + _ci_rxq_rearm_avx512(rxq);
> + break;
> +#else
> + /* fall back to AVX2 unless requested not to */
> + /* fall through */
> +#endif
> + case CI_RX_VEC_LEVEL_AVX2:
> +#ifdef __AVX2__
> + _ci_rxq_rearm_avx2(rxq);
> + break;
> +#else
> + /* fall back to SSE if AVX2 isn't supported */
> + /* fall through */
> +#endif
> + case CI_RX_VEC_LEVEL_SSE:
> + _ci_rxq_rearm_sse(rxq, desc_len);
> + break;
> + }
> + } else {
> + /* for 32-byte descriptors only support SSE */
> + _ci_rxq_rearm_sse(rxq, desc_len);
> + }
> +
> + rxq->rxrearm_start += rearm_thresh;
> + if (rxq->rxrearm_start >= rxq->nb_rx_desc)
> + rxq->rxrearm_start = 0;
> +
> + rxq->rxrearm_nb -= rearm_thresh;
> +
> + rx_id = (uint16_t)((rxq->rxrearm_start == 0) ?
> + (rxq->nb_rx_desc - 1) : (rxq->rxrearm_start - 1));
> +
> + /* Update the tail pointer on the NIC */
> + rte_write32_wc(rte_cpu_to_le_32(rx_id), rxq->qrx_tail);
> +}
> +
> +#endif /* _COMMON_INTEL_RX_VEC_SSE_H_ */
<snip>
