[dpdk-dev] [PATCH] net/virtio: fix AVX512 datapath selection

[dpdk-dev] [PATCH] net/virtio: fix AVX512 datapath selection

[Maxime Coquelin]

The AVX512 packed ring datapath selection was only done
at build time, but it should also be checked at runtime
that the CPU supports it.

This patch add a CPU flags check so that non-vectorized
path is selected at runtime if AVX512 is not supported.

Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 312871cb48..49ccef12c7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1965,8 +1965,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 			PMD_DRV_LOG(INFO,
 				"building environment do not support packed ring vectorized");
 #else
-			hw->use_vec_rx = 1;
-			hw->use_vec_tx = 1;
+			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
+				hw->use_vec_rx = 1;
+				hw->use_vec_tx = 1;
+			}
 #endif
 		}
 	}
-- 
2.25.4

Re: [dpdk-dev] [PATCH] net/virtio: fix AVX512 datapath selection

[Ferruh Yigit]

On 5/11/2020 3:47 PM, Maxime Coquelin wrote:
> The AVX512 packed ring datapath selection was only done
> at build time, but it should also be checked at runtime
> that the CPU supports it.
> 
> This patch add a CPU flags check so that non-vectorized
> path is selected at runtime if AVX512 is not supported.
> 
> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 312871cb48..49ccef12c7 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1965,8 +1965,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  			PMD_DRV_LOG(INFO,
>  				"building environment do not support packed ring vectorized");
>  #else
> -			hw->use_vec_rx = 1;
> -			hw->use_vec_tx = 1;
> +			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
> +				hw->use_vec_rx = 1;
> +				hw->use_vec_tx = 1;
> +			}
>  #endif
>  		}
>  	}
> 

Bruce & Radu also highlighted that the in meson, instead of updating 'cflags',
which is for all driver, the cflags for 'virtio_rxtx_packed_avx.c' should be
updated.

[dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Ferruh Yigit]

From: Maxime Coquelin <maxime.coquelin@redhat.com>

The AVX512 packed ring datapath selection was only done
at build time, but it should also be checked at runtime
that the CPU supports it.

This patch add a CPU flags check so that non-vectorized
path is selected at runtime if AVX512 is not supported.

Also in meson build enable vectorization only for relevant file, not for
all driver.

Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Bruce Richardson <bruce.richardson@intel.com>
Cc: Radu Nicolau <radu.nicolau@intel.com>
Cc: Luca Boccassi <bluca@debian.org>

For meson I mainly adapted implementation from other driver, not able to
test or verify myself.
---
 drivers/net/virtio/meson.build     | 9 +++++++--
 drivers/net/virtio/virtio_ethdev.c | 6 ++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
index 5cc529f6aa..3fd6051f4b 100644
--- a/drivers/net/virtio/meson.build
+++ b/drivers/net/virtio/meson.build
@@ -11,8 +11,14 @@ deps += ['kvargs', 'bus_pci']
 if arch_subdir == 'x86'
 	if not machine_args.contains('-mno-avx512f')
 		if cc.has_argument('-mavx512f') and cc.has_argument('-mavx512vl') and cc.has_argument('-mavx512bw')
-			cflags += ['-mavx512f', '-mavx512bw', '-mavx512vl']
 			cflags += ['-DCC_AVX512_SUPPORT']
+			virtio_avx512_lib = static_library('virtio_avx512_lib',
+					      'virtio_rxtx_packed_avx.c',
+					      dependencies: [static_rte_ethdev,
+						static_rte_kvargs, static_rte_bus_pci],
+					      include_directories: includes,
+					      c_args: [cflags, '-mavx512f', '-mavx512bw', '-mavx512vl'])
+			objs += virtio_avx512_lib.extract_objects('virtio_rxtx_packed_avx.c')
 			if (toolchain == 'gcc' and cc.version().version_compare('>=8.3.0'))
 				cflags += '-DVHOST_GCC_UNROLL_PRAGMA'
 			elif (toolchain == 'clang' and cc.version().version_compare('>=3.7.0'))
@@ -20,7 +26,6 @@ if arch_subdir == 'x86'
 			elif (toolchain == 'icc' and cc.version().version_compare('>=16.0.0'))
 				cflags += '-DVHOST_ICC_UNROLL_PRAGMA'
 			endif
-			sources += files('virtio_rxtx_packed_avx.c')
 		endif
 	endif
 	sources += files('virtio_rxtx_simple_sse.c')
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 312871cb48..49ccef12c7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1965,8 +1965,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 			PMD_DRV_LOG(INFO,
 				"building environment do not support packed ring vectorized");
 #else
-			hw->use_vec_rx = 1;
-			hw->use_vec_tx = 1;
+			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
+				hw->use_vec_rx = 1;
+				hw->use_vec_tx = 1;
+			}
 #endif
 		}
 	}
-- 
2.25.4

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Maxime Coquelin]

On 5/11/20 8:48 PM, Ferruh Yigit wrote:
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> The AVX512 packed ring datapath selection was only done
> at build time, but it should also be checked at runtime
> that the CPU supports it.
> 
> This patch add a CPU flags check so that non-vectorized
> path is selected at runtime if AVX512 is not supported.
> 
> Also in meson build enable vectorization only for relevant file, not for
> all driver.
> 
> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Bruce Richardson <bruce.richardson@intel.com>
> Cc: Radu Nicolau <radu.nicolau@intel.com>
> Cc: Luca Boccassi <bluca@debian.org>
> 
> For meson I mainly adapted implementation from other driver, not able to
> test or verify myself.
> ---
>  drivers/net/virtio/meson.build     | 9 +++++++--
>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)

Thanks Ferruh, I cannot test either right now but it looks good to me:

In case you're waiting for it:
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Maxime

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Ferruh Yigit]

On 5/11/2020 8:49 PM, Maxime Coquelin wrote:
> 
> 
> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> The AVX512 packed ring datapath selection was only done
>> at build time, but it should also be checked at runtime
>> that the CPU supports it.
>>
>> This patch add a CPU flags check so that non-vectorized
>> path is selected at runtime if AVX512 is not supported.
>>
>> Also in meson build enable vectorization only for relevant file, not for
>> all driver.
>>
>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: Bruce Richardson <bruce.richardson@intel.com>
>> Cc: Radu Nicolau <radu.nicolau@intel.com>
>> Cc: Luca Boccassi <bluca@debian.org>
>>
>> For meson I mainly adapted implementation from other driver, not able to
>> test or verify myself.
>> ---
>>  drivers/net/virtio/meson.build     | 9 +++++++--
>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> Thanks Ferruh, I cannot test either right now but it looks good to me:
> 
> In case you're waiting for it:
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Liu, Yong]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 12, 2020 3:50 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Liu,
> Yong <yong.liu@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> Luca Boccassi <bluca@debian.org>
> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> 
> 
> 
> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > The AVX512 packed ring datapath selection was only done
> > at build time, but it should also be checked at runtime
> > that the CPU supports it.
> >
> > This patch add a CPU flags check so that non-vectorized
> > path is selected at runtime if AVX512 is not supported.
> >
> > Also in meson build enable vectorization only for relevant file, not for
> > all driver.
> >
> > Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > Cc: Bruce Richardson <bruce.richardson@intel.com>
> > Cc: Radu Nicolau <radu.nicolau@intel.com>
> > Cc: Luca Boccassi <bluca@debian.org>
> >
> > For meson I mainly adapted implementation from other driver, not able
> to
> > test or verify myself.
> > ---
> >  drivers/net/virtio/meson.build     | 9 +++++++--
> >  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> Thanks Ferruh, I cannot test either right now but it looks good to me:
> 

Hi Maxime & Ferruh,
IMHO, meson build update is the essential part for fixing unexpected AVX512 instructions. 
Change in virtio_ethdev may cause building issues on ppc and arm platform.  Is it convenient to revert that change?

Regards,
Marvin

> In case you're waiting for it:
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Maxime

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Maxime Coquelin]

On 5/12/20 5:29 AM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, May 12, 2020 3:50 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Liu,
>> Yong <yong.liu@intel.com>
>> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
>> Marchand <david.marchand@redhat.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
>> Luca Boccassi <bluca@debian.org>
>> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
>>
>>
>>
>> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> The AVX512 packed ring datapath selection was only done
>>> at build time, but it should also be checked at runtime
>>> that the CPU supports it.
>>>
>>> This patch add a CPU flags check so that non-vectorized
>>> path is selected at runtime if AVX512 is not supported.
>>>
>>> Also in meson build enable vectorization only for relevant file, not for
>>> all driver.
>>>
>>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> ---
>>> Cc: Bruce Richardson <bruce.richardson@intel.com>
>>> Cc: Radu Nicolau <radu.nicolau@intel.com>
>>> Cc: Luca Boccassi <bluca@debian.org>
>>>
>>> For meson I mainly adapted implementation from other driver, not able
>> to
>>> test or verify myself.
>>> ---
>>>  drivers/net/virtio/meson.build     | 9 +++++++--
>>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> Thanks Ferruh, I cannot test either right now but it looks good to me:
>>
> 
> Hi Maxime & Ferruh,
> IMHO, meson build update is the essential part for fixing unexpected AVX512 instructions. 
> Change in virtio_ethdev may cause building issues on ppc and arm platform.  Is it convenient to revert that change?

As replied to v1:

With a bit more of context, we can see that it only affects packed ring
when CC_AVX512_SUPPORT is set. So it does break neither split ring nor
ARM/PPC:

	if (vectorized) {
		if (!vtpci_packed_queue(hw)) {
			hw->use_vec_rx = 1;
		} else {
#if !defined(CC_AVX512_SUPPORT)
			PMD_DRV_LOG(INFO,
				"building environment do not support packed ring vectorized");
#else
			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
				hw->use_vec_rx = 1;
				hw->use_vec_tx = 1;
			}
#endif
		}
	}

So IMO, no revert has to be done.

> Regards,
> Marvin
> 
>> In case you're waiting for it:
>> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Maxime
> 

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Liu, Yong]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 12, 2020 4:36 PM
> To: Liu, Yong <yong.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> Luca Boccassi <bluca@debian.org>; Wang, Zhihong
> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> 
> 
> 
> On 5/12/20 5:29 AM, Liu, Yong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, May 12, 2020 3:50 AM
> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Zhihong
> >> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Liu,
> >> Yong <yong.liu@intel.com>
> >> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> >> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> >> Luca Boccassi <bluca@debian.org>
> >> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> >>
> >>
> >>
> >> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>
> >>> The AVX512 packed ring datapath selection was only done
> >>> at build time, but it should also be checked at runtime
> >>> that the CPU supports it.
> >>>
> >>> This patch add a CPU flags check so that non-vectorized
> >>> path is selected at runtime if AVX512 is not supported.
> >>>
> >>> Also in meson build enable vectorization only for relevant file, not for
> >>> all driver.
> >>>
> >>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> >>>
> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> ---
> >>> Cc: Bruce Richardson <bruce.richardson@intel.com>
> >>> Cc: Radu Nicolau <radu.nicolau@intel.com>
> >>> Cc: Luca Boccassi <bluca@debian.org>
> >>>
> >>> For meson I mainly adapted implementation from other driver, not
> able
> >> to
> >>> test or verify myself.
> >>> ---
> >>>  drivers/net/virtio/meson.build     | 9 +++++++--
> >>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
> >>>  2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> Thanks Ferruh, I cannot test either right now but it looks good to me:
> >>
> >
> > Hi Maxime & Ferruh,
> > IMHO, meson build update is the essential part for fixing unexpected
> AVX512 instructions.
> > Change in virtio_ethdev may cause building issues on ppc and arm
> platform.  Is it convenient to revert that change?
> 
> As replied to v1:
> 
> With a bit more of context, we can see that it only affects packed ring
> when CC_AVX512_SUPPORT is set. So it does break neither split ring nor
> ARM/PPC:
> 
> 	if (vectorized) {
> 		if (!vtpci_packed_queue(hw)) {
> 			hw->use_vec_rx = 1;
> 		} else {
> #if !defined(CC_AVX512_SUPPORT)
> 			PMD_DRV_LOG(INFO,
> 				"building environment do not support
> packed ring vectorized");
> #else
> 			if
> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
> 				hw->use_vec_rx = 1;
> 				hw->use_vec_tx = 1;
> 			}
> #endif
> 		}
> 	}
> 
> So IMO, no revert has to be done.
> 

Ok, I messed it with my previous building fix.  It will be no harm for  this double check. 

> > Regards,
> > Marvin
> >
> >> In case you're waiting for it:
> >> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>
> >> Maxime
> >

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Maxime Coquelin]

On 5/12/20 10:46 AM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, May 12, 2020 4:36 PM
>> To: Liu, Yong <yong.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
>> Marchand <david.marchand@redhat.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
>> Luca Boccassi <bluca@debian.org>; Wang, Zhihong
>> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
>> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
>>
>>
>>
>> On 5/12/20 5:29 AM, Liu, Yong wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, May 12, 2020 3:50 AM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Liu,
>>>> Yong <yong.liu@intel.com>
>>>> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
>>>> Marchand <david.marchand@redhat.com>; Richardson, Bruce
>>>> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
>>>> Luca Boccassi <bluca@debian.org>
>>>> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
>>>>
>>>>
>>>>
>>>> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>
>>>>> The AVX512 packed ring datapath selection was only done
>>>>> at build time, but it should also be checked at runtime
>>>>> that the CPU supports it.
>>>>>
>>>>> This patch add a CPU flags check so that non-vectorized
>>>>> path is selected at runtime if AVX512 is not supported.
>>>>>
>>>>> Also in meson build enable vectorization only for relevant file, not for
>>>>> all driver.
>>>>>
>>>>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>> ---
>>>>> Cc: Bruce Richardson <bruce.richardson@intel.com>
>>>>> Cc: Radu Nicolau <radu.nicolau@intel.com>
>>>>> Cc: Luca Boccassi <bluca@debian.org>
>>>>>
>>>>> For meson I mainly adapted implementation from other driver, not
>> able
>>>> to
>>>>> test or verify myself.
>>>>> ---
>>>>>  drivers/net/virtio/meson.build     | 9 +++++++--
>>>>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>>>>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> Thanks Ferruh, I cannot test either right now but it looks good to me:
>>>>
>>>
>>> Hi Maxime & Ferruh,
>>> IMHO, meson build update is the essential part for fixing unexpected
>> AVX512 instructions.
>>> Change in virtio_ethdev may cause building issues on ppc and arm
>> platform.  Is it convenient to revert that change?
>>
>> As replied to v1:
>>
>> With a bit more of context, we can see that it only affects packed ring
>> when CC_AVX512_SUPPORT is set. So it does break neither split ring nor
>> ARM/PPC:
>>
>> 	if (vectorized) {
>> 		if (!vtpci_packed_queue(hw)) {
>> 			hw->use_vec_rx = 1;
>> 		} else {
>> #if !defined(CC_AVX512_SUPPORT)
>> 			PMD_DRV_LOG(INFO,
>> 				"building environment do not support
>> packed ring vectorized");
>> #else
>> 			if
>> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
>> 				hw->use_vec_rx = 1;
>> 				hw->use_vec_tx = 1;
>> 			}
>> #endif
>> 		}
>> 	}
>>
>> So IMO, no revert has to be done.
>>
> 
> Ok, I messed it with my previous building fix.  It will be no harm for  this double check. 

While it does not break, I agree for the unnecessary double-check.
You can send a clean-up patch to remove this part in -rc3.

Thanks,
Maxime

>>> Regards,
>>> Marvin
>>>
>>>> In case you're waiting for it:
>>>> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>
>>>> Maxime
>>>
> 

Re: [dpdk-dev] [PATCH v2] net/virtio: fix AVX512 datapath selection

[Liu, Yong]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 12, 2020 6:04 PM
> To: Liu, Yong <yong.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> Luca Boccassi <bluca@debian.org>; Wang, Zhihong
> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> 
> 
> 
> On 5/12/20 10:46 AM, Liu, Yong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, May 12, 2020 4:36 PM
> >> To: Liu, Yong <yong.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> >> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> >> Luca Boccassi <bluca@debian.org>; Wang, Zhihong
> >> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> >> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> >>
> >>
> >>
> >> On 5/12/20 5:29 AM, Liu, Yong wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Tuesday, May 12, 2020 3:50 AM
> >>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Zhihong
> >>>> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Liu,
> >>>> Yong <yong.liu@intel.com>
> >>>> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; David
> >>>> Marchand <david.marchand@redhat.com>; Richardson, Bruce
> >>>> <bruce.richardson@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>;
> >>>> Luca Boccassi <bluca@debian.org>
> >>>> Subject: Re: [PATCH v2] net/virtio: fix AVX512 datapath selection
> >>>>
> >>>>
> >>>>
> >>>> On 5/11/20 8:48 PM, Ferruh Yigit wrote:
> >>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>
> >>>>> The AVX512 packed ring datapath selection was only done
> >>>>> at build time, but it should also be checked at runtime
> >>>>> that the CPU supports it.
> >>>>>
> >>>>> This patch add a CPU flags check so that non-vectorized
> >>>>> path is selected at runtime if AVX512 is not supported.
> >>>>>
> >>>>> Also in meson build enable vectorization only for relevant file, not
> for
> >>>>> all driver.
> >>>>>
> >>>>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> >>>>>
> >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>> ---
> >>>>> Cc: Bruce Richardson <bruce.richardson@intel.com>
> >>>>> Cc: Radu Nicolau <radu.nicolau@intel.com>
> >>>>> Cc: Luca Boccassi <bluca@debian.org>
> >>>>>
> >>>>> For meson I mainly adapted implementation from other driver, not
> >> able
> >>>> to
> >>>>> test or verify myself.
> >>>>> ---
> >>>>>  drivers/net/virtio/meson.build     | 9 +++++++--
> >>>>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
> >>>>>  2 files changed, 11 insertions(+), 4 deletions(-)
> >>>>
> >>>> Thanks Ferruh, I cannot test either right now but it looks good to me:
> >>>>
> >>>
> >>> Hi Maxime & Ferruh,
> >>> IMHO, meson build update is the essential part for fixing unexpected
> >> AVX512 instructions.
> >>> Change in virtio_ethdev may cause building issues on ppc and arm
> >> platform.  Is it convenient to revert that change?
> >>
> >> As replied to v1:
> >>
> >> With a bit more of context, we can see that it only affects packed ring
> >> when CC_AVX512_SUPPORT is set. So it does break neither split ring nor
> >> ARM/PPC:
> >>
> >> 	if (vectorized) {
> >> 		if (!vtpci_packed_queue(hw)) {
> >> 			hw->use_vec_rx = 1;
> >> 		} else {
> >> #if !defined(CC_AVX512_SUPPORT)
> >> 			PMD_DRV_LOG(INFO,
> >> 				"building environment do not support
> >> packed ring vectorized");
> >> #else
> >> 			if
> >> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
> >> 				hw->use_vec_rx = 1;
> >> 				hw->use_vec_tx = 1;
> >> 			}
> >> #endif
> >> 		}
> >> 	}
> >>
> >> So IMO, no revert has to be done.
> >>
> >
> > Ok, I messed it with my previous building fix.  It will be no harm for  this
> double check.
> 
> While it does not break, I agree for the unnecessary double-check.
> You can send a clean-up patch to remove this part in -rc3.
> 

Thanks a lot, have sent clean-up patch.

> Thanks,
> Maxime
> 
> >>> Regards,
> >>> Marvin
> >>>
> >>>> In case you're waiting for it:
> >>>> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>
> >>>> Maxime
> >>>
> >

Re: [dpdk-dev] [PATCH] net/virtio: fix AVX512 datapath selection

[Liu, Yong]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, May 11, 2020 10:47 PM
> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH] net/virtio: fix AVX512 datapath selection
> 
> The AVX512 packed ring datapath selection was only done
> at build time, but it should also be checked at runtime
> that the CPU supports it.
> 
> This patch add a CPU flags check so that non-vectorized
> path is selected at runtime if AVX512 is not supported.
> 
> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 312871cb48..49ccef12c7 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1965,8 +1965,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  			PMD_DRV_LOG(INFO,
>  				"building environment do not support
> packed ring vectorized");
>  #else
> -			hw->use_vec_rx = 1;
> -			hw->use_vec_tx = 1;
> +			if
> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
> +				hw->use_vec_rx = 1;
> +				hw->use_vec_tx = 1;
> +			}
>  #endif

Hi Maxime,
Here is pre-setting for vectorized path selection, virtio_dev_configure will do second time check. 
Running environment will be checked in second time check.  We can move some checks from virtio_dev_configure to here, but is it needed to do that?

BTW, both split ring and packed ring will utilized this setting, it will break split vectorized datapath is server not has AVX512F flag.
And it may cause building issue on those platforms which not defined RTE_CPUFLAG_AVX512F.

Thanks,
Marvin

>  		}
>  	}
> --
> 2.25.4

Re: [dpdk-dev] [PATCH] net/virtio: fix AVX512 datapath selection

[Maxime Coquelin]

On 5/12/20 4:27 AM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Monday, May 11, 2020 10:47 PM
>> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH] net/virtio: fix AVX512 datapath selection
>>
>> The AVX512 packed ring datapath selection was only done
>> at build time, but it should also be checked at runtime
>> that the CPU supports it.
>>
>> This patch add a CPU flags check so that non-vectorized
>> path is selected at runtime if AVX512 is not supported.
>>
>> Fixes: ccb10995c2ad ("net/virtio: add election for vectorized path")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index 312871cb48..49ccef12c7 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1965,8 +1965,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>  			PMD_DRV_LOG(INFO,
>>  				"building environment do not support
>> packed ring vectorized");
>>  #else
>> -			hw->use_vec_rx = 1;
>> -			hw->use_vec_tx = 1;
>> +			if
>> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
>> +				hw->use_vec_rx = 1;
>> +				hw->use_vec_tx = 1;
>> +			}
>>  #endif
> 
> Hi Maxime,
> Here is pre-setting for vectorized path selection, virtio_dev_configure will do second time check. 
> Running environment will be checked in second time check.  We can move some checks from virtio_dev_configure to here, but is it needed to do that?
> 
> BTW, both split ring and packed ring will utilized this setting, it will break split vectorized datapath is server not has AVX512F flag.
> And it may cause building issue on those platforms which not defined RTE_CPUFLAG_AVX512F.

With a bit more of context, we can see that it only affects packed ring
when CC_AVX512_SUPPORT is set. So it does break neither split ring nor
ARM/PPC:

	if (vectorized) {
		if (!vtpci_packed_queue(hw)) {
			hw->use_vec_rx = 1;
		} else {
#if !defined(CC_AVX512_SUPPORT)
			PMD_DRV_LOG(INFO,
				"building environment do not support packed ring vectorized");
#else
			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F)) {
				hw->use_vec_rx = 1;
				hw->use_vec_tx = 1;
			}
#endif
		}
	}

> Thanks,
> Marvin
> 
>>  		}
>>  	}
>> --
>> 2.25.4
>