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