[dpdk-dev] [PATCH v5 6/8] net/ice: support Rx AVX2 vector

[Lu, Wenzhuo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> Sent: Tuesday, March 26, 2019 5:29 PM
> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 6/8] net/ice: support Rx AVX2 vector
> 
> Hi,
> 
> On 3/26/19 2:00 AM, Lu, Wenzhuo wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> >> Sent: Monday, March 25, 2019 4:26 PM
> >> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v5 6/8] net/ice: support Rx AVX2
> >> vector
> >>
> >> Hi,
> >>
> >> On 3/25/19 3:22 AM, Lu, Wenzhuo wrote:
> >>> Hi Maxime,
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> >>>> Sent: Friday, March 22, 2019 6:12 PM
> >>>> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH v5 6/8] net/ice: support Rx AVX2
> >>>> vector
> >>>
> >>>
> >>>>> +#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
> >>>>
> >>>> I see same is done for other Intel NICs, but I wonder what would be
> >>>> the performance cost of making it dynamic, if any cost?
> >>> Currently we don't have a good idea to make it dynamic. If we use
> >>> pointer
> >> to point to different functions for 16 byte and 32 byte, there's too
> >> much duplicate code to make it hard to maintain. If we use the same
> >> function, and check the configure in it. It impacts the performance.
> >>
> >> Have you done some measurements, what would be the performance
> >> impact?
> > I mean if we check the configuration is 16 byte or 32 byte, this check will
> consume extra CPU cycles.
> > That why I think the better way is to have different paths for 16 byte and
> 32 byte. We should choose the appropriate path at the beginning.
> >
> >>
> >>> As HW does not support to change the configuration dynamically. The
> >> device must be stopped and restarted if the configuration is changed.
> >> It's not very helpful to make it a dynamic configuration. We assume
> >> that the users can make their choice at the beginning and will not change
> it.
> >>
> >> The problem is that the user has to recompile to switch between the
> >> two configurations. And it may not be an option for the user if he
> >> uses dpdk packaged by a distribution, for example.
> >>
> >> Maybe I was not clear, but I don't mean to be able to switch mode
> >> while the port is started. I think it would be better to make it
> >> possible to switch mode at application startup time.
> > Yes, I understand the problem is the recompiling. But we think the users
> will not change it after they made decision. That's why's acceptable in
> previous drivers.
> 
> The problem is that the user may not be able to change it, if he does not get
> DPDK from source but from a distribution like Debian, Ubuntu or Red Hat.
> 
> In this case, it means the user has no choice than sticking to 32 bytes
> descriptors.
Normally using 32 bytes is the default behavior and it's good to do that.
But I have to say I don't quite understand the scenario. DPDK is open source, whatever OS that users are using, nothing prevents them going to dpdk website to get the code and customize it.

> 
> > Agree it's better to remove all the compile configuration. Looks like that's
> what we're trying to do. We'd like to think about how to optimize it later.
> 
> My suggestion would be a devarg, so that you can have a per-port policy
> (which is another advantage of doing so).
We're thinking about moving some configuration from per port to per queue.
To my opinion, it's also a case that maybe it’s better to make it a queue's parameter.
Obviously it’s an API change. So we have to be slow and careful :)

> 
> >
> >
> >>
> >>>
> >>>>
> >>>> Having it dynamic (as a dev arg for instance) would make it
> >>>> possible to change the value when the user is using dpdk from a
> >>>> distro. It would also help testing coverage.
> >>>>
> >>>> Btw, how do you select this option with meson build system?
> >>> Not very familiar with meson. As I know, we can change the
> >>> meson.build
> >> to add the configure.
> >>>
> >>
> >> Ok, then please try to do it, because the legacy build system is
> >> going to be deprecated.