Re: [dpdk-dev] [PATCH] net/virtio: fix indirect descriptors reconnection

["Ding, Xuan" <xuan.ding@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Ding, Xuan
> Sent: Thursday, September 23, 2021 4:36 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <Chenbo.Xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <Jiayu.Hu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; stable@dpdk.org; Liu, Yong <yong.liu@intel.com>
> Subject: RE: [PATCH] net/virtio: fix indirect descriptors reconnection
> 
> Hi Maxime,
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Thursday, September 23, 2021 4:16 PM
> > To: Ding, Xuan <xuan.ding@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Wang, Yinan
> > <yinan.wang@intel.com>; stable@dpdk.org; Liu, Yong <yong.liu@intel.com>
> > Subject: Re: [PATCH] net/virtio: fix indirect descriptors reconnection
> >
> > Hi Xuan,
> >
> > On 8/19/21 07:35, Xuan Ding wrote:
> > > Since packed indirect descriptors are added and initialized when
> > > initializing vring, the reconnection path also needs to be considered.
> > >
> > > Fixes: 381f39ebb78a ("net/virtio: fix packed ring indirect descricptors setup")
> > > Cc: stable@dpdk.org
> > > Cc: yong.liu@intel.com
> > >
> > > Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > > ---
> > >   drivers/net/virtio/virtqueue.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> > > index 1f9af3c31b..47229f35c4 100644
> > > --- a/drivers/net/virtio/virtqueue.c
> > > +++ b/drivers/net/virtio/virtqueue.c
> > > @@ -208,6 +208,20 @@ virtqueue_txvq_reset_packed(struct virtqueue *vq)
> > >   			rte_pktmbuf_free(dxp->cookie);
> > >   			dxp->cookie = NULL;
> > >   		}
> >
> > It makes me think we might save quite some bytes by not allocating
> > indirect descriptors when feature is not negotiated, but it might have
> > a cost in term of performance.
> 
> Do you mean we allocate the indirect descriptors after checking the indirect
> feature in virtio TX queue,
> instead of allocating it at initialization?
> If so, that makes sense, and hence we don't to initialize it in two places.

Sorry for replying myself, as for performance, I can have a try and test the
impact on performance.

Thanks,
Xuan

> 
> >
> > > +
> > > +		struct virtio_tx_region *txr;
> >
> > Don't mix declarations within code.
> >
> > > +		txr = txvq->virtio_net_hdr_mz->addr;
> > > +		/* first indirect descriptor is always the tx header */
> > > +		struct vring_packed_desc *start_dp =
> > > +			txr[desc_idx].tx_packed_indir;
> >
> > Ditto.
> 
> Thanks, will update in next version.
> 
> Regards,
> Xuan
> 
> >
> > > +		vring_desc_init_indirect_packed(start_dp,
> > > +		      RTE_DIM(txr[desc_idx].tx_packed_indir));
> > > +		start_dp->addr = txvq->virtio_net_hdr_mem
> > > +			+ desc_idx * sizeof(*txr)
> > > +			+ offsetof(struct virtio_tx_region,
> > > +				   tx_hdr);
> > > +		start_dp->len = vq->hw->vtnet_hdr_size;
> > > +
> > >   	}
> > >
> > >   	vring_desc_init_packed(vq, size);
> > >
> >
> > Thanks,
> > Maxime