diff mbox series

[v2] vhost: fix packed ring zero-copy

Message ID 20200224151419.85565-1-yong.liu@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series [v2] vhost: fix packed ring zero-copy |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed

Commit Message

Marvin Liu Feb. 24, 2020, 3:14 p.m. UTC 
  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

Maxime Coquelin Feb. 24, 2020, 8:28 a.m. UTC | #1 
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
David Marchand Feb. 24, 2020, 10:32 a.m. UTC | #2 
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.
Maxime Coquelin Feb. 24, 2020, 11:14 a.m. UTC | #3 
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
diff mbox series

Patch

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