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

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

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