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

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

Hi Slava,

Thanks for your reply. I've no more description to talk about the code
design. And I agree with your thinking, all roads lead to Rome, please
don't hesitate to submit a patch to clean up things. Your rich experience
is very appreciated to speed up the design, thanks! ;-)

BR,
Haiyue

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@mellanox.com>
> Sent: Monday, November 4, 2019 03:31
> To: Wang, Haiyue <haiyue.wang@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> jerinjacobk@gmail.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> Sun, Chenmin <chenmin.sun@intel.com>; Damjan Marion (damarion) <damarion@cisco.com>
> Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information
> 
> Hi, Haiyue
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Sunday, November 3, 2019 13:38
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>; Liu, Yu Y
> > <yu.y.liu@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>; Sun,
> > Chenmin <chenmin.sun@intel.com>; Damjan Marion (damarion)
> > <damarion@cisco.com>
> > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
> > information
> >
> > Hi Slava,
> >
> > > -----Original Message-----
> > > From: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Sent: Sunday, November 3, 2019 16:59
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; Liu, Yu Y
> > <yu.y.liu@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > <ferruh.yigit@intel.com>;
> > > jerinjacobk@gmail.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Kinsella,
> > Ray <ray.kinsella@intel.com>;
> > > Sun, Chenmin <chenmin.sun@intel.com>; Damjan Marion (damarion)
> > <damarion@cisco.com>
> > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst
> > mode information
> > >
> > > > -----Original Message-----
> > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > Sent: Sunday, November 3, 2019 4:34
> > > > To: Slava Ovsiienko <viacheslavo@mellanox.com>; Liu, Yu Y
> > > > <yu.y.liu@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > > > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > > > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>; Sun,
> > > > Chenmin <chenmin.sun@intel.com>; Damjan Marion (damarion)
> > > > <damarion@cisco.com>
> > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst
> > mode
> > > > information
> > > >
> > > > Hi Thomas, Slava,
> > > >
> > > > Please see the inline reply in one place.
> > > >
> > > > > -----Original Message-----
> > > > > From: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > > > Sent: Saturday, November 2, 2019 16:39
> > > > > To: Liu, Yu Y <yu.y.liu@intel.com>; Wang, Haiyue
> > > > > <haiyue.wang@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>
> > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > > > > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > > > > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>; Sun,
> > > > > Chenmin <chenmin.sun@intel.com>; Damjan Marion (damarion)
> > > > > <damarion@cisco.com>
> > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst
> > > > > mode information
> > > > >
> > > > > Hi
> > > > > > -----Original Message-----
> > > > > > From: Liu, Yu Y <yu.y.liu@intel.com>
> > > > > > Sent: Saturday, November 2, 2019 8:56
> > > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; Thomas Monjalon
> > > > > > <thomas@monjalon.net>
> > > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > > > > > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > > > > > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> > > > > > Sun, Chenmin <chenmin.sun@intel.com>; Slava Ovsiienko
> > > > > > <viacheslavo@mellanox.com>; Damjan Marion (damarion)
> > > > > > <damarion@cisco.com>; Liu, Yu Y <yu.y.liu@intel.com>
> > > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting
> > > > > > burst mode information
> > > > > >
> > > > > > Add Damjan from FD.io for awareness...
> > > > > >
> > > > > > Hi Thomas,
> > > > > >
> > > > > > Long time no see. Sorry I use outlook which is not friendly to
> > > > > > community email.
> > > > > >
> > > > > > >Anyway I will propose to replace this API in the next release.
> > > > > > Will your plan be affected by API/ABI stable plan?
> > > > > > BTW, if you propose new change in next release, it will make DPDK
> > > > > > consumer(FD.io) to change again.
> > > > > > So even if it is not affected to the API/ABI stable plan, do we
> > > > > > still have time to get a solution for everyone in DPDK 19.11 with
> > > > > > your contribution/acceleration?
> > > > > >
> > > > > > > I suspect a real hidden issue in Intel CPUs that you try to mitigate.
> > > > > > Please be rest assured it is not the case.
> > > > > > This request is just from one FD.io project internal bug " tx/rx
> > > > > > burst function is shown as nil" reported by Chenmin.
> > > > >
> > > > > Why just the presenting string with function name (possible with suffix)
> > is
> > > > not enough?
> > > > > I would like to see this API  (strings approach) in mlx5 either,
> > > > > dropping the entire feature does not look nice, as for me.
> > > > >
> > > > > We could consider some requirements for the name suffices to
> > > > > distinguish whether function uses vector instructions and which ones if
> > any.
> > > > >
> > > > > > My understanding is DPDK behavior was taken as bug for someone in
> > > > > > FD.io project and potentially will mislead other DPDK consumer.
> > > > >
> > > > > Why does FD.io code want to know which vector extension is used by
> > burst
> > > > routines?
> > > > > Is it going to share/preserve some resources (registers, etc.)? Is it
> > robust ?
> > > > > Burst routines might not know whether vector extensions is used (they
> > > > > might call libraries, even rte_memcpy() can use vectors in implicit
> > fashion).
> > > > >
> > > >
> > > > 1.
> > > >
> > > > The original issue description is:
> > > > "VPP uses dladdr() to translate a function address to name, however,
> > some
> > > > tx/rx functions  in DPDK are invisible for dladdr(), which is because they
> > are
> > > > defined as static."
> > > >
> > > > 2.
> > > >
> > > > So the RFC design is: one function, one description, like:
> > > >
> > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > > >
> > work.dpdk.org%2Fpatch%2F57644%2F&amp;data=02%7C01%7Cviacheslavo
> > > >
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > > >
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&amp;sdat
> > > >
> > a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&amp;reserved=
> > > > 0
> > > >
> > > > 	+#ifdef RTE_ARCH_X86
> > > > +	else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec_avx2)
> > > > +		len = snprintf(buf, sz, "AVX2 Vector Scattered Rx");
> > > > +	else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec)
> > > > +		len = snprintf(buf, sz, "Vector Scattered Rx");
> > > > +	else if (dev->rx_pkt_burst == ice_recv_pkts_vec_avx2)
> > > > +		len = snprintf(buf, sz, "AVX2 Vector Rx");
> > > > +	else if (dev->rx_pkt_burst == ice_recv_pkts_vec)
> > > > +		len = snprintf(buf, sz, "Vector Rx"); #endif
> > > The application gets the routine names with dladdr(). OK, it happens.
> > > It is not clear for me why instead of direct replacement/extension of dladdr
> > > functionality some new names were introduced and then converted to
> > flags.
> > >
> >
> > Sorry, can you explain more ? Who 'direct replacement/extension of dladdr'
> > ?
> > VPP, or DPDK ?
> >
> > > > 3. Since the main issue is as Damjan replied in another thread:
> > > >    "people are reporting lower performance caused by DPDK deciding for
> > > > variety
> > > >     of reasons to switch from vector PMD to scalar one."
> > > >
> > > >    And Ferruh replied also:
> > > >     "As I understand this is to let applications to give informed decision
> > based
> > > >      on what vectorization is used in the driver, currently this is not known
> > by
> > > >      the application.
> > > >
> > > >      And as previously replied, the main target of the API is to define the
> > vector
> > > >      path, not all optimizations, so the number is limited."
> > > >
> > > >      So we enhanced it with bit, example detail is (Yes, we defined a lit
> > more,
> > > >      so we removed it in this patch):
> > >
> > > There are might be a lot of various burst functions,  vectorized or not,
> > > with various sets of supported offloads. Yes, identifying the engaged burst
> > > routine is meaningful, but it is not clear for me, why the vectorizing type
> > > should have dedicated means (flags) to identify ?
> > >
> >
> > The new 'rte_eth_rx/tx_burst_mode_get' works like logging, but in fact, the
> > log
> > message is something special, like "Vector Neon/AltiVec/SSE/AVX2" and the
> > device
> > specific offloads as you said.
> >
> > This kind of string "Vector Neon/AltiVec/SSE/AVX2" can be common, we not
> > treat it
> > as 'flag', it is a normal bit like macro definition, and it will be translated into
> > string later. And we want to make PMD's string format life to be easy, don't
> > need
> > to call 'snprintf/sprintf' with the copied string format.
> >
> > So now, the log message format is: device specific (if have) + "Vector ..." (if
> > have, this is not MUST, if the PMD doesn't use vector, but at least, this is not
> > hardware specific, it is some common from arch:
> > lib/librte_eal/common/arch/arm,ppc_64,x86).
> >
> > Further, as a SDK, the API exposes these common bit data for application
> > easily access
> > if it may need, DON'T NEED TO BREAK THE ABI/API. Compared to
> > 'strstr/strcasestr',
> > 'mode->options & RTE_ETH_BURST_VECTOR' is more friendly ?
> 
> Yes, it is more friendly for app. But there is quite another question: do we really
> need these flags for logging purposes ? There are no explicitly expressed
> requirements from applications for these flags.
> 
> > > >
> > > >
> > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > > >
> > work.dpdk.org%2Fpatch%2F61196%2F&amp;data=02%7C01%7Cviacheslavo
> > > >
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > > >
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&amp;sdat
> > > >
> > a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&amp;reserved
> > > > =0
> > > >
> > > > 4. And thanks Jerin's suggestion, I think his word can be more accurate:
> > "This
> > > > would
> > > >    help to reuse some of the flags to name conversion logic across all
> > PMDs"
> > > > for the
> > > >    reason we try to use bit to reduce some string format effort, it will be
> > > > handled
> > > >    by the API internally "burst_mode_options_append(struct
> > > > rte_eth_burst_mode *mode)".
> > > >    Now the new API will return the string finally:
> > > >
> > > > #define RTE_ETH_BURST_MODE_ALT_OPT_SIZE 1024 struct
> > > > rte_eth_burst_mode {
> > > > 	uint64_t options;
> > > >
> > > > 	/**< Each PMD can fill specific burst mode information into this, and
> > > > 	 * ethdev APIs will append the 'options' string format at its end.
> > > > 	 */
> > > > 	char alternate_options[RTE_ETH_BURST_MODE_ALT_OPT_SIZE];
> > > > };
> > > >
> > > > So MLX PMD can add 'full_empw', 'mtsc_empw' etc into
> > 'alternate_options'
> > > > firstly, assign 'RTE_ETH_BURST_VECTOR | RTE_ETH_BURST_SSE' to
> > 'options'
> > > > as needed, then finally, 'alternate_options' will be "full_empw, Vector
> > SSE".
> > >
> > > For mlx5 tx_burst these flags have no meaning. All information regarding
> > routine
> > > is encoded within the name, mtsc stands for:
> > > m - multisegment
> > > t - TSO
> > > s - software tunnel parser
> > > c - check sum
> > >
> > > There are no two versions of mtsc_empw - "mtsc_empw, Vector SSE",
> > "mtsc_empw, Vector Neon".
> > > If we developed vectorized version, I would prefer "mtsc_empw_sse".
> > >
> > > To summarize:
> > > - application uses routine names, gets with dladdr(). Nice.
> > > - compatible API providing names of internal routines is proposed. Nice.
> > > - users now are able to identify the engaged burst routine. Nice.
> > > - proposed API is extended, some vector related flags were added.
> > Hmmm.... Questionable.
> > >   Why vector related only? Why do we change the string format? (name ->
> > name, options)
> >
> > Again, vector is not "only", it is just 'main' characteristic "tree ./drivers/net/ |
> > grep rxtx_".
> > we can design the Rx/Tx burst function mainly by vector type, it is
> > straightforward.
> 
> OK, we have some flag field proposed. Saying "why vector only" I meant
> that vectorizing is just one of optimizing technics. I do not see it as
> "main tree characteristics", sorry.
> 
> What if some other vendor would like to add its own flags? For example,
> mlx5 could add at least 8 optimizing flags for tx_burst and 4 flags for rx_burst
> (besides vector related ones). Why not? Why do we decide to add vector flags only?
> Other vendors might come into play and add its own flags describing the burst routine
> features and optimizations. And then say - "hey, these parameters define our
> internal rxtx tree. It is very critical for performance, user must know about ones".
> 
> >
> > Why 'name -> name' ?
> Sorry for the MS Outlook (and I'm on the way to Mutt now),
> it is not community friendly.
> Correct sentence: "name" to "name, options"
> 
> > 1.) [v4,4/4] app/testpmd: show the Rx/Tx burst mode description
> > 	https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%
> > 2Fpatchwork.dpdk.org%2Fpatch%2F61198%2F&amp;data=02%7C01%7Cviac
> > heslavo%40mellanox.com%7C0ba4e73594684f944b7608d760525c97%7Ca6
> > 52971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083779173049231&a
> > mp;sdata=2NCHJJpsAYsb07FTEmvr4EiIVudm7ZCVhemh2g1MBG0%3D&amp;r
> > eserved=0
> > This is handled by the application itself, not so friendly, many lines of code to
> > show.
> Yes, it does not look nice, as for me. String should be simple and just provided by PMD,
> without any extra flags/options, IMHO.
> >
> >
> > 2). [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
> > information
> > 	if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) == 0)
> > 		printf("\nBurst mode: %s", mode.alternate_options);
> >
> > 	This design may meet your question above if I understand correctly.
> > 	"It is not clear for me why instead of direct replacement/extension
> > of dladdr
> >        functionality some new names were introduced and then converted to
> > flags".
> >
> >
> > Last, again, we define the bits 'RTE_ETH_BURST_XXX' for making the log
> > message
> > generation process easily if you agree vector type is common, the vector can
> > be
> Simple string returned by PMD eliminates "message process generation" at all.
> No flags/options - no generation needed at all. In my opinion, PMD just should
> return strings like "rx_burst_vector_sse", "rx_burst_vector_neon", etc.
> 
> > used to improve the performance. And if new burst design can be used for
> > most
> > PMDs, use it as bit, the API helps to translate it to string. And the application
> > can use the bit to do other kind of information display.
> >
> > We define it a little more than 'simple string' for just making life easy. In fact,
> > the patch comes from "simple string", RFC v1, v2, v3, PATCH v1 v2 v3 v4.
> 
> Applications live OK with dladdr(). The returned name is used for logging.
> There is NO explicit requirements from application to provide some extra options,
> besides the name (or, at least, these ones are not visible for me).
> 
> Sorry, it is not clear for me, how by introducing extra flags and the extra
> "easy message generation process" we make life easier. If PMD just provides
> the simple string "rx_burst_vector_sse", everyone seeing this string in the log
> understands what and how the named rx_burst is doing,  right? Do you think
> the message like "rx_burst, Vector SSE" looks better? OK, your PMD
> is free to return it.
> 
> Honestly, I do not mind against flags strictly, but I do not see any new meanings
> introduced by flags. It requires extra code, it might introduce some ambiguity,
> it would be ridiculous to see something like that:
> "rx_burst_vector_neon, Vector_AVX"
> And, the last,  the flag field is a potential scarce resource for vendors.
> 
> With best regards, Slava
> 
> > >
> > >
> > > > Intel PMD can just assign "options", then finally, 'alternate_options' will
> > be
> > > > "Vector SSE".
> > >
> > > As I see from initial patch, Intel PMD has dedicated routines with unique
> > names for
> > > each type of vectorization. Is there some burst routine  with single name
> > which could
> > > operate with different vectorization types, depending on configuration?
> > >
> > > With best regards, Slava
> > >
> > > >
> > > > How about the design idea ? Again, this 'options' is not to do
> > standardization,
> > > > just want to reduce the duplicated name string things.
> > > >
> > > > > With best regards, Slava
> > > > >
> > > > > > Haiyue is working with Chenmin to address the issue and with your
> > > > > > support it will be even better.
> > > > > >
> > > > > > Your support will be highly appreciated!
> > > > > >
> > > > > > Thanks & Regards,
> > > > > > Yu Liu
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Wang, Haiyue
> > > > > > Sent: Saturday, November 2, 2019 1:30 PM
> > > > > > To: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > > > > > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > > > > > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> > > > > > Sun, Chenmin <chenmin.sun@intel.com>; Slava Ovsiienko
> > > > > > <viacheslavo@mellanox.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for
> > > > > > getting burst mode information
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > Sent: Saturday, November 2, 2019 06:46
> > > > > > > To: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh
> > > > > > > <ferruh.yigit@intel.com>; jerinjacobk@gmail.com; Ye, Xiaolong
> > > > > > > <xiaolong.ye@intel.com>; Kinsella, Ray <ray.kinsella@intel.com>;
> > > > > > > Sun, Chenmin <chenmin.sun@intel.com>; Slava Ovsiienko
> > > > > > > <viacheslavo@mellanox.com>
> > > > > > > Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting
> > > > > > > burst mode information
> > > > > > >
> > > > > > > Thank you for trying to address comments done late.
> > > > > > >
> > > > > > > 31/10/2019 18:11, Haiyue Wang:
> > > > > > > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > > > > > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > > > >
> > > > > >
> > > > > > > > +#define RTE_ETH_BURST_ALTIVEC       (1ULL << 2)
> > > > > > > > +#define RTE_ETH_BURST_NEON          (1ULL << 3)
> > > > > > > > +#define RTE_ETH_BURST_SSE           (1ULL << 4)
> > > > > > > > +#define RTE_ETH_BURST_AVX2          (1ULL << 5)
> > > > > > > > +#define RTE_ETH_BURST_AVX512        (1ULL << 6)
> > > > > > >
> > > > > > > Of course, I still believe that giving a special treatment to
> > > > > > > vector instructions is wrong.
> > > > > > > You did not justify why it needs to be defined in bits instead of
> > > > > > > string. I am not asking again because anyway you don't really
> > > > > > > reply. I think you are executing an order you received and I don't
> > > > > > > want to blame you more.
> > > > > > > I suspect a real hidden issue in Intel CPUs that you try to mitigate.
> > > > > > > No need to reply to this comment.
> > > > > > > Anyway I will propose to replace this API in the next release.
> > > > > >
> > > > > > Never mind, if this design is truly ugly, drop it all now. I also
> > > > > > prefer to do the best, that's why open source is amazing, thanks!
> > > > > > ;-)