DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid
  2020-02-24 14:50 [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu
@ 2020-02-24  7:25 ` Ye Xiaolong
  2020-02-24  7:39   ` Liu, Yong
  2020-02-24  7:27 ` Maxime Coquelin
  2020-02-24 15:14 ` [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy Marvin Liu
  2 siblings, 1 reply; 8+ messages in thread
From: Ye Xiaolong @ 2020-02-24  7:25 UTC (permalink / raw)
  To: Marvin Liu; +Cc: maxime.coquelin, dev, tiwei.bie, zhihong.wang, stable

For the subject, what about:

vhost: fix invalid zmbuf buffer id

On 02/24, Marvin Liu wrote:
>zc mbufs should record available buffer id when doing dequeue zcopy.
>There's no guarantee that local queue avail index equal to buffer index.

s/equal to/is equal to

>
>Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
>Cc: stable@dpdk.org
>
>Signed-off-by: Marvin Liu <yong.liu@intel.com>
>Reported-by: Yinan Wang <yinan.wang@intel.com>
>
>diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>index 37c47c7dc..210415904 100644
>--- a/lib/librte_vhost/virtio_net.c
>+++ b/lib/librte_vhost/virtio_net.c
>@@ -2004,7 +2004,7 @@ virtio_dev_tx_batch_packed_zmbuf(struct virtio_net *dev,
> 
> 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
> 		zmbufs[i]->mbuf = pkts[i];
>-		zmbufs[i]->desc_idx = avail_idx + i;
>+		zmbufs[i]->desc_idx = ids[i];
> 		zmbufs[i]->desc_count = 1;
> 	}
> 
>@@ -2045,7 +2045,7 @@ virtio_dev_tx_single_packed_zmbuf(struct virtio_net *dev,
> 		return -1;
> 	}
> 	zmbuf->mbuf = *pkts;
>-	zmbuf->desc_idx = vq->last_avail_idx;
>+	zmbuf->desc_idx = buf_id;
> 	zmbuf->desc_count = desc_count;
> 
> 	rte_mbuf_refcnt_update(*pkts, 1);
>-- 
>2.17.1
>

Apart from above,

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid
  2020-02-24 14:50 [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu
  2020-02-24  7:25 ` Ye Xiaolong
@ 2020-02-24  7:27 ` Maxime Coquelin
  2020-02-24 15:14 ` [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy Marvin Liu
  2 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-02-24  7:27 UTC (permalink / raw)
  To: Marvin Liu; +Cc: dev, tiwei.bie, zhihong.wang, stable

I would make the title a bit more generic, e.g.:

vhost: fix packed ring zero-copy

On 2/24/20 3:50 PM, Marvin Liu wrote:
> zc mbufs should record available buffer id when doing dequeue zcopy.

Available buffer ID should be stored in the mbuf in the packed-ring
dequeue path.

> There's no guarantee that local queue avail index equal to buffer index.
> 
> Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> Reported-by: Yinan Wang <yinan.wang@intel.com>
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 37c47c7dc..210415904 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -2004,7 +2004,7 @@ virtio_dev_tx_batch_packed_zmbuf(struct virtio_net *dev,
>  
>  	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
>  		zmbufs[i]->mbuf = pkts[i];
> -		zmbufs[i]->desc_idx = avail_idx + i;
> +		zmbufs[i]->desc_idx = ids[i];
>  		zmbufs[i]->desc_count = 1;
>  	}
>  
> @@ -2045,7 +2045,7 @@ virtio_dev_tx_single_packed_zmbuf(struct virtio_net *dev,
>  		return -1;
>  	}
>  	zmbuf->mbuf = *pkts;
> -	zmbuf->desc_idx = vq->last_avail_idx;
> +	zmbuf->desc_idx = buf_id;
>  	zmbuf->desc_count = desc_count;
>  
>  	rte_mbuf_refcnt_update(*pkts, 1);
> 

With the commit message fixed:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid
  2020-02-24  7:25 ` Ye Xiaolong
@ 2020-02-24  7:39   ` Liu, Yong
  0 siblings, 0 replies; 8+ messages in thread
From: Liu, Yong @ 2020-02-24  7:39 UTC (permalink / raw)
  To: Ye, Xiaolong, maxime.coquelin; +Cc: dev, Bie, Tiwei, Wang, Zhihong, stable

Thanks, xiaolong & maxime. Commits log has been fixed in v2.

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Monday, February 24, 2020 3:26 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: maxime.coquelin@redhat.com; dev@dpdk.org; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid
> 
> For the subject, what about:
> 
> vhost: fix invalid zmbuf buffer id
> 
> On 02/24, Marvin Liu wrote:
> >zc mbufs should record available buffer id when doing dequeue zcopy.
> >There's no guarantee that local queue avail index equal to buffer index.
> 
> s/equal to/is equal to
> 
> >
> >Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single
> dequeue")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >Reported-by: Yinan Wang <yinan.wang@intel.com>
> >
> >diff --git a/lib/librte_vhost/virtio_net.c
> b/lib/librte_vhost/virtio_net.c
> >index 37c47c7dc..210415904 100644
> >--- a/lib/librte_vhost/virtio_net.c
> >+++ b/lib/librte_vhost/virtio_net.c
> >@@ -2004,7 +2004,7 @@ virtio_dev_tx_batch_packed_zmbuf(struct virtio_net
> *dev,
> >
> > 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
> > 		zmbufs[i]->mbuf = pkts[i];
> >-		zmbufs[i]->desc_idx = avail_idx + i;
> >+		zmbufs[i]->desc_idx = ids[i];
> > 		zmbufs[i]->desc_count = 1;
> > 	}
> >
> >@@ -2045,7 +2045,7 @@ virtio_dev_tx_single_packed_zmbuf(struct
> virtio_net *dev,
> > 		return -1;
> > 	}
> > 	zmbuf->mbuf = *pkts;
> >-	zmbuf->desc_idx = vq->last_avail_idx;
> >+	zmbuf->desc_idx = buf_id;
> > 	zmbuf->desc_count = desc_count;
> >
> > 	rte_mbuf_refcnt_update(*pkts, 1);
> >--
> >2.17.1
> >
> 
> Apart from above,
> 
> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy
  2020-02-24 15:14 ` [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy Marvin Liu
@ 2020-02-24  8:28   ` Maxime Coquelin
  2020-02-24 10:32     ` David Marchand
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-02-24  8:28 UTC (permalink / raw)
  To: Marvin Liu, tiwei.bie, zhihong.wang, Thomas Monjalon, David Marchand
  Cc: dev, stable

Hi David & Thomas,

On 2/24/20 4:14 PM, Marvin Liu wrote:
> Available buffer ID should be stored in the zmbuf in the packed-ring
> dequeue path. There's no guarantee that local queue avail index is
> equal to buffer ID.
> 
> Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> Reported-by: Yinan Wang <yinan.wang@intel.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 

If it is not too late, I think we should pick this patch for
v20.02. It is fixing a regression introduced in DPDK v19.11.

The risk of this patch causing a regression is very low, as it is
touching only the zero-copy packed ring dequeue path, which wihtout this
patch is anyway broken.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy
  2020-02-24  8:28   ` Maxime Coquelin
@ 2020-02-24 10:32     ` David Marchand
  2020-02-24 11:14       ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2020-02-24 10:32 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: Marvin Liu, Tiwei Bie, Zhihong Wang, Thomas Monjalon, dev, dpdk stable

On Mon, Feb 24, 2020 at 9:28 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Hi David & Thomas,
>
> On 2/24/20 4:14 PM, Marvin Liu wrote:
> > Available buffer ID should be stored in the zmbuf in the packed-ring
> > dequeue path. There's no guarantee that local queue avail index is
> > equal to buffer ID.
> >
> > Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> > Reported-by: Yinan Wang <yinan.wang@intel.com>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
>
> If it is not too late, I think we should pick this patch for
> v20.02. It is fixing a regression introduced in DPDK v19.11.

I might have cold feet, but taking this fix right now feels risky.
If the problem has been there since 19.11, it can wait 20.05 and it
will go to 19.11 after proper validation.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy
  2020-02-24 10:32     ` David Marchand
@ 2020-02-24 11:14       ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-02-24 11:14 UTC (permalink / raw)
  To: David Marchand
  Cc: Marvin Liu, Tiwei Bie, Zhihong Wang, Thomas Monjalon, dev, dpdk stable



On 2/24/20 11:32 AM, David Marchand wrote:
> On Mon, Feb 24, 2020 at 9:28 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> Hi David & Thomas,
>>
>> On 2/24/20 4:14 PM, Marvin Liu wrote:
>>> Available buffer ID should be stored in the zmbuf in the packed-ring
>>> dequeue path. There's no guarantee that local queue avail index is
>>> equal to buffer ID.
>>>
>>> Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>>> Reported-by: Yinan Wang <yinan.wang@intel.com>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>
>> If it is not too late, I think we should pick this patch for
>> v20.02. It is fixing a regression introduced in DPDK v19.11.
> 
> I might have cold feet, but taking this fix right now feels risky.
> If the problem has been there since 19.11, it can wait 20.05 and it
> will go to 19.11 after proper validation.

Ok, I get your point and it's your call.  Now, this fix is really
isolated to zero-copy packed ring, so the only risk IMO is to break
something that is already not working.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid
@ 2020-02-24 14:50 Marvin Liu
  2020-02-24  7:25 ` Ye Xiaolong
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Marvin Liu @ 2020-02-24 14:50 UTC (permalink / raw)
  To: maxime.coquelin; +Cc: dev, tiwei.bie, zhihong.wang, Marvin Liu, stable

zc mbufs should record available buffer id when doing dequeue zcopy.
There's no guarantee that local queue avail index equal to buffer index.

Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
Cc: stable@dpdk.org

Signed-off-by: Marvin Liu <yong.liu@intel.com>
Reported-by: Yinan Wang <yinan.wang@intel.com>

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 37c47c7dc..210415904 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -2004,7 +2004,7 @@ virtio_dev_tx_batch_packed_zmbuf(struct virtio_net *dev,
 
 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
 		zmbufs[i]->mbuf = pkts[i];
-		zmbufs[i]->desc_idx = avail_idx + i;
+		zmbufs[i]->desc_idx = ids[i];
 		zmbufs[i]->desc_count = 1;
 	}
 
@@ -2045,7 +2045,7 @@ virtio_dev_tx_single_packed_zmbuf(struct virtio_net *dev,
 		return -1;
 	}
 	zmbuf->mbuf = *pkts;
-	zmbuf->desc_idx = vq->last_avail_idx;
+	zmbuf->desc_idx = buf_id;
 	zmbuf->desc_count = desc_count;
 
 	rte_mbuf_refcnt_update(*pkts, 1);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy
  2020-02-24 14:50 [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu
  2020-02-24  7:25 ` Ye Xiaolong
  2020-02-24  7:27 ` Maxime Coquelin
@ 2020-02-24 15:14 ` Marvin Liu
  2020-02-24  8:28   ` Maxime Coquelin
  2 siblings, 1 reply; 8+ messages in thread
From: Marvin Liu @ 2020-02-24 15:14 UTC (permalink / raw)
  To: maxime.coquelin, tiwei.bie, zhihong.wang; +Cc: dev, stable, Marvin Liu

Available buffer ID should be stored in the zmbuf in the packed-ring
dequeue path. There's no guarantee that local queue avail index is
equal to buffer ID.

Fixes: d1eafb532268 ("vhost: add packed ring zcopy batch and single dequeue")
Cc: stable@dpdk.org

Signed-off-by: Marvin Liu <yong.liu@intel.com>
Reported-by: Yinan Wang <yinan.wang@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 37c47c7dc..210415904 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -2004,7 +2004,7 @@ virtio_dev_tx_batch_packed_zmbuf(struct virtio_net *dev,
 
 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
 		zmbufs[i]->mbuf = pkts[i];
-		zmbufs[i]->desc_idx = avail_idx + i;
+		zmbufs[i]->desc_idx = ids[i];
 		zmbufs[i]->desc_count = 1;
 	}
 
@@ -2045,7 +2045,7 @@ virtio_dev_tx_single_packed_zmbuf(struct virtio_net *dev,
 		return -1;
 	}
 	zmbuf->mbuf = *pkts;
-	zmbuf->desc_idx = vq->last_avail_idx;
+	zmbuf->desc_idx = buf_id;
 	zmbuf->desc_count = desc_count;
 
 	rte_mbuf_refcnt_update(*pkts, 1);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-02-24 11:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24 14:50 [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu
2020-02-24  7:25 ` Ye Xiaolong
2020-02-24  7:39   ` Liu, Yong
2020-02-24  7:27 ` Maxime Coquelin
2020-02-24 15:14 ` [dpdk-dev] [PATCH v2] vhost: fix packed ring zero-copy Marvin Liu
2020-02-24  8:28   ` Maxime Coquelin
2020-02-24 10:32     ` David Marchand
2020-02-24 11:14       ` Maxime Coquelin

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git