[v2] vhost: fix packed ring zero-copy
Checks
Commit Message
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>
Comments
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.
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
@@ -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);