On Wed, Jun 04, 2025 at 11:43:01AM +0200, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@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.bura...@intel.com> > > > --- > > > > > > > Acked-by: Bruce Richardson <bruce.richard...@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. > I disagree, it's not needed and will only make the code less readable.
> > > + 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. > The fallthrough tags are not necessary where you have a list of tags without any statements in between. It's common practice to have a couple of values match a single switch arm, and the fallthrough is obvious in those cases, since there are no statements between them. Once you start having some code for a case, then you need it, since you need to make it clear that it's not accidental. /Bruce
