* [dpdk-stable] [PATCH] vhost: fix zmbuf buffer id invalid @ 2020-02-24 14:50 Marvin Liu 2020-02-24 7:25 ` [dpdk-stable] [dpdk-dev] " 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
* Re: [dpdk-stable] [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid 2020-02-24 14:50 [dpdk-stable] [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 ` [dpdk-stable] " Maxime Coquelin 2020-02-24 15:14 ` [dpdk-stable] [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-stable] [dpdk-dev] [PATCH] vhost: fix zmbuf buffer id invalid 2020-02-24 7:25 ` [dpdk-stable] [dpdk-dev] " 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-stable] [PATCH] vhost: fix zmbuf buffer id invalid 2020-02-24 14:50 [dpdk-stable] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu 2020-02-24 7:25 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong @ 2020-02-24 7:27 ` Maxime Coquelin 2020-02-24 15:14 ` [dpdk-stable] [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
* [dpdk-stable] [PATCH v2] vhost: fix packed ring zero-copy 2020-02-24 14:50 [dpdk-stable] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu 2020-02-24 7:25 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong 2020-02-24 7:27 ` [dpdk-stable] " 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
* Re: [dpdk-stable] [PATCH v2] vhost: fix packed ring zero-copy 2020-02-24 15:14 ` [dpdk-stable] [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-stable] [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-stable] [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
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-stable] [PATCH] vhost: fix zmbuf buffer id invalid Marvin Liu 2020-02-24 7:25 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong 2020-02-24 7:39 ` Liu, Yong 2020-02-24 7:27 ` [dpdk-stable] " Maxime Coquelin 2020-02-24 15:14 ` [dpdk-stable] [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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).