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

["Liu, Yong" <yong.liu@intel.com>]

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