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

[Thomas Monjalon <thomas@monjalon.net>]

25/10/2019 16:08, Ferruh Yigit:
> 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?

I'm not saying blocking the merge.
My bad is that I missed the patch and I am asking for help with a notification
in this case. Same for Andrew I guess.
Note: it is merged in master and I am looking to improve this feature.

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

This is a weak definition...

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

It would require some doxygen explanations.

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

Why application needs this info to be standardized?
I think it should not. I am really against such standardization.
I think it would hurt more than help because there are a lot more
major infos than just knowing the ISA it is using.

I think such info should be used only for debugging or have a clue
about the expected performance.
You cannot standardize the expectations of a specific implementation.

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

I would replace it with a string. This is the most flexible API.

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