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.bura...@intel.com>
> ---
>
Acked-by: Bruce Richardson <bruce.richard...@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>
