diff mbox series

net/virtio: fix AVX512 datapath selection

Message ID 20200511144720.241224-1-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: fix AVX512 datapath selection |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/Intel-compilation fail Compilation issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Maxime Coquelin May 11, 2020, 2:47 p.m. UTC 
  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(-)

Comments

Ferruh Yigit May 11, 2020, 3:11 p.m. UTC | #1 
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.
Marvin Liu May 12, 2020, 2:27 a.m. UTC | #2 
> -----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
Maxime Coquelin May 12, 2020, 8:35 a.m. UTC | #3 
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
>
diff mbox series

Patch

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
 		}
 	}