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

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

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, November 6, 2019 08:34
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; jerinjacobk@gmail.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com; viacheslavo@mellanox.com; damarion@cisco.com; Ye, Xiaolong
> <xiaolong.ye@intel.com>; Sun, Chenmin <chenmin.sun@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> Liu, Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: enhance the API for getting burst mode information
> 
> 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.
> 

Will take care next time.

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

The first is for all flags, but only one now, so looks like for the same item.
The second is just for RTE_ETH_BURST_FLAG_PER_QUEUE flag.

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

1. 'a constant string' is hard for PMD expanding if it wants to make the string
   dynamic according to the setting, like: http://patchwork.dpdk.org/patch/62352/
   (although based on bit options design).

2. And for dynamic string, if it is *return type*, then the PMD needs to
   handle the memory allocation, and the application frees it. And 'uint64_t flags'
   is output parameter, so it should be like 'uint64_t *flags', but this needs the
   application to declare it or not, and needs PMDs to check whether it is passed
   or not, then set it.

   So for making things easy, the 'struct rte_eth_burst_mode' may be nice, then the
   application just declares one line : 'struct rte_eth_burst_mode mode', then all
   things are filled by PMD in one place.


>