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

[Bruce Richardson <bruce.richardson@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?

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