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

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

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