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

[Thomas Monjalon <thomas@monjalon.net>]

26/10/2019 11:23, Wang, Haiyue:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 26/10/2019 06:40, Wang, Haiyue:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 25/10/2019 18:02, Jerin Jacob:
> > > > > On Fri, Oct 25, 2019 at 9:15 PM Thomas Monjalon <thomas@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?
> > >
> > > Intel PMDs use the *generic* methods like x86 SSE, AVX2, ARM NEON, PPC ALTIVEC,
> > > 'dev->data->scattered_rx' etc for the target : "DPDK is the Data Plane Development Kit
> > > that consists of libraries to accelerate packet processing workloads running on a wide
> > > variety of CPU architectures."
> > 
> > How RTE_ETH_BURST_SCATTERED and RTE_ETH_BURST_BULK_ALLOC are generic?
> > They just match some features of the Intel PMDs.
> > Why not exposing other optimizations of the Rx/Tx implementations?
> > You totally missed the point of generic burst mode description.
> > 
> > > If understand these new experimental APIs from above, then bit options is the best,
> > > and we didn't invent new words to describe them, just from the CPU & other *generic*
> > > technology. And the application can loop to check which kind of burst is running by
> > > just simple bit test.
> > >
> > > If PMDs missed these, they can update them in future roadmaps to enhance their PMDs,
> > > like MLX5 supports ARM NEON, x86 SSE.
> > 
> > I have no word!
> > You really think other PMDs should learn from Intel how to "enhance" their PMD?
> > You talk about mlx5, did you look at its code? Did you see the burst modes
> > depending on which specific hardware path is used (MPRQ, EMPW, inline)?
> > Or depending on which offloads are handled?
> > 
> > Again, the instruction set used by the function is a small part
> > of the burst mode optimization.
> > 
> > So you did not reply to my question:
> > Why not just returning a name for the selected Rx/Tx mode?
> 
> In fact, RFC v1/v2 returns the *name*, but the *name* is hard for
> application to do further processing, strcmp, strstr ? Not so nice
> for C code, and it is not so standard, So switch it to bit definition.

Again, please answer my question: why do you need it?
I think it is just informative, that's why a string should be enough.
I am clearly against the bitmap because it is way too much restrictive.
I disagree that knowing it is using AVX2 or AVX512 is so interesting.
What you would like to know is whether it is processing packets 4 by 4,
for instance, or to know which offload is supported, or what hardware trick
is used in the datapath design.
There are so many options in a datapath design that it cannot be
represented with a bitmap. And it makes no sense to have some design
criterias more important than others.
I Cc an Intel architect (Edwin) who could explain you how much
a datapath design is more complicate than just using AVX instructions.

> If the API is not bad, but the data is not so good, then accept below ? ;-)

No, the API is bad.