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