diff mbox series

[v8,2/9] net/virtio: enable vectorized path

Message ID 20200423123106.78513-3-yong.liu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series add packed ring vectorized path |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Marvin Liu April 23, 2020, 12:30 p.m. UTC 
  Previously, virtio split ring vectorized path is enabled as default.
This is not suitable for everyone because of that path not follow virtio
spec. Add new config for virtio vectorized path selection. By default
vectorized path is disabled.

Signed-off-by: Marvin Liu <yong.liu@intel.com>

Comments

Maxime Coquelin April 23, 2020, 8:33 a.m. UTC | #1 
On 4/23/20 2:30 PM, Marvin Liu wrote:
> Previously, virtio split ring vectorized path is enabled as default.

s/is/was/
s/as/by/

> This is not suitable for everyone because of that path not follow virtio

s/because of that path not follow/because that path does not follow the/

> spec. Add new config for virtio vectorized path selection. By default
> vectorized path is disabled.

I think we can keep it enabled by default for consistency between make &
meson, now that you are providing a devarg for it that is disabled by
default.

Maybe we can just drop this config flag, what do you think?

Thanks,
Maxime

> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/config/common_base b/config/common_base
> index 00d8d0792..334a26a17 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -456,6 +456,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> +CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
>  
>  #
>  # Compile virtio device emulation inside virtio PMD driver
> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
> index c9edb84ee..4b69827ab 100644
> --- a/drivers/net/virtio/Makefile
> +++ b/drivers/net/virtio/Makefile
> @@ -28,6 +28,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
>  
> +ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR),y)
>  ifeq ($(CONFIG_RTE_ARCH_X86),y)
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_sse.c
>  else ifeq ($(CONFIG_RTE_ARCH_PPC_64),y)
> @@ -35,6 +36,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_altivec.c
>  else ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM) $(CONFIG_RTE_ARCH_ARM64)),)
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_neon.c
>  endif
> +endif
>  
>  ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
> diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
> index 15150eea1..ce3525ef5 100644
> --- a/drivers/net/virtio/meson.build
> +++ b/drivers/net/virtio/meson.build
> @@ -8,6 +8,7 @@ sources += files('virtio_ethdev.c',
>  	'virtqueue.c')
>  deps += ['kvargs', 'bus_pci']
>  
> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_INC_VECTOR', 1)
>  if arch_subdir == 'x86'
>  	sources += files('virtio_rxtx_simple_sse.c')
>  elif arch_subdir == 'ppc'
>
Marvin Liu April 23, 2020, 8:46 a.m. UTC | #2 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, April 23, 2020 4:34 PM
> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Wang, Zhihong <zhihong.wang@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH v8 2/9] net/virtio: enable vectorized path
> 
> 
> 
> On 4/23/20 2:30 PM, Marvin Liu wrote:
> > Previously, virtio split ring vectorized path is enabled as default.
> 
> s/is/was/
> s/as/by/
> 
> > This is not suitable for everyone because of that path not follow virtio
> 
> s/because of that path not follow/because that path does not follow the/
> 
> > spec. Add new config for virtio vectorized path selection. By default
> > vectorized path is disabled.
> 
> I think we can keep it enabled by default for consistency between make &
> meson, now that you are providing a devarg for it that is disabled by
> default.
> 
> Maybe we can just drop this config flag, what do you think?
> 

Maxime, 
Devarg will only have effect on virtio-user path selection, while DPDK configuration can affect both virtio pmd and virtio-user.
It maybe worth to add new configuration as it can allow user to choice whether disabled vectorized path in virtio pmd.  
IMHO, AVX512 instructions should be selective in each component. 

Regards,
Marvin

> Thanks,
> Maxime
> 
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> > diff --git a/config/common_base b/config/common_base
> > index 00d8d0792..334a26a17 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -456,6 +456,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> > +CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
> >
> >  #
> >  # Compile virtio device emulation inside virtio PMD driver
> > diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
> > index c9edb84ee..4b69827ab 100644
> > --- a/drivers/net/virtio/Makefile
> > +++ b/drivers/net/virtio/Makefile
> > @@ -28,6 +28,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
> virtio_rxtx.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
> >
> > +ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR),y)
> >  ifeq ($(CONFIG_RTE_ARCH_X86),y)
> >  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_sse.c
> >  else ifeq ($(CONFIG_RTE_ARCH_PPC_64),y)
> > @@ -35,6 +36,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
> virtio_rxtx_simple_altivec.c
> >  else ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM)
> $(CONFIG_RTE_ARCH_ARM64)),)
> >  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_neon.c
> >  endif
> > +endif
> >
> >  ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
> >  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
> > diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
> > index 15150eea1..ce3525ef5 100644
> > --- a/drivers/net/virtio/meson.build
> > +++ b/drivers/net/virtio/meson.build
> > @@ -8,6 +8,7 @@ sources += files('virtio_ethdev.c',
> >  	'virtqueue.c')
> >  deps += ['kvargs', 'bus_pci']
> >
> > +dpdk_conf.set('RTE_LIBRTE_VIRTIO_INC_VECTOR', 1)
> >  if arch_subdir == 'x86'
> >  	sources += files('virtio_rxtx_simple_sse.c')
> >  elif arch_subdir == 'ppc'
> >
Maxime Coquelin April 23, 2020, 8:49 a.m. UTC | #3 
On 4/23/20 10:46 AM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, April 23, 2020 4:34 PM
>> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> Wang, Zhihong <zhihong.wang@intel.com>
>> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
>> Subject: Re: [PATCH v8 2/9] net/virtio: enable vectorized path
>>
>>
>>
>> On 4/23/20 2:30 PM, Marvin Liu wrote:
>>> Previously, virtio split ring vectorized path is enabled as default.
>>
>> s/is/was/
>> s/as/by/
>>
>>> This is not suitable for everyone because of that path not follow virtio
>>
>> s/because of that path not follow/because that path does not follow the/
>>
>>> spec. Add new config for virtio vectorized path selection. By default
>>> vectorized path is disabled.
>>
>> I think we can keep it enabled by default for consistency between make &
>> meson, now that you are providing a devarg for it that is disabled by
>> default.
>>
>> Maybe we can just drop this config flag, what do you think?
>>
> 
> Maxime, 
> Devarg will only have effect on virtio-user path selection, while DPDK configuration can affect both virtio pmd and virtio-user.
> It maybe worth to add new configuration as it can allow user to choice whether disabled vectorized path in virtio pmd. 

Ok, so we had a misunderstanding. I was requesting the the devarg to be
effective also for the Virtio PMD, disabled by default.

Thanks,
Maxime
> IMHO, AVX512 instructions should be selective in each component. 
> 
> Regards,
> Marvin
> 
>> Thanks,
>> Maxime
>>
>>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>>>
>>> diff --git a/config/common_base b/config/common_base
>>> index 00d8d0792..334a26a17 100644
>>> --- a/config/common_base
>>> +++ b/config/common_base
>>> @@ -456,6 +456,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
>>> +CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
>>>
>>>  #
>>>  # Compile virtio device emulation inside virtio PMD driver
>>> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
>>> index c9edb84ee..4b69827ab 100644
>>> --- a/drivers/net/virtio/Makefile
>>> +++ b/drivers/net/virtio/Makefile
>>> @@ -28,6 +28,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
>> virtio_rxtx.c
>>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
>>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
>>>
>>> +ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR),y)
>>>  ifeq ($(CONFIG_RTE_ARCH_X86),y)
>>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_sse.c
>>>  else ifeq ($(CONFIG_RTE_ARCH_PPC_64),y)
>>> @@ -35,6 +36,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
>> virtio_rxtx_simple_altivec.c
>>>  else ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM)
>> $(CONFIG_RTE_ARCH_ARM64)),)
>>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_neon.c
>>>  endif
>>> +endif
>>>
>>>  ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
>>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
>>> diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
>>> index 15150eea1..ce3525ef5 100644
>>> --- a/drivers/net/virtio/meson.build
>>> +++ b/drivers/net/virtio/meson.build
>>> @@ -8,6 +8,7 @@ sources += files('virtio_ethdev.c',
>>>  	'virtqueue.c')
>>>  deps += ['kvargs', 'bus_pci']
>>>
>>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_INC_VECTOR', 1)
>>>  if arch_subdir == 'x86'
>>>  	sources += files('virtio_rxtx_simple_sse.c')
>>>  elif arch_subdir == 'ppc'
>>>
>
Marvin Liu April 23, 2020, 9:59 a.m. UTC | #4 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, April 23, 2020 4:50 PM
> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Wang, Zhihong <zhihong.wang@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH v8 2/9] net/virtio: enable vectorized path
> 
> 
> 
> On 4/23/20 10:46 AM, Liu, Yong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, April 23, 2020 4:34 PM
> >> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> Wang, Zhihong <zhihong.wang@intel.com>
> >> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> >> Subject: Re: [PATCH v8 2/9] net/virtio: enable vectorized path
> >>
> >>
> >>
> >> On 4/23/20 2:30 PM, Marvin Liu wrote:
> >>> Previously, virtio split ring vectorized path is enabled as default.
> >>
> >> s/is/was/
> >> s/as/by/
> >>
> >>> This is not suitable for everyone because of that path not follow virtio
> >>
> >> s/because of that path not follow/because that path does not follow the/
> >>
> >>> spec. Add new config for virtio vectorized path selection. By default
> >>> vectorized path is disabled.
> >>
> >> I think we can keep it enabled by default for consistency between make &
> >> meson, now that you are providing a devarg for it that is disabled by
> >> default.
> >>
> >> Maybe we can just drop this config flag, what do you think?
> >>
> >
> > Maxime,
> > Devarg will only have effect on virtio-user path selection, while DPDK
> configuration can affect both virtio pmd and virtio-user.
> > It maybe worth to add new configuration as it can allow user to choice
> whether disabled vectorized path in virtio pmd.
> 
> Ok, so we had a misunderstanding. I was requesting the the devarg to be
> effective also for the Virtio PMD, disabled by default.
> 
Got you, will change in next vesion.

> Thanks,
> Maxime
> > IMHO, AVX512 instructions should be selective in each component.
> >
> > Regards,
> > Marvin
> >
> >> Thanks,
> >> Maxime
> >>
> >>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >>>
> >>> diff --git a/config/common_base b/config/common_base
> >>> index 00d8d0792..334a26a17 100644
> >>> --- a/config/common_base
> >>> +++ b/config/common_base
> >>> @@ -456,6 +456,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> >>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
> >>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
> >>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> >>> +CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
> >>>
> >>>  #
> >>>  # Compile virtio device emulation inside virtio PMD driver
> >>> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
> >>> index c9edb84ee..4b69827ab 100644
> >>> --- a/drivers/net/virtio/Makefile
> >>> +++ b/drivers/net/virtio/Makefile
> >>> @@ -28,6 +28,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
> >> virtio_rxtx.c
> >>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
> >>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
> >>>
> >>> +ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR),y)
> >>>  ifeq ($(CONFIG_RTE_ARCH_X86),y)
> >>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_sse.c
> >>>  else ifeq ($(CONFIG_RTE_ARCH_PPC_64),y)
> >>> @@ -35,6 +36,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) +=
> >> virtio_rxtx_simple_altivec.c
> >>>  else ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM)
> >> $(CONFIG_RTE_ARCH_ARM64)),)
> >>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_neon.c
> >>>  endif
> >>> +endif
> >>>
> >>>  ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
> >>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
> >>> diff --git a/drivers/net/virtio/meson.build
> b/drivers/net/virtio/meson.build
> >>> index 15150eea1..ce3525ef5 100644
> >>> --- a/drivers/net/virtio/meson.build
> >>> +++ b/drivers/net/virtio/meson.build
> >>> @@ -8,6 +8,7 @@ sources += files('virtio_ethdev.c',
> >>>  	'virtqueue.c')
> >>>  deps += ['kvargs', 'bus_pci']
> >>>
> >>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_INC_VECTOR', 1)
> >>>  if arch_subdir == 'x86'
> >>>  	sources += files('virtio_rxtx_simple_sse.c')
> >>>  elif arch_subdir == 'ppc'
> >>>
> >
diff mbox series

Patch

diff --git a/config/common_base b/config/common_base
index 00d8d0792..334a26a17 100644
--- a/config/common_base
+++ b/config/common_base
@@ -456,6 +456,7 @@  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
+CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
 
 #
 # Compile virtio device emulation inside virtio PMD driver
diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
index c9edb84ee..4b69827ab 100644
--- a/drivers/net/virtio/Makefile
+++ b/drivers/net/virtio/Makefile
@@ -28,6 +28,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
 
+ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR),y)
 ifeq ($(CONFIG_RTE_ARCH_X86),y)
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_sse.c
 else ifeq ($(CONFIG_RTE_ARCH_PPC_64),y)
@@ -35,6 +36,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_altivec.c
 else ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM) $(CONFIG_RTE_ARCH_ARM64)),)
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple_neon.c
 endif
+endif
 
 ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
index 15150eea1..ce3525ef5 100644
--- a/drivers/net/virtio/meson.build
+++ b/drivers/net/virtio/meson.build
@@ -8,6 +8,7 @@  sources += files('virtio_ethdev.c',
 	'virtqueue.c')
 deps += ['kvargs', 'bus_pci']
 
+dpdk_conf.set('RTE_LIBRTE_VIRTIO_INC_VECTOR', 1)
 if arch_subdir == 'x86'
 	sources += files('virtio_rxtx_simple_sse.c')
 elif arch_subdir == 'ppc'