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

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

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

> +		/* 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.

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