RE: [PATCH v2 12/15] net/intel: introduce infrastructure for Rx path selection

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

> 
> On Thu, Aug 07, 2025 at 12:39:46PM +0000, Ciara Loftus wrote:
> > The code for determining which Rx path to select during initialisation
> > has become complicated in many intel drivers due to the amount of
> > different paths and features available within each path. This commit
> > aims to simplify and genericize the path selection logic.
> >
> > The following information about each Rx burst function is stored and
> > used by the new common function to select the appropriate Rx path:
> > - Rx Offloads
> > - SIMD bitwidth
> > - Flexible RXD usage
> > - Bulk alloc function
> > - Scattered function
> > ---
> > v2:
> > * renamed various items from "burst" to "path"
> > * added missing doxygen comments
> > * attempted to improve the logic in the select function around the bulk
> >  alloc feature
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> 
> Two comments inline below.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> 
> > ---
> >  drivers/net/intel/common/rx.h | 103
> ++++++++++++++++++++++++++++++++++
> >  1 file changed, 103 insertions(+)
> >
> > diff --git a/drivers/net/intel/common/rx.h b/drivers/net/intel/common/rx.h
> > index 70b597e8dc..6d134622e6 100644
> > --- a/drivers/net/intel/common/rx.h
> > +++ b/drivers/net/intel/common/rx.h
> > @@ -10,6 +10,7 @@
> >  #include <unistd.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_ethdev.h>
> > +#include <rte_vect.h>
> >
> >  #include "desc.h"
> >
> > @@ -125,6 +126,26 @@ struct ci_rx_queue {
> >  	};
> >  };
> >
> > +#define CI_RX_PATH_SCATTERED 1
> > +#define CI_RX_PATH_FLEX_DESC 1
> > +#define CI_RX_PATH_BULK_ALLOC 1
> > +#define CI_RX_PATH_DISABLED 1
> > +
> 
> Do we need these defines? They are not used in this patch anyway. With the
> values converted to "bool", using true/false should be fine I think.

They're not necessarily needed but I think they improve the readability when they are used in the next three patches when defining the features of each Rx burst function. If you don't agree I can remove them.

> 
> > +struct ci_rx_path_features {
> > +	uint32_t rx_offloads;
> > +	enum rte_vect_max_simd simd_width;
> > +	bool scattered;
> > +	bool flex_desc;
> > +	bool bulk_alloc;
> > +	bool disabled;
> > +};
> > +
> > +struct ci_rx_path_info {
> > +	eth_rx_burst_t pkt_burst;
> > +	const char *info;
> > +	struct ci_rx_path_features features;
> > +};
> > +
> >  static inline uint16_t
> >  ci_rx_reassemble_packets(struct rte_mbuf **rx_bufs, uint16_t nb_bufs,
> uint8_t *split_flags,
> >  		struct rte_mbuf **pkt_first_seg, struct rte_mbuf
> **pkt_last_seg,
> > @@ -222,4 +243,86 @@ ci_rxq_vec_capable(uint16_t nb_desc, uint16_t
> rx_free_thresh, uint64_t offloads)
> >  	return true;
> >  }
> >
> > +/**
> > + * Select the best matching Rx path based on features
> > + *
> > + * @param req_features
> > + *   The requested features for the Rx path
> > + * @param infos
> > + *   Array of information about the available Rx paths
> > + * @param num_paths
> > + *   Number of available paths in the infos array
> > + * @param default_path
> > + *   Index of the default path to use if no suitable path is found
> > + *
> > + * @return
> > + *   The packet burst function index that best matches the requested
> features,
> > + *   or default_path if no suitable path is found
> > + */
> > +static inline int
> > +ci_rx_path_select(struct ci_rx_path_features req_features,
> > +			const struct ci_rx_path_info *infos,
> > +			int num_paths,
> > +			int default_path)
> > +{
> > +	int i, idx = -1;
> > +	const struct ci_rx_path_features *current_features = NULL;
> > +
> > +	for (i = 0; i < num_paths; i++) {
> > +		const struct ci_rx_path_features *path_features =
> &infos[i].features;
> > +
> > +		/* Do not select a disabled rx path. */
> > +		if (path_features->disabled)
> > +			continue;
> > +
> > +		/* If requested, ensure the path uses the flexible descriptor. */
> > +		if (path_features->flex_desc != req_features.flex_desc)
> > +			continue;
> > +
> > +		/* If requested, ensure the path supports scattered RX. */
> > +		if (path_features->scattered != req_features.scattered)
> > +			continue;
> > +
> 
> I think this test should be in a similar format for the next bulk-alloc
> one. After all, if scattered Rx is requested, we must provide a scattered
> path. However, if scattered is not requested, a scattered Rx path should
> still work fine. So:
> 
> 	if (req_features.scattered && !path_features.scattered)
> 		continue;

This would be a diversion from existing behaviour. Currently we never select a scattered function when it is not requested.
Like you said it would still work, but the path selected may not be what users have come to expect.

> 
> > +		/* Do not use a bulk alloc path if not requested. */
> > +		if (path_features->bulk_alloc && !req_features.bulk_alloc)
> > +			continue;
> > +
> > +		/* Ensure the path supports the requested RX offloads. */
> > +		if ((path_features->rx_offloads & req_features.rx_offloads) !=
> > +				req_features.rx_offloads)
> > +			continue;
> > +
> > +		/* Ensure the path's SIMD width is compatible with the
> requested width. */
> > +		if (path_features->simd_width > req_features.simd_width)
> > +			continue;
> > +
> > +		/* Do not select the path if it is less suitable than the current
> path. */
> > +		if (current_features != NULL) {
> > +			/* 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) >
> > +					rte_popcount32(current_features-
> >rx_offloads))
> > +				continue;
> > +			/* Do not select paths without bulk alloc support if
> requested and the
> > +			 * current path already meets this requirement.
> > +			 */
> > +			if (!path_features->bulk_alloc &&
> req_features.bulk_alloc &&
> > +					current_features->bulk_alloc)
> > +				continue;
> > +		}
> > +
> > +		/* Finally, select the path since it has met all the requirements.
> */
> > +		idx = i;
> > +		current_features = &infos[idx].features;
> > +	}
> > +
> > +	/* No path was found so use the default. */
> > +	if (idx == -1)
> > +		return default_path;
> > +
> 
> Very minor nit - I think you don't need "-1" as a sentinal value. Just
> initialize "idx = default_path" and drop this block. From what I see you
> never make comparisons based on idx, you use current_features for that.

+1 I think this was a left over from some logic I removed in v1/v2.

> 
> > +	return idx;
> > +}
> > +
> >  #endif /* _COMMON_INTEL_RX_H_ */
> > --
> > 2.34.1
> >