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

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

> -----Original Message-----
> From: Wang, Haiyue
> Sent: Sunday, November 3, 2019 10: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://patchwork.dpdk.org/patch/57644/
> 
> 	+#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
> 
> 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):
>        https://patchwork.dpdk.org/patch/61196/
> 
> 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".
> 
> Intel PMD can just assign "options", then finally, 'alternate_options' will be "Vector SSE".
> 
> How about the design idea ? Again, this 'options' is not to do standardization, just
> want to reduce the duplicated name string things.
> 

Also, one more thing about 'RTE_ETH_BURST_PER_QUEUE', I added new comment to make it
be more understandable, how about this ?

/**< If the Rx/Tx queues have different burst mode description, then PMD return
 * this bit, so the application can enumerate all queues burst mode description
 * as needed.
 */
#define RTE_ETH_BURST_PER_QUEUE     (1ULL << 63)

so that, application can know more about PMD's burst information:

  rte_eth_rx_burst_mode_get(port_id, 0, &mode); /* Use queue Id #0 to get burst mode firstly*/

  if (mode.options & RTE_ETH_BURST_PER_QUEUE)
	loop other queues.

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