Hi Maxime,
> -----Original Message-----
> From: Ding, Xuan
> Sent: Thursday, September 23, 2021 4:36 PM
> To: Maxime Coquelin <maxime.coque...@redhat.com>; Xia, Chenbo
> <chenbo....@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu...@intel.com>; Wang, Yinan
> <yinan.w...@intel.com>; sta...@dpdk.org; Liu, Yong <yong....@intel.com>
> Subject: RE: [PATCH] net/virtio: fix indirect descriptors reconnection
>
> Hi Maxime,
>
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coque...@redhat.com>
> > Sent: Thursday, September 23, 2021 4:16 PM
> > To: Ding, Xuan <xuan.d...@intel.com>; Xia, Chenbo <chenbo....@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu...@intel.com>; Wang, Yinan
> > <yinan.w...@intel.com>; sta...@dpdk.org; Liu, Yong <yong....@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: sta...@dpdk.org
> > > Cc: yong....@intel.com
> > >
> > > Signed-off-by: Xuan Ding <xuan.d...@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
