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

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

On Wed, Nov 12, 2025 at 03:52:59PM +0000, Loftus, Ciara wrote:
> > 
> > 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.
> 
I'll take this as-is so, and you can base the follow-on on top of this.