Re: [PATCH v4 19/25] net/intel: generalize vectorized Rx rearm

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

On Fri, May 30, 2025 at 02:57:15PM +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.
> 
> While we're at it, also make sure to use common definitions for things like
> burst size, rearm threshold, and descriptors per loop, which is currently
> defined separately in each driver.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

One minor comment inline below.

> Notes:
>     v3 -> v4:
>     - Rename rx_vec_sse.h to rx_vec_x86.h
>     - Use the common descriptor format instead of constant propagation
>     - Use the new unified definitions for burst size, rearm threshold, and descriptors per loop
>     - Whitespace and variable name cleanups for vector code
> 
>  drivers/net/intel/common/rx.h               |   4 +
>  drivers/net/intel/common/rx_vec_x86.h       | 303 ++++++++++++++++++++
>  drivers/net/intel/ice/ice_rxtx.h            |  12 +-
>  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, 322 insertions(+), 317 deletions(-)
>  create mode 100644 drivers/net/intel/common/rx_vec_x86.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 8d5466eb44..cf83994c47 100644
> --- a/drivers/net/intel/common/rx.h
> +++ b/drivers/net/intel/common/rx.h
> @@ -15,6 +15,10 @@
>  
>  #define CI_RX_MAX_BURST 32
>  #define CI_RX_MAX_NSEG 2
> +#define CI_VPMD_RX_BURST            32
> +#define CI_VPMD_DESCS_PER_LOOP      4
> +#define CI_VPMD_DESCS_PER_LOOP_WIDE 8
> +#define CI_VPMD_RX_REARM_THRESH     CI_VPMD_RX_BURST
>  
>  struct ci_rx_queue;
>  
> diff --git a/drivers/net/intel/common/rx_vec_x86.h b/drivers/net/intel/common/rx_vec_x86.h
> new file mode 100644
> index 0000000000..7c57016df7

<snip>

> +
> +static __rte_always_inline void
> +ci_rxq_rearm(struct ci_rx_queue *rxq, const enum ci_rx_vec_level vec_level)

I think a comment on this function would be good to point out that since
it's inlined from the header and the final parameter is a compile-time
constant, the compiler eliminates all the branches in the switch statement
below.

> +{
> +	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) < 0)
> +		return;
> +
> +#ifdef RTE_NET_INTEL_USE_16BYTE_DESC
> +	switch (vec_level) {
> +	case CI_RX_VEC_LEVEL_AVX512:
> +#ifdef __AVX512VL__
> +		_ci_rxq_rearm_avx512(rxq);
> +		break;
> +#else
> +		/* fall back to AVX2 */
> +		/* fall through */
> +#endif
> +	case CI_RX_VEC_LEVEL_AVX2:
> +#ifdef __AVX2__
> +		_ci_rxq_rearm_avx2(rxq);
> +		break;
> +#else
> +		/* fall back to SSE */
> +		/* 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 */
> +	switch (vec_level) {
> +	case CI_RX_VEC_LEVEL_AVX512:
> +	case CI_RX_VEC_LEVEL_AVX2:
> +	case CI_RX_VEC_LEVEL_SSE:
> +		_ci_rxq_rearm_sse(rxq);
> +		break;
> +	}
> +#endif /* RTE_NET_INTEL_USE_16BYTE_DESC */
<snip>