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

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

[Ciara Loftus]

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>
---
 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;
 			/* Do not select paths without bulk alloc support if requested and the
-- 
2.34.1

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

[Bruce Richardson]

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
> 

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

[Loftus, Ciara]

> 
> 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
> >

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

[Bruce Richardson]

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.

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

[Bruce Richardson]

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>
> > 

Applied to dpdk-next-net-intel.

Thanks.

/Bruce