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

[Wang, Haiyue]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk at gmail.com>
> Sent: Tuesday, October 29, 2019 23:59
> To: Wang, Haiyue <haiyue.wang at intel.com>
> Cc: Thomas Monjalon <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>; dpdk-dev
> <dev at dpdk.org>; Ye, Xiaolong <xiaolong.ye at intel.com>; Kinsella, Ray <ray.kinsella at intel.com>;
> Iremonger, Bernard <bernard.iremonger at intel.com>; Sun, Chenmin <chenmin.sun at intel.com>; Andrew
> Rybchenko <arybchenko at solarflare.com>; Slava Ovsiienko <viacheslavo at mellanox.com>; Stephen Hemminger
> <stephen at networkplumber.org>; David Marchand <david.marchand at redhat.com>; Jerin Jacob
> <jerinj at marvell.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information
> 
> On Tue, Oct 29, 2019 at 9:12 PM Wang, Haiyue <haiyue.wang at intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk at gmail.com>
> > > Sent: Tuesday, October 29, 2019 22:09
> > > To: Wang, Haiyue <haiyue.wang at intel.com>
> > > Cc: Thomas Monjalon <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>; dpdk-dev
> > > <dev at dpdk.org>; Ye, Xiaolong <xiaolong.ye at intel.com>; Kinsella, Ray <ray.kinsella at intel.com>;
> > > Iremonger, Bernard <bernard.iremonger at intel.com>; Sun, Chenmin <chenmin.sun at intel.com>; Andrew
> > > Rybchenko <arybchenko at solarflare.com>; Slava Ovsiienko <viacheslavo at mellanox.com>; Stephen
> Hemminger
> > > <stephen at networkplumber.org>; David Marchand <david.marchand at redhat.com>; Jerin Jacob
> > > <jerinj at marvell.com>
> > > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information
> > >
> > > > > > >
> > > > > >
> > > > > > How about *_str_* style ?
> > > > >
> > > > > _name kind of implies it the string. may be _mode is good as it is short.
> > > > >
> > > > > > int
> > > > > > rte_eth_rx_burst_mode_str_get(uint16_t port_id, uint16_t queue_id,
> > > > > >                                       char *buf, int buflen)
> > > > >
> > > >
> > > > About the function, keep the same is better ? Then we need no whole
> > > > replace, just update the parameters, and the parameters indicated that
> > > > it is in string format.
> > >
> > > In this case, we need additional PMD op to get the buflen as
> > > the application will not know the buffer size in advance. It needs to come
> > > from the driver and common code.
> > >
> > >
> > > See below.
> > >
> > > >
> > > > > We don't need buflen as it is not known to the application. The
> > > > > typical pattern, we followed,
> > > > > in dpdk is, when function called buf as NULL then the function returns
> > > > > the expected size so that
> > > > > the application can alloc and get the buffer from ethdev layer on the
> > > > > next iteration.
> > > > >
> > > > >
> > > >
> > > > A little complicated and too heavy for using ? where is the example code ?
> > >
> > > See rte_eth_xstats_get_names() API as example for dynamic buffer allocation
> > > and similar use case in DPDK.
> >
> > In fact, this is no so complex as dynamic string handling. :)
> >
> >         /* Get count */
> >         cnt_xstats = rte_eth_xstats_get_names(port_id, NULL, 0);
> >         if (cnt_xstats  < 0) {
> >                 printf("Error: Cannot get count of xstats\n");
> >                 return;
> >         }
> >
> > xstats_names = malloc(sizeof(struct rte_eth_xstat_name) * cnt_xstats);
> >
> > struct rte_eth_xstat_name {
> >         char name[RTE_ETH_XSTATS_NAME_SIZE]; /**< The statistic name. */
> > };
> 
> 
> not really, it will be,[1]
> 
> count = rte_eth_rx_burst_mode_name(port_id, queue_id, NULL);
> buf = malloc(count);
> count = rte_eth_rx_burst_mode_name(port_id, queue_id, buf);
> 
> 

I mean 'cnt_xstats' can be calculated like:
	count = RTE_NB_STATS;
	count += nb_rxqs * RTE_NB_RXQ_STATS;
	count += nb_txqs * RTE_NB_TXQ_STATS;

But for string, what I know is that use 'snprintf(NULL, 0, ...)' to get
the real length, in other words:
	1). NULL, call 'snprintf' to loop to get the length
	2). Not none, call 'sprint' to loop to dump into buffer.
 So in the function, have to handle two kind of loops.

And application has to handle memory allocation, free etc. Why not
just use a structure ? It is clean.

> >
> >
> > Frankly speaking, after re-thinking, I prefer to keep current design.
> > 1) Use the structure to expose the *raw* data. Make the APIs work as
> > a SDK, DON'T image to format the string for user. User can call the
> > API to dump to file, print to console etc. Because he has the raw
> > data.
> 
> [1] Dont have such issue.
> 
> >
> > struct rte_eth_burst_mode {
> >         uint64_t options;
> > };
> >
> > The 'options' bit definition reflects the rx/tx source code structure roughly:
> >
> > "tree drivers/net/ | grep rxtx"
> > │   ├── virtio_rxtx_simple_sse.c
> >     └── vmxnet3_rxtx.c
> 
> Files don't represent the actual mode PMD is running. So listing the
> file example is not correct.
> 

I mean the function in it.

> 
> >
> > 2. add "char dev_specific[]" data as needed if a PMD wants to expose it,
> >    enhance the APIs step by step, and do it as needed.
> 
> This would change ABI for no reason and who allocates the memory for
> dev_specific?

I mean like 'char dev_specific[128]', not zero length data. Sorry for confusion.