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

[Thomas Monjalon <thomas@monjalon.net>]

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?

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

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

> +};

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?

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