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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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
>