[v1,3/3] ethdev: enhance the API for getting burst mode information
Checks
Commit Message
1. Change the rte_eth_rx/tx_burst_mode_get ethdev APIs to convert the
'uint64_t options' to string and append it to 'alternate_options', so
that application access the string directly.
2. Change the 'enum rte_eth_burst_mode_option' to macro definition,
since ISO C restricts enumerator values to range of ‘int’, but it
needs 64bit.
3. Remove the 'rte_eth_burst_mode_option_name()' API.
Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
app/test-pmd/config.c | 27 ++-------
doc/guides/nics/features.rst | 3 +-
doc/guides/rel_notes/release_19_11.rst | 2 -
lib/librte_ethdev/rte_ethdev.c | 76 +++++++++++++++++-------
lib/librte_ethdev/rte_ethdev.h | 47 +++++----------
lib/librte_ethdev/rte_ethdev_version.map | 1 -
6 files changed, 76 insertions(+), 80 deletions(-)
Comments
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
> -enum rte_eth_burst_mode_option {
> - RTE_ETH_BURST_SCALAR = (1 << 0),
> - RTE_ETH_BURST_VECTOR = (1 << 1),
> -
> - /**< bits[15:2] are reserved for each vector type */
> - RTE_ETH_BURST_ALTIVEC = (1 << 2),
> - RTE_ETH_BURST_NEON = (1 << 3),
> - RTE_ETH_BURST_SSE = (1 << 4),
> - RTE_ETH_BURST_AVX2 = (1 << 5),
> - RTE_ETH_BURST_AVX512 = (1 << 6),
> -
> - RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> - RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> - RTE_ETH_BURST_SIMPLE = (1 << 18),
> -
> - RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> -};
> +#define RTE_ETH_BURST_SCALAR (1ULL << 0)
> +#define RTE_ETH_BURST_VECTOR (1ULL << 1)
Only one bit is needed: if it is not vector, it is scalar.
I think you can remove the scalar bit.
> +/**< bits[15:2] are reserved for each vector type */
Why 15:2 instead of 2:15?
> +#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.
> +/**< Support per queue burst */
> +#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
This comment is meaningless.
If this bit has a usage, please explain how to use this bit
in the comment.
> -----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! ;-)
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.
My understanding is DPDK behavior was taken as bug for someone in FD.io project and potentially will mislead other DPDK consumer.
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! ;-)
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).
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! ;-)
02/11/2019 07:55, Liu, Yu Y:
> 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?
The API is experimental, so it can be changed later.
> BTW, if you propose new change in next release, it will make DPDK consumer(FD.io) to change again.
Yes I agree it is not nice.
> 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?
Yes we have time.
But you insist on an API without any good justification.
> > 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.
> My understanding is DPDK behavior was taken as bug for someone in FD.io project and potentially will mislead other DPDK consumer.
> Haiyue is working with Chenmin to address the issue and with your support it will be even better.
>
> Your support will be highly appreciated!
I already said what I consider to be good: a simple string.
Of course I may be wrong, that's why I asked questions.
But half of the questions are just ignored.
If you want to progress, please reply to the questions asked by Slava in this thread.
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wang, Haiyue
> > From: Thomas Monjalon <thomas@monjalon.net>
> >
> > 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! ;-)
> On 2 Nov 2019, at 09:38, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>
> 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).
This is jut debug CLI print, it was added by me as a result of frustration of dealing constantly with
operational issues where people are reporting lower performance caused by DPDK deciding
for variety of reasons to switch from vector PMD to scalar one.
>
>> 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! ;-)
>
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.
> 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! ;-)
> -----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! ;-)
Hi Thomas,
Reply the missed:
> -----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
> > -enum rte_eth_burst_mode_option {
> > - RTE_ETH_BURST_SCALAR = (1 << 0),
> > - RTE_ETH_BURST_VECTOR = (1 << 1),
> > -
> > - /**< bits[15:2] are reserved for each vector type */
> > - RTE_ETH_BURST_ALTIVEC = (1 << 2),
> > - RTE_ETH_BURST_NEON = (1 << 3),
> > - RTE_ETH_BURST_SSE = (1 << 4),
> > - RTE_ETH_BURST_AVX2 = (1 << 5),
> > - RTE_ETH_BURST_AVX512 = (1 << 6),
> > -
> > - RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> > - RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> > - RTE_ETH_BURST_SIMPLE = (1 << 18),
> > -
> > - RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> > -};
> > +#define RTE_ETH_BURST_SCALAR (1ULL << 0)
> > +#define RTE_ETH_BURST_VECTOR (1ULL << 1)
>
> Only one bit is needed: if it is not vector, it is scalar.
> I think you can remove the scalar bit.
>
1. 'options' can be all zero, so we can't assume it is 'scalar', so let's the PMD
set it explicitly.
2. As replied before, it seems a little redundant, but a bit-opaque definition
will make string format easily, like a direct 'for' loop:
----------------------------------
static const struct {
uint64_t option;
const char *name;
} rte_burst_option_names[] = {
{ RTE_ETH_BURST_SCALAR, "Scalar" },
{ RTE_ETH_BURST_VECTOR, "Vector" },
{ RTE_ETH_BURST_ALTIVEC, "AltiVec" },
{ RTE_ETH_BURST_NEON, "Neon" },
----------------------------------
And in fact, only one vector type will be returned, but if use the bit save definition
like register, there will be something like 'RTE_ETH_BURST_VERTOR_TYPE_MASK', this will
make PMD and string format harder ...
> > +/**< bits[15:2] are reserved for each vector type */
>
> Why 15:2 instead of 2:15?
>
Changed as:
/**< bits 2 ~ 15 are reserved for each vector type */
> > +#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.
>
> > +/**< Support per queue burst */
> > +#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
>
> This comment is meaningless.
> If this bit has a usage, please explain how to use this bit
> in the comment.
>
Changes as:
/**< If the Rx/Tx queues have different burst mode description, the bit will be
* set by PMD, then the application can enumerate to retrive burst description
* for all PMD queues.
*/
> -----Original Message-----
> From: Damjan Marion (damarion) <damarion@cisco.com>
> Sent: Saturday, November 2, 2019 21:21
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Liu, Yu Y <yu.y.liu@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org;
> arybchenko@solarflare.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> jerinjacobk@gmail.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Ray Kinsella
> <ray.kinsella@intel.com>; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
> information
>
>
>
> > On 2 Nov 2019, at 09:38, Slava Ovsiienko <viacheslavo@mellanox.com>
> wrote:
> >
> > 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).
>
> This is jut debug CLI print, it was added by me as a result of frustration of
> dealing constantly with operational issues where people are reporting lower
> performance caused by DPDK deciding for variety of reasons to switch from
> vector PMD to scalar one.
And it seems there is no need for flags, as for me.
I think the simple string would be quite enough to identify the datapath routine.
Also, we have exact the same issue with mlx5 PMD, so the API (in simple
string version) is desirable (+1 from me).
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! ;-)
> >
> -----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&data=02%7C01%7Cviacheslavo
> %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&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.
> 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 ?
>
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.dpdk.org%2Fpatch%2F61196%2F&data=02%7C01%7Cviacheslavo
> %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&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)
> 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!
> > > ;-)
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&data=02%7C01%7Cviacheslavo
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&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 ?
> >
> > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > work.dpdk.org%2Fpatch%2F61196%2F&data=02%7C01%7Cviacheslavo
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&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.
Why 'name -> name' ?
1.) [v4,4/4] app/testpmd: show the Rx/Tx burst mode description
https://patchwork.dpdk.org/patch/61198/
This is handled by the application itself, not so friendly, many lines of code to show.
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
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.
>
>
> > 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!
> > > > ;-)
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&data=02%7C01%7Cviacheslavo
> > >
> %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > >
> d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > >
> a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&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&data=02%7C01%7Cviacheslavo
> > >
> %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > >
> d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > >
> a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&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&data=02%7C01%7Cviac
> heslavo%40mellanox.com%7C0ba4e73594684f944b7608d760525c97%7Ca6
> 52971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083779173049231&a
> mp;sdata=2NCHJJpsAYsb07FTEmvr4EiIVudm7ZCVhemh2g1MBG0%3D&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!
> > > > > ;-)
On 02/11/2019 19:21, Damjan Marion (damarion) wrote:
>
>
>> On 2 Nov 2019, at 09:38, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>>
>> 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).
>
> This is jut debug CLI print, it was added by me as a result of frustration of dealing constantly with
> operational issues where people are reporting lower performance caused by DPDK deciding
> for variety of reasons to switch from vector PMD to scalar one.
+1
I stated much the same elsewhere in this thread.
If your using DPDK in place of alternative, you are doing so for performance reasons.
If your performance drops in half becauses of configuration, you sure as hell want to understand why!
Today consuming applications get wildly different performance from DPDK based on configuration.
For me that is a huge usability issue and we can't leave the end user scratching their heads
wonder what is going on.
You program to our API and well take care of that doesn't cut it.
User in the field deal with real performance degradation and need information to solve problems.
This empowers them, and they shouldn't need to break out gdb to make it happen.
>
>>
>>> 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! ;-)
>>
>
>
03/11/2019 04:52, Wang, Haiyue:
> Hi Thomas,
>
> Reply the missed:
>
> From: Thomas Monjalon <thomas@monjalon.net>
> >
> > 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
> > > -enum rte_eth_burst_mode_option {
> > > - RTE_ETH_BURST_SCALAR = (1 << 0),
> > > - RTE_ETH_BURST_VECTOR = (1 << 1),
> > > -
> > > - /**< bits[15:2] are reserved for each vector type */
> > > - RTE_ETH_BURST_ALTIVEC = (1 << 2),
> > > - RTE_ETH_BURST_NEON = (1 << 3),
> > > - RTE_ETH_BURST_SSE = (1 << 4),
> > > - RTE_ETH_BURST_AVX2 = (1 << 5),
> > > - RTE_ETH_BURST_AVX512 = (1 << 6),
> > > -
> > > - RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> > > - RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> > > - RTE_ETH_BURST_SIMPLE = (1 << 18),
> > > -
> > > - RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> > > -};
> > > +#define RTE_ETH_BURST_SCALAR (1ULL << 0)
> > > +#define RTE_ETH_BURST_VECTOR (1ULL << 1)
> >
> > Only one bit is needed: if it is not vector, it is scalar.
> > I think you can remove the scalar bit.
> >
>
> 1. 'options' can be all zero, so we can't assume it is 'scalar', so let's the PMD
> set it explicitly.
So what means zero? neither scalar nor vector?
> 2. As replied before, it seems a little redundant, but a bit-opaque definition
> will make string format easily, like a direct 'for' loop:
> ----------------------------------
> static const struct {
> uint64_t option;
> const char *name;
> } rte_burst_option_names[] = {
> { RTE_ETH_BURST_SCALAR, "Scalar" },
> { RTE_ETH_BURST_VECTOR, "Vector" },
>
> { RTE_ETH_BURST_ALTIVEC, "AltiVec" },
> { RTE_ETH_BURST_NEON, "Neon" },
> ----------------------------------
>
> And in fact, only one vector type will be returned, but if use the bit save definition
> like register, there will be something like 'RTE_ETH_BURST_VERTOR_TYPE_MASK', this will
> make PMD and string format harder ...
I don't understand what is hard in parsing a bit.
But I'm probably not skilled enough.
Also I would be interested to understand what means scalar exactly here?
It is processing packets one by one?
And vector, does it mean four packets at once?
How can I differentiate a function which is processing packets two packets at once?
> > > +/**< bits[15:2] are reserved for each vector type */
> >
> > Why 15:2 instead of 2:15?
>
> Changed as:
> /**< bits 2 ~ 15 are reserved for each vector type */
>
> > > +#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.
> >
> > > +/**< Support per queue burst */
> > > +#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
> >
> > This comment is meaningless.
> > If this bit has a usage, please explain how to use this bit
> > in the comment.
> >
>
> Changes as:
>
> /**< If the Rx/Tx queues have different burst mode description, the bit will be
> * set by PMD, then the application can enumerate to retrive burst description
> * for all PMD queues.
> */
OK this is a better description, thanks.
Indeed this parameter looks to be a good idea.
If we would drop this bit-field, the per-queue bit could be
a parameter of the function.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, November 4, 2019 06:21
> 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
>
> 03/11/2019 04:52, Wang, Haiyue:
> > Hi Thomas,
> >
> > Reply the missed:
> >
> > From: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > 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
> > > > -enum rte_eth_burst_mode_option {
> > > > - RTE_ETH_BURST_SCALAR = (1 << 0),
> > > > - RTE_ETH_BURST_VECTOR = (1 << 1),
> > > > -
> > > > - /**< bits[15:2] are reserved for each vector type */
> > > > - RTE_ETH_BURST_ALTIVEC = (1 << 2),
> > > > - RTE_ETH_BURST_NEON = (1 << 3),
> > > > - RTE_ETH_BURST_SSE = (1 << 4),
> > > > - RTE_ETH_BURST_AVX2 = (1 << 5),
> > > > - RTE_ETH_BURST_AVX512 = (1 << 6),
> > > > -
> > > > - RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> > > > - RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> > > > - RTE_ETH_BURST_SIMPLE = (1 << 18),
> > > > -
> > > > - RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> > > > -};
> > > > +#define RTE_ETH_BURST_SCALAR (1ULL << 0)
> > > > +#define RTE_ETH_BURST_VECTOR (1ULL << 1)
> > >
> > > Only one bit is needed: if it is not vector, it is scalar.
> > > I think you can remove the scalar bit.
> > >
> >
> > 1. 'options' can be all zero, so we can't assume it is 'scalar', so let's the PMD
> > set it explicitly.
>
> So what means zero? neither scalar nor vector?
>
No log output, since PMD doesn't want to say something.
> > 2. As replied before, it seems a little redundant, but a bit-opaque definition
> > will make string format easily, like a direct 'for' loop:
> > ----------------------------------
> > static const struct {
> > uint64_t option;
> > const char *name;
> > } rte_burst_option_names[] = {
> > { RTE_ETH_BURST_SCALAR, "Scalar" },
> > { RTE_ETH_BURST_VECTOR, "Vector" },
> >
> > { RTE_ETH_BURST_ALTIVEC, "AltiVec" },
> > { RTE_ETH_BURST_NEON, "Neon" },
> > ----------------------------------
> >
> > And in fact, only one vector type will be returned, but if use the bit save definition
> > like register, there will be something like 'RTE_ETH_BURST_VERTOR_TYPE_MASK', this will
> > make PMD and string format harder ...
>
> I don't understand what is hard in parsing a bit.
> But I'm probably not skilled enough.
>
> Also I would be interested to understand what means scalar exactly here?
> It is processing packets one by one?
> And vector, does it mean four packets at once?
> How can I differentiate a function which is processing packets two packets at once?
>
About vector, What I know is : git log -p drivers/net/mlx5/mlx5_rxtx_vec_altivec.h
For scalar, what I know is the function written by pure C syntax.
> > > > +/**< bits[15:2] are reserved for each vector type */
> > >
> > > Why 15:2 instead of 2:15?
> >
> > Changed as:
> > /**< bits 2 ~ 15 are reserved for each vector type */
> >
> > > > +#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.
> > >
> > > > +/**< Support per queue burst */
> > > > +#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
> > >
> > > This comment is meaningless.
> > > If this bit has a usage, please explain how to use this bit
> > > in the comment.
> > >
> >
> > Changes as:
> >
> > /**< If the Rx/Tx queues have different burst mode description, the bit will be
> > * set by PMD, then the application can enumerate to retrive burst description
> > * for all PMD queues.
> > */
>
> OK this is a better description, thanks.
> Indeed this parameter looks to be a good idea.
> If we would drop this bit-field, the per-queue bit could be
> a parameter of the function.
>
I've no objection to drop bit-field. If the experimental code can have something good,
that's fine enough.
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&data=02%7C01%7Cviacheslavo
> > > >
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > > >
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > > >
> > a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&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&data=02%7C01%7Cviacheslavo
> > > >
> > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7
> > > >
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat
> > > >
> > a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&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&data=02%7C01%7Cviac
> > heslavo%40mellanox.com%7C0ba4e73594684f944b7608d760525c97%7Ca6
> > 52971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083779173049231&a
> > mp;sdata=2NCHJJpsAYsb07FTEmvr4EiIVudm7ZCVhemh2g1MBG0%3D&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!
> > > > > > ;-)
On 11/3/2019 7:50 AM, Slava Ovsiienko wrote:
>
>
>> -----Original Message-----
>> From: Damjan Marion (damarion) <damarion@cisco.com>
>> Sent: Saturday, November 2, 2019 21:21
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>> Cc: Liu, Yu Y <yu.y.liu@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
>> Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org;
>> arybchenko@solarflare.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> jerinjacobk@gmail.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Ray Kinsella
>> <ray.kinsella@intel.com>; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
>> information
>>
>>
>>
>>> On 2 Nov 2019, at 09:38, Slava Ovsiienko <viacheslavo@mellanox.com>
>> wrote:
>>>
>>> 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).
>>
>> This is jut debug CLI print, it was added by me as a result of frustration of
>> dealing constantly with operational issues where people are reporting lower
>> performance caused by DPDK deciding for variety of reasons to switch from
>> vector PMD to scalar one.
>
> And it seems there is no need for flags, as for me.
> I think the simple string would be quite enough to identify the datapath routine.
> Also, we have exact the same issue with mlx5 PMD, so the API (in simple
> string version) is desirable (+1 from me).
>
Hi Thomas, Slava,
It has been discussed in the previous mail thread [1], there was no objection to
it and Haiyue seems send the version based on it.
For me having the flag still makes sense, because:
1) It helps standardizing the provided string.
2) Helps to the application to consume the provided data via API (not text)
The idea was PMD sets the flag for the know standard features, provides the text
for the non standard part.
The APIs converts the flag to the string and append to text so it will be
complete for the app if just wants to log it. Flags still has the standard part
for the application what to use it.
Initially standard part was just vectorization but it can be extended by time as
more common data path used by PMDs. Text part is always free text.
After above said, I think the API has been already discussed more than enough,
and Haiyue already sent an all string version, OK to go with it.
[1]
http://inbox.dpdk.org/dev/a6893929-f981-4701-7cce-52b5e8ec934e@intel.com/
> 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! ;-)
>>>
>
On Tue, 5 Nov 2019 15:12:07 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 11/3/2019 7:50 AM, Slava Ovsiienko wrote:
> >
> >
> >> -----Original Message-----
> >> From: Damjan Marion (damarion) <damarion@cisco.com>
> >> Sent: Saturday, November 2, 2019 21:21
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >> Cc: Liu, Yu Y <yu.y.liu@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> >> Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org;
> >> arybchenko@solarflare.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> >> jerinjacobk@gmail.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Ray Kinsella
> >> <ray.kinsella@intel.com>; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
> >> information
> >>
> >>
> >>
> >>> On 2 Nov 2019, at 09:38, Slava Ovsiienko <viacheslavo@mellanox.com>
> >> wrote:
> >>>
> >>> 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).
> >>
> >> This is jut debug CLI print, it was added by me as a result of frustration of
> >> dealing constantly with operational issues where people are reporting lower
> >> performance caused by DPDK deciding for variety of reasons to switch from
> >> vector PMD to scalar one.
> >
> > And it seems there is no need for flags, as for me.
> > I think the simple string would be quite enough to identify the datapath routine.
> > Also, we have exact the same issue with mlx5 PMD, so the API (in simple
> > string version) is desirable (+1 from me).
> >
>
> Hi Thomas, Slava,
>
> It has been discussed in the previous mail thread [1], there was no objection to
> it and Haiyue seems send the version based on it.
>
>
> For me having the flag still makes sense, because:
> 1) It helps standardizing the provided string.
> 2) Helps to the application to consume the provided data via API (not text)
>
> The idea was PMD sets the flag for the know standard features, provides the text
> for the non standard part.
> The APIs converts the flag to the string and append to text so it will be
> complete for the app if just wants to log it. Flags still has the standard part
> for the application what to use it.
> Initially standard part was just vectorization but it can be extended by time as
> more common data path used by PMDs. Text part is always free text.
>
>
> After above said, I think the API has been already discussed more than enough,
> and Haiyue already sent an all string version, OK to go with it.
>
> [1]
> http://inbox.dpdk.org/dev/a6893929-f981-4701-7cce-52b5e8ec934e@intel.com/
The string gives more flexibility there is no reason that driver should not
be free to offer any number of algorithms. Likewise, the application should
not care which algorithm is used. The return value must be for information
use only.
@@ -350,21 +350,6 @@ nic_stats_mapping_display(portid_t port_id)
nic_stats_mapping_border, nic_stats_mapping_border);
}
-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);
- }
-}
-
void
rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
{
@@ -397,10 +382,8 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
(qinfo.scattered_rx != 0) ? "on" : "off");
printf("\nNumber of RXDs: %hu", qinfo.nb_desc);
- if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0) {
- printf("\nBurst mode:");
- burst_mode_options_display(mode.options);
- }
+ if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
+ printf("\nBurst mode: %s", mode.alternate_options);
printf("\n");
}
@@ -433,10 +416,8 @@ tx_queue_infos_display(portid_t port_id, uint16_t queue_id)
(qinfo.conf.tx_deferred_start != 0) ? "on" : "off");
printf("\nNumber of TXDs: %hu", qinfo.nb_desc);
- if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) == 0) {
- printf("\nBurst mode:");
- burst_mode_options_display(mode.options);
- }
+ if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) == 0)
+ printf("\nBurst mode: %s", mode.alternate_options);
printf("\n");
}
@@ -879,8 +879,7 @@ Burst mode info
Supports to get Rx/Tx packet burst mode information.
* **[implements] eth_dev_ops**: ``rx_burst_mode_get``, ``tx_burst_mode_get``.
-* **[related] API**: ``rte_eth_rx_burst_mode_get()``, ``rte_eth_tx_burst_mode_get()``,
- ``rte_eth_burst_mode_option_name()``.
+* **[related] API**: ``rte_eth_rx_burst_mode_get()``, ``rte_eth_tx_burst_mode_get()``.
.. _nic_features_other:
@@ -114,8 +114,6 @@ New Features
``rte_eth_tx_burst_mode_get`` that allow an application
to retrieve the mode information about RX/TX packet burst
such as Scalar or Vector, and Vector technology like AVX2.
- Another new function ``rte_eth_burst_mode_option_name`` is
- provided for burst mode options stringification.
* **Updated the Intel ice driver.**
@@ -179,9 +179,6 @@ static const struct {
{ RTE_ETH_BURST_AVX2, "AVX2" },
{ RTE_ETH_BURST_AVX512, "AVX512" },
- { RTE_ETH_BURST_SCATTERED, "Scattered" },
- { RTE_ETH_BURST_BULK_ALLOC, "Bulk Alloc" },
- { RTE_ETH_BURST_SIMPLE, "Simple" },
{ RTE_ETH_BURST_PER_QUEUE, "Per Queue" },
};
@@ -4236,11 +4233,55 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
return 0;
}
+static inline const char *
+burst_mode_option_name(uint64_t option)
+{
+ const char *name = "";
+ unsigned int i;
+
+ for (i = 0; i < RTE_DIM(rte_burst_option_names); ++i) {
+ if (option == rte_burst_option_names[i].option) {
+ name = rte_burst_option_names[i].name;
+ break;
+ }
+ }
+
+ return name;
+}
+
+static int
+burst_mode_options_append(struct rte_eth_burst_mode *mode)
+{
+ int optlen, len = strlen(mode->alternate_options);
+ const char *sep = len > 0 ? ", " : "";
+ uint64_t options = mode->options;
+ int offset;
+
+ while (options != 0) {
+ offset = rte_bsf64(options);
+
+ optlen = snprintf(&mode->alternate_options[len],
+ RTE_ETH_BURST_MODE_ALT_OPT_SIZE - len,
+ "%s%s",
+ sep, burst_mode_option_name(1ULL << offset));
+ if (optlen < 0 ||
+ optlen >= (RTE_ETH_BURST_MODE_ALT_OPT_SIZE - len))
+ return -ENOSPC; /* An error or output was truncated */
+
+ len += optlen;
+ options &= ~(1ULL << offset);
+ sep = " ";
+ }
+
+ return 0;
+}
+
int
rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
struct rte_eth_burst_mode *mode)
{
struct rte_eth_dev *dev;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -4256,8 +4297,12 @@ rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_burst_mode_get, -ENOTSUP);
memset(mode, 0, sizeof(*mode));
- return eth_err(port_id,
- dev->dev_ops->rx_burst_mode_get(dev, queue_id, mode));
+
+ ret = dev->dev_ops->rx_burst_mode_get(dev, queue_id, mode);
+ if (ret == 0)
+ ret = burst_mode_options_append(mode);
+
+ return eth_err(port_id, ret);
}
int
@@ -4265,6 +4310,7 @@ rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
struct rte_eth_burst_mode *mode)
{
struct rte_eth_dev *dev;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -4280,24 +4326,12 @@ rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_burst_mode_get, -ENOTSUP);
memset(mode, 0, sizeof(*mode));
- return eth_err(port_id,
- dev->dev_ops->tx_burst_mode_get(dev, queue_id, mode));
-}
-
-const char *
-rte_eth_burst_mode_option_name(uint64_t option)
-{
- const char *name = "";
- unsigned int i;
- for (i = 0; i < RTE_DIM(rte_burst_option_names); ++i) {
- if (option == rte_burst_option_names[i].option) {
- name = rte_burst_option_names[i].name;
- break;
- }
- }
+ ret = dev->dev_ops->tx_burst_mode_get(dev, queue_id, mode);
+ if (ret == 0)
+ ret = burst_mode_options_append(mode);
- return name;
+ return eth_err(port_id, ret);
}
int
@@ -1250,30 +1250,29 @@ struct rte_eth_txq_info {
/**
* Burst mode types, values can be ORed to define the burst mode of a driver.
*/
-enum rte_eth_burst_mode_option {
- RTE_ETH_BURST_SCALAR = (1 << 0),
- RTE_ETH_BURST_VECTOR = (1 << 1),
-
- /**< bits[15:2] are reserved for each vector type */
- RTE_ETH_BURST_ALTIVEC = (1 << 2),
- RTE_ETH_BURST_NEON = (1 << 3),
- RTE_ETH_BURST_SSE = (1 << 4),
- RTE_ETH_BURST_AVX2 = (1 << 5),
- RTE_ETH_BURST_AVX512 = (1 << 6),
-
- RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
- RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
- RTE_ETH_BURST_SIMPLE = (1 << 18),
-
- RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
-};
+#define RTE_ETH_BURST_SCALAR (1ULL << 0)
+#define RTE_ETH_BURST_VECTOR (1ULL << 1)
+/**< bits[15:2] are reserved for each vector type */
+#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)
+/**< Support per queue burst */
+#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
/**
* Ethernet device RX/TX queue packet burst mode information structure.
* Used to retrieve information about packet burst mode setting.
*/
+#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];
};
/** Maximum name length for extended statistics counters */
@@ -3706,20 +3705,6 @@ __rte_experimental
int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
struct rte_eth_burst_mode *mode);
-/**
- * Retrieve name about burst mode option.
- *
- * @param option
- * The burst mode option of type *rte_eth_burst_mode_option*.
- *
- * @return
- * - "": Not found
- * - "xxx": name of the mode option.
- */
-__rte_experimental
-const char *
-rte_eth_burst_mode_option_name(uint64_t option);
-
/**
* Retrieve device registers and register attributes (number of registers and
* register size)
@@ -287,5 +287,4 @@ EXPERIMENTAL {
# added in 19.11
rte_eth_rx_burst_mode_get;
rte_eth_tx_burst_mode_get;
- rte_eth_burst_mode_option_name;
};