RE: [PATCH] net/intel: ensure correct Rx path is selected

["Loftus, Ciara" <ciara.loftus@intel.com>]

> 
> On Wed, Nov 12, 2025 at 03:11:23PM +0000, Ciara Loftus wrote:
> > The common rx path selection logic iterates through an array of
> > candidate paths and selects the best fit for the requested features.
> > Currently, in the event that two potential candidates are identified,
> > the one with the fewer offloads (and thus less complex path) is
> > selected. However this is not correct, because if the path with more
> > offloads has a greater SIMD width, that should be chosen. This commit
> > reworks the logic so that the number of offloads is only taken into
> > consideration when choosing between two paths with the same SIMD
> width.
> >
> > Since the paths arrays are ordered from lowest SIMD width to highest,
> > and vector paths tend to have fewer offloads enabled than scalar paths,
> > "new" candidate paths with greater SIMDs widths tended to have fewer or
> > equal offloads than the "current" candidate paths and thus were
> > correctly accepted as the best candidate. For this reason the incorrect
> > logic did not cause any incorrect path selections in practise.
> >
> > Fixes: 9d99641d80a0 ("net/intel: introduce infrastructure for Rx path
> selection")
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> But see one comment inline below.
> 
> >  drivers/net/intel/common/rx.h | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/intel/common/rx.h b/drivers/net/intel/common/rx.h
> > index 5012e4fced..9fa3cdc64d 100644
> > --- a/drivers/net/intel/common/rx.h
> > +++ b/drivers/net/intel/common/rx.h
> > @@ -300,8 +300,11 @@ ci_rx_path_select(struct ci_rx_path_features
> req_features,
> >  			/* Do not select paths with lower SIMD width than the
> current path. */
> >  			if (path_features->simd_width < current_features-
> >simd_width)
> >  				continue;
> > -			/* Do not select paths with more offloads enabled
> than the current path. */
> > -			if (rte_popcount32(path_features->rx_offloads) >
> > +			/* Do not select paths with more offloads enabled
> than the current path if
> > +			 * the SIMD widths are the same.
> > +			 */
> > +			if (path_features->simd_width == current_features-
> >simd_width &&
> > +					rte_popcount32(path_features-
> >rx_offloads) >
> >  					rte_popcount32(current_features-
> >rx_offloads))
> >  				continue;
> 
> The logic is correct, but reviewing this in isolation is a little confusing
> because of the naming. Normally "current" implies the item we are currently
> checking, so I assumed that path was the currently selected path, but it's
> not. The roles are actually reversed, and "path" is the one being checked,
> and current is the chosen one.
> 
> A simple fix that I think would help readability is to rename "current" to
> "chosen_path". We could also rename "path" to "curr_path" in that case, but
> I think it's unnecessary, as "chosen_path" and "path" should be relatively
> clear.
> 
> What do you think?

+1 definitely agree the naming could be improved. I'll submit a follow-on patch.

> 
> >  			/* Do not select paths without bulk alloc support if
> requested and the
> > --
> > 2.34.1
> >