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.lof...@intel.com>
Two comments inline below.
Acked-by: Bruce Richardson <bruce.richard...@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
>
