diff mbox series

net/virtio: fix indirect descriptors reconnection

Message ID 20210819053518.106296-1-xuan.ding@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: fix indirect descriptors reconnection |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing fail Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Ding, Xuan Aug. 19, 2021, 5:35 a.m. UTC 
  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(+)

Comments

Maxime Coquelin Sept. 23, 2021, 8:15 a.m. UTC | #1 
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.

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

> +		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
Ding, Xuan Sept. 23, 2021, 8:35 a.m. UTC | #2 
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.

> 
> > +
> > +		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
Ding, Xuan Sept. 23, 2021, 9:21 a.m. UTC | #3 
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
Maxime Coquelin Oct. 12, 2021, 8:48 a.m. UTC | #4 
On 9/23/21 11:21, Ding, Xuan wrote:
> 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 for looking at this, but maybe that could be done later, after
having the fix merged first if you don't have the bandwidth?

Maxime
> 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
>
Wang, Yinan Oct. 13, 2021, 12:28 a.m. UTC | #5 
Tested-by: Yinan Wang <yinan.wang@intel.com>

> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: 2021?8?19? 13:35
> To: 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>; Ding, Xuan <xuan.ding@intel.com>;
> stable@dpdk.org; Liu, Yong <yong.liu@intel.com>
> Subject: [PATCH] net/virtio: fix indirect descriptors reconnection
> 
> 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>
diff mbox series

Patch

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;
 		}
+
+		struct virtio_tx_region *txr;
+		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;
+		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);