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

[Jerin Jacob]

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


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


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