Re: [dpdk-dev] [PATCH v2] ethdev: enhance the API for getting burst mode information

[Thomas Monjalon <thomas@monjalon.net>]

04/11/2019 11:39, Haiyue Wang:
> Change the type of burst mode information from bit field to free string
> data, so that each PMD can describe the Rx/Tx busrt functions flexibly.
> 
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
> 
> v2: - Drop the bit field for burst mode information handling.

Please use --in-reply-to, so the versions of a patch can be in the same thread.

> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
>  /**
> - * Burst mode types, values can be ORed to define the burst mode of a driver.
> + * Generic Burst mode flag definition, values can be ORed.
> + */
> +#define RTE_ETH_BURST_FLAG_PER_QUEUE     (1ULL << 0)
> +/**< If the queues have different burst mode description, this bit will be set
> + * by PMD, then the application can iterate to retrieve burst description for
> + * all other queues.
>   */

I am not sure you can have a doxygen comment before and after the same item.

> -enum rte_eth_burst_mode_option {
> -	RTE_ETH_BURST_SCALAR = (1 << 0),
> -	RTE_ETH_BURST_VECTOR = (1 << 1),
> -
> -	/**< 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),
> -
> -	RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> -};

Thank you

>  /**
>   * 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;
> +	uint64_t flags; /**< The ORed values of RTE_ETH_BURST_FLAG_xxx */
> +
> +#define RTE_ETH_BURST_MODE_INFO_SIZE 1024 /**< Maximum size for information */
> +	char info[RTE_ETH_BURST_MODE_INFO_SIZE]; /**< burst mode information */
>  };

I think the API can be simpler by passing the flags as function parameter.

In my understanding the burst mode name is fixed per Rx/Tx function,
so it can be a constant string referenced with a simple char*.

This is the current API:

int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
    struct rte_eth_burst_mode *mode);

I wonder what do you think of such API? (just a proposal for comments):

char *rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id, uint64_t flags);

Or is there some cases where you want to build the string with snprintf?
(I cannot think about a case, given it should mapped to a C-function)