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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/25/2019 10:36 AM, Thomas Monjalon wrote:
> 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.

It has been discussed in the mail list and went through multiple discussions,
patch is out since the August, +1 to cc all maintainers I missed that part,
but when the patch is reviewed and there is no objection, why block the merge?

> 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?
> 
>> +
>> +	/**< 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.

It is used in some drivers as simple scalar path with most features are missing.

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

That was a comment by me, most PMDs doesn't support different vector path per
queue, for those PMDs, to make application life easy PMD can say if it supports
per queue or not with this flag.
If PMD supports only per port data path, application doesn't need to call this
per queue if it want.

> 
>> +};
> 
> 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.

The main target of the API is to define the vector path, not all optimizations,
so the number is limited.

When there is a standardized output it can be easier to be consumed by the
applications.

> 
>> +/**
>> + * 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?

Again by a request from me, to not need to break the API if we need to add more
thing in the future.

> 
>> +/**
>> + * 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.