Re: [dpdk-dev] [PATCH v9 5/9] net/virtio: add vectorized packed ring Rx path

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

> -----Original Message-----
> From: Liu, Yong
> Sent: Friday, April 24, 2020 9:41 PM
> To: 'Maxime Coquelin' <maxime.coquelin@redhat.com>; Ye, Xiaolong
> <xiaolong.ye@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: RE: [PATCH v9 5/9] net/virtio: add vectorized packed ring Rx path
> 
> 
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Friday, April 24, 2020 9:34 PM
> > To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> > Wang, Zhihong <zhihong.wang@intel.com>
> > Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> > Subject: Re: [PATCH v9 5/9] net/virtio: add vectorized packed ring Rx path
> >
> >
> >
> > On 4/24/20 3:12 PM, Liu, Yong wrote:
> > >> IIUC, the only difference with the non-vectorized version is the GSO
> > >> support removed here.
> > >> gso_type being in the same cacheline as flags in virtio_net_hdr, I don't
> > >> think checking the performance gain is worth the added maintainance
> > >> effort due to code duplication.
> > >>
> > >> Please prove I'm wrong, otherwise please move virtio_rx_offload() in a
> > >> header and use it here. Alternative if it really imapcts performance is
> > >> to put all the shared code in a dedicated function that can be re-used
> > >> by both implementations.
> > >>
> > > Maxime,
> > > It won't be much performance difference between non-vectorized and
> > vectorized.
> > > The reason to add special vectorized version is for skipping the handling
> of
> > garbage GSO packets.
> > > As all descs have been handled in batch, it is needed to revert when
> found
> > garbage packets.
> > > That will introduce complicated logic in vectorized path.
> >
> 
> Dequeue function will call virtio_discard_rxbuf when found gso info in hdr is
> invalid.
> IMHO, there's no need to check gso info when GSO not negotiated.
> There's an alternative way is that use single function handle GSO packets but
> its performance will be worse than normal function.
> 
> if ((hdr->gso_type & VIRTIO_NET_HDR_GSO_ECN) ||
> 	 (hdr->gso_size == 0)) {
> 	 return -EINVAL;
> }
> 

Hi Maxime,
There's about 6% performance drop in loopback case after handling this special case in Rx path.
I prefer to keep current implementation. What's your option?

Thanks,
Marvin

> >
> > What do you mean by garbage packet?
> > Is it really good to just ignore such issues?
> >
> > Thanks,
> > Maxime
> >
> > > Regards,
> > > Marvin
> > >