[dpdk-dev] [PATCH] vhost: fix dequeue zero copy

[dpdk-dev] [PATCH] vhost: fix dequeue zero copy

[Yuanhan Liu]

For zero copy mode, we need pin the mbuf to not let the underlaying PMD
driver (or the app) free the mbuf. Currently, only the heading mbuf is
pinned. However, the mbuf free function would try to free all mbufs
in the mbuf chain (-1 to the refcnt). This may lead the head mbuf being
still pinned, while the other subsequent mbufs are actually freed. Which
is wrong.

It becomes more fatal after the mbuf refactor, more specificly, after
the commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool"). The
refcnt resets to 1 after the last real reference. OTOH, it leads to a
situtation that we never know one mbuf is actually freed or not. This
would result the mbuf __just__ after the heading mbuf being freed twice:
it's firstly freed (and put back to mempool) when the underlaying PMD
finishes the DMA.  Later, it will then be freed again when vhost unpins
it. Meaning, one mbuf may be returned to the mempool twice, while in
turn, being allocated twice later. Something uncertain may happen then.
For example, the VM2VM case becomes broken.

Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index b9f2168..d3d4424 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -871,6 +871,8 @@ static inline int __attribute__((always_inline))
 					"allocate memory for mbuf.\n");
 				return -1;
 			}
+			if (unlikely(dev->dequeue_zero_copy))
+				rte_mbuf_refcnt_update(cur, 1);
 
 			prev->next = cur;
 			prev->data_len = mbuf_offset;
-- 
1.9.0

Re: [dpdk-dev] [PATCH] vhost: fix dequeue zero copy

[Yuanhan Liu]

On Wed, Apr 19, 2017 at 01:26:01PM +0800, Yuanhan Liu wrote:
> For zero copy mode, we need pin the mbuf to not let the underlaying PMD
> driver (or the app) free the mbuf. Currently, only the heading mbuf is
> pinned. However, the mbuf free function would try to free all mbufs
> in the mbuf chain (-1 to the refcnt). This may lead the head mbuf being
> still pinned, while the other subsequent mbufs are actually freed. Which
> is wrong.
> 
> It becomes more fatal after the mbuf refactor, more specificly, after
> the commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool"). The
> refcnt resets to 1 after the last real reference. OTOH, it leads to a
> situtation that we never know one mbuf is actually freed or not. This
> would result the mbuf __just__ after the heading mbuf being freed twice:
> it's firstly freed (and put back to mempool) when the underlaying PMD
> finishes the DMA.  Later, it will then be freed again when vhost unpins
> it. Meaning, one mbuf may be returned to the mempool twice, while in
> turn, being allocated twice later. Something uncertain may happen then.
> For example, the VM2VM case becomes broken.
> 
> Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
> 
> Cc: stable@dpdk.org
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Applied to dpdk-next-virtio.

	--yliu