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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 4 June 2025 11.32
> 
> 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.
> 

[...]

> > +
> > +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:

If you are respinning this patch, add "/* fall through */" here.

> > +	case CI_RX_VEC_LEVEL_AVX2:

And here.

> > +	case CI_RX_VEC_LEVEL_SSE:
> > +		_ci_rxq_rearm_sse(rxq);
> > +		break;
> > +	}
> > +#endif /* RTE_NET_INTEL_USE_16BYTE_DESC */
> <snip>

Please try building with 32-byte descriptors; the compiler should have complained about the implicit fall through.