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 <[email protected]>
> ---
Acked-by: Bruce Richardson <[email protected]>
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
>
