> >
> > 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.
> >
>
> I'd tend to prefer assigning bools to just true/false for type-safety and
> type-clarity. In terms of readability with defines like above: if we do
> want separate defines for scattered, flex-desc etc. I'd tend toward having
> them as enums and the variables below typed using those enums.
> Unfortunately, that's not really feasible with the struct below, because
> while bools only occupy 1 byte, even a two-element enum would use up 4
> bytes per value, which is padding we cannot afford. :-(
>
>
I went with splitting out the boolean features into their own sub-structure,
which you suggested in a previous thread.
The definition can look like this now:
[I40E_RX_AVX512_SCATTERED] = { i40e_recv_scattered_pkts_vec_avx512,
"Vector AVX512 Scattered", {I40E_RX_VECTOR_OFFLOADS, RTE_VECT_SIMD_512,
{.scattered = true, .bulk_alloc = true}}},
Let me know what you think. It's perhaps a bit unusual with the nested
structures but I'm ok with that since it's more readable than a series of
true/false. However, if you would like to just go with the simple true/false
approach I'm happy to spin up a new patch.
Ciara
> > >
> > > > +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.
> >
>
> True. I suppose if we put this in place, we would also have to put in a
> later check for whether the currently selected path is scattered and, if
> scattered Rx is not requested, choose an otherwise equivalent
> non-scattered path over it.
>
> Given that we are looking for compatibility with what we have now, I'm ok
> to keep things as they are.
>
> /Bruce
