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

[Wang, Haiyue]

> -----Original Message-----
> From: Jerin Jacob [mailto:jerinjacobk at gmail.com]
> Sent: Saturday, October 26, 2019 14:58
> To: Thomas Monjalon <thomas at monjalon.net>
> Cc: Yigit, Ferruh <ferruh.yigit at intel.com>; Wang, Haiyue <haiyue.wang 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 Sat, Oct 26, 2019 at 3:57 AM Thomas Monjalon <thomas at monjalon.net> wrote:
> >
> > 25/10/2019 18:02, Jerin Jacob:
> > > On Fri, Oct 25, 2019 at 9:15 PM Thomas Monjalon <thomas at monjalon.net> wrote:
> > > > 25/10/2019 16:08, Ferruh Yigit:
> > > > > On 10/25/2019 10:36 AM, Thomas Monjalon wrote:
> > > > > > 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.
> > > > >
> > > > > It has been discussed in the mail list and went through multiple discussions,
> > > > > patch is out since the August, +1 to cc all maintainers I missed that part,
> > > > > but when the patch is reviewed and there is no objection, why block the merge?
> > > >
> > > > I'm not saying blocking the merge.
> > > > My bad is that I missed the patch and I am asking for help with a notification
> > > > in this case. Same for Andrew I guess.
> > > > Note: it is merged in master and I am looking to improve this feature.
> > >
> > > > > >> +/**
> > > > > >> + * 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?
> > > > >
> > > > > Again by a request from me, to not need to break the API if we need to add more
> > > > > thing in the future.
> > > >
> > > > I would replace it with a string. This is the most flexible API.
> > >
> > > IMO, Probably, best of both worlds make a good option here,
> > > as Haiyue suggested if we have an additional dev_specific[1] in structure.
> > > and when a pass to the application, let common code make final string as
> > > (options flags to string + dev_specific)
> > >
> > > options flag can be zero if PMD does not have any generic flags nor
> > > interested in such a scheme.
> > > Generic flags will help at least to have some common code.
> > >
> > > [1]
> > > struct rte_eth_burst_mode {
> > >         uint64_t options;
> > >         char dev_specific[128]; /* PMD has specific burst mode information */
> > > };
> >
> > I really don't see how we can have generic flags.
> > The flags which are proposed are just matching
> > the functions implemented in Intel PMDs.
> > And this is a complicate solution.
> > Why not just returning a name for the selected Rx/Tx mode?
> 
> +1 only for the name
> 
> Let me clarify my earlier proposal:
> 
> 1) The public ethdev API should return only "string" i.e the flags
> SHOULD NOT be exposed as ethdev API
> i.e
> int rte_eth_tx_burst_mode_name(uint16_t port_id, uint16_t queue_id, char *name);
> 
> 2) The PMD interface  to the common code can be following
> 
>  struct eth_pmd_burst_mode {
>         uint64_t options;
>          char name[128]; /* PMD specific burst mode information */
> };
> 
> typedef int (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
>         uint16_t queue_id, struct eth_burst_mode *mode)
> 
> 3) The implementation of rte_eth_tx_burst_mode_name() shall do optons
> flag to string converion(again internal to common code implemetation)
> and concatenate with eth_pmd_burst_mode::name
> 
> This would help to reuse some of the flags to name conversion logic
> across all PMDs.
> And PMD are free to return  eth_pmd_burst_mode::options as zero in
> that case final
> string only be eth_pmd_burst_mode::name.
> 

In fact, 'rte_eth_burst_mode_option_name' for single option, not
for struct eth_pmd_burst_mode::option[s]. Need loop to display them.


static void
burst_mode_options_display(uint64_t options)
{
	int offset;

	while (options != 0) {
		offset = rte_bsf64(options);

		printf(" %s",
		       rte_eth_burst_mode_option_name(1ULL << offset));

		options &= ~(1ULL << offset);
	}
}

But can change the name show like:

	if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) == 0) {
		printf("\nBurst mode:");
		burst_mode_options_display(mode.options);

		if (mode.name[0] != '\0')
			printf("(%s)", mode.name); // use (...) to highlight device hardware specific.
	}

> I don't see any downside with this approach and it best of both worlds.
> 
> 
> 
> 
> 
> 
> >
> >