Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information

["Wang, Haiyue" <haiyue.wang@intel.com>]

Hi Thomas & Andrew,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, October 25, 2019 17:37
> To: Wang, Haiyue <haiyue.wang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Sun, Chenmin <chenmin.sun@intel.com>;
> arybchenko@solarflare.com; viacheslavo@mellanox.com; stephen@networkplumber.org;
> david.marchand@redhat.com; jerinj@marvell.com
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information
> 
> 15/10/2019 09:51, Haiyue Wang:
> > Some PMDs have more than one RX/TX burst paths, add the ethdev API
> > that allows an application to retrieve the mode information about
> > Rx/Tx packet burst such as Scalar or Vector, and Vector technology
> > like AVX2.
> 
> I missed this patch. I and Andrew, maintainers of ethdev, were not CC'ed.
> Ferruh, I would expect to be Cc'ed and/or get a notification before merging.
> Hopefully it is not too late to fix this API before releasing 19.11.
> 
> I think the idea of getting infos from PMD internal mode is not bad.
> But I strongly disagree with standardizing the names. More below.
> 
> [...]
> > +enum rte_eth_burst_mode_option {
> > +	RTE_ETH_BURST_SCALAR = (1 << 0),
> > +	RTE_ETH_BURST_VECTOR = (1 << 1),
> 
> 2 bits for a boolean value?
> 

They have the Boolean value, but prefer to keep bit opaque, then can
loop to access all the bits ...

> > +
> > +	/**< bits[15:2] are reserved for each vector type */
> > +	RTE_ETH_BURST_ALTIVEC = (1 << 2),
> > +	RTE_ETH_BURST_NEON = (1 << 3),
> > +	RTE_ETH_BURST_SSE = (1 << 4),
> > +	RTE_ETH_BURST_AVX2 = (1 << 5),
> > +	RTE_ETH_BURST_AVX512 = (1 << 6),
> > +
> > +	RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> > +	RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> > +	RTE_ETH_BURST_SIMPLE = (1 << 18),
> 
> What means simple? Looks meaningless.
> 

The below is the 'simple' code, yes, not a good name, will try to extract
more generic name.

	/* Use a simple Tx queue if possible (only fast free is allowed) */
	ad->tx_simple_allowed =
		(txq->offloads ==
		(txq->offloads & DEV_TX_OFFLOAD_MBUF_FAST_FREE) &&
		txq->tx_rs_thresh >= ICE_TX_MAX_BURST);


> > +	RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> 
> What is per-queue burst? No need to add a comment if not adding any info.
> The burst API is *already* per-queue.
> 

Not for this *burst API* per-queue, but for 'dev->rx/tx_pkt_burst', if the
PMD can do burst selection per-queue internally, like NOT ALL VECTOR / SCALAR.

> > +};
> 
> How can we imagine standardizing the PMD optimizations?
> PMD developers are free to have as many burst implementation as they want.
> If we want to report info about what is used, it can be only a free string.
> 
> > +/**
> > + * Ethernet device RX/TX queue packet burst mode information structure.
> > + * Used to retrieve information about packet burst mode setting.
> > + */
> > +struct rte_eth_burst_mode {
> > +	uint64_t options;
> > +};
> 
> Why a struct for an integer?
> 

For expanding the information for mode, don't have to change the API parameters.

> > +/**
> > + * Retrieve information about the Rx packet burst mode.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param queue_id
> > + *   The Rx queue on the Ethernet device for which information
> > + *   will be retrieved.
> > + * @param mode
> > + *   A pointer to a structure of type *rte_eth_burst_mode* to be filled
> > + *   with the information of the packet burst mode.
> 
> No reference to the enum rte_eth_burst_mode_option or RTE_ETH_BURST_ prefix?
> 
> > + *
> > + * @return
> > + *   - 0: Success
> > + *   - -ENOTSUP: routine is not supported by the device PMD.
> > + *   - -EINVAL:  The port_id or the queue_id is out of range.
> > + */
> > +__rte_experimental
> > +int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> > +	struct rte_eth_burst_mode *mode);
> [...]
> > +/**
> > + * Retrieve name about burst mode option.
> > + *
> > + * @param mode
> > + *   The burst mode option of type *rte_eth_burst_mode_option*.
> > + *
> > + * @return
> > + *   - "": Not found
> > + *   - "xxx": name of the mode option.
> > + */
> > +__rte_experimental
> > +const char *
> > +rte_eth_burst_mode_option_name(uint64_t option);
> 
> rte_eth_burst_mode_name would be a better function name.
> But anyway, this function should not exist.
> The string should be freely returned by the PMD
> in the burst_mode_get functions.
> 

Design like : common part (options: vector, AVX2 etc) + device specific ?

struct rte_eth_burst_mode {
	uint64_t options;
	char dev_specific[128]; /* PMD has specific burst mode information */
};

Then rte_eth_burst_mode_option_name is good name to keep its small scope
on *options* stringification.