[dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang

[dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang

[Zhihong Wang]

This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost code,
which causes the guest to hang once any packets are enqueued when mrg_rxbuf
is turned on.

How to test?

 1. Start testpmd in the host with a vhost port.

 2. Start a Windows VM image with qemu and connect to the vhost port.

 3. Start io forwarding with tx_first in host testpmd.

For 16.07 code, the Windows VM will hang once any packets are enqueued.

Cc: <stable@dpdk.org>
Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 08a73fd..5806f99 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	uint16_t start_idx = vq->last_used_idx;
 	uint16_t cur_idx = start_idx;
 	uint64_t desc_addr;
+	uint32_t desc_chain_head;
+	uint32_t desc_chain_len;
 	uint32_t mbuf_offset, mbuf_avail;
 	uint32_t desc_offset, desc_avail;
 	uint32_t cpy_len;
@@ -412,6 +414,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	desc_avail  = buf_vec[vec_idx].buf_len - dev->vhost_hlen;
 	desc_offset = dev->vhost_hlen;
+	desc_chain_head = buf_vec[vec_idx].desc_idx;
+	desc_chain_len = desc_offset;
 
 	mbuf_avail  = rte_pktmbuf_data_len(m);
 	mbuf_offset = 0;
@@ -419,19 +423,21 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		/* done with current desc buf, get the next one */
 		if (desc_avail == 0) {
 			desc_idx = buf_vec[vec_idx].desc_idx;
+			vec_idx++;
 
 			if (!(vq->desc[desc_idx].flags & VRING_DESC_F_NEXT)) {
 				/* Update used ring with desc information */
 				used_idx = cur_idx++ & (vq->size - 1);
-				vq->used->ring[used_idx].id  = desc_idx;
-				vq->used->ring[used_idx].len = desc_offset;
+				vq->used->ring[used_idx].id = desc_chain_head;
+				vq->used->ring[used_idx].len = desc_chain_len;
 				vhost_log_used_vring(dev, vq,
 					offsetof(struct vring_used,
 						 ring[used_idx]),
 					sizeof(vq->used->ring[used_idx]));
+				desc_chain_head = buf_vec[vec_idx].desc_idx;
+				desc_chain_len = 0;
 			}
 
-			vec_idx++;
 			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
 			if (unlikely(!desc_addr))
 				return 0;
@@ -463,11 +469,12 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		mbuf_offset += cpy_len;
 		desc_avail  -= cpy_len;
 		desc_offset += cpy_len;
+		desc_chain_len += cpy_len;
 	}
 
 	used_idx = cur_idx & (vq->size - 1);
-	vq->used->ring[used_idx].id = buf_vec[vec_idx].desc_idx;
-	vq->used->ring[used_idx].len = desc_offset;
+	vq->used->ring[used_idx].id = desc_chain_head;
+	vq->used->ring[used_idx].len = desc_chain_len;
 	vhost_log_used_vring(dev, vq,
 		offsetof(struct vring_used, ring[used_idx]),
 		sizeof(vq->used->ring[used_idx]));
-- 
2.7.4

Re: [dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang

[Yuanhan Liu]

On Mon, Aug 29, 2016 at 11:35:59PM -0400, Zhihong Wang wrote:
> This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost code,
> which causes the guest to hang once any packets are enqueued when mrg_rxbuf
> is turned on.

This commit log lacks two important pieces: why does the hang happen and
how does your patch fix it.

> How to test?
> 
>  1. Start testpmd in the host with a vhost port.
> 
>  2. Start a Windows VM image with qemu and connect to the vhost port.
> 
>  3. Start io forwarding with tx_first in host testpmd.
> 
> For 16.07 code, the Windows VM will hang once any packets are enqueued.
> 
> Cc: <stable@dpdk.org>
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> ---
>  lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 08a73fd..5806f99 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	uint16_t start_idx = vq->last_used_idx;
>  	uint16_t cur_idx = start_idx;
>  	uint64_t desc_addr;
> +	uint32_t desc_chain_head;
> +	uint32_t desc_chain_len;

What's the point of introducing "desc_chain_len"? It has the same value
of desc_offset.

	--yliu

Re: [dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang

[Wang, Zhihong]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, September 5, 2016 1:25 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> yuanhan.liu@linux.intel.com; thomas.monjalon@6wind.com;
> stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang
> 
> On Mon, Aug 29, 2016 at 11:35:59PM -0400, Zhihong Wang wrote:
> > This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost
> code,
> > which causes the guest to hang once any packets are enqueued when
> mrg_rxbuf
> > is turned on.
> 
> This commit log lacks two important pieces: why does the hang happen and
> how does your patch fix it.

Okay, I'll add it in v5.

> 
> > How to test?
> >
> >  1. Start testpmd in the host with a vhost port.
> >
> >  2. Start a Windows VM image with qemu and connect to the vhost port.
> >
> >  3. Start io forwarding with tx_first in host testpmd.
> >
> > For 16.07 code, the Windows VM will hang once any packets are enqueued.
> >
> > Cc: <stable@dpdk.org>
> > Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> > ---
> >  lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> > index 08a73fd..5806f99 100644
> > --- a/lib/librte_vhost/vhost_rxtx.c
> > +++ b/lib/librte_vhost/vhost_rxtx.c
> > @@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net
> *dev, struct vhost_virtqueue *vq,
> >  	uint16_t start_idx = vq->last_used_idx;
> >  	uint16_t cur_idx = start_idx;
> >  	uint64_t desc_addr;
> > +	uint32_t desc_chain_head;
> > +	uint32_t desc_chain_len;
> 
> What's the point of introducing "desc_chain_len"? It has the same value
> of desc_offset.

No it's not, desc_offset is the offset of the current desc only.
That's where the old code goes wrong.

If you take a look at the virtio spec:

/* le32 is used here for ids for padding reasons. */
struct vring_used_elem {
/* Index of start of used descriptor chain. */
le32 id;
/* Total length of the descriptor chain which was written to. */
le32 len;
};

> 
> 	--yliu

Re: [dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang

[Yuanhan Liu]

On Mon, Sep 05, 2016 at 05:25:31AM +0000, Wang, Zhihong wrote:
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Monday, September 5, 2016 1:25 PM
> > To: Wang, Zhihong <zhihong.wang@intel.com>
> > Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> > yuanhan.liu@linux.intel.com; thomas.monjalon@6wind.com;
> > stable@dpdk.org
> > Subject: Re: [dpdk-stable] [PATCH v4 1/6] vhost: fix windows vm hang
> > 
> > On Mon, Aug 29, 2016 at 11:35:59PM -0400, Zhihong Wang wrote:
> > > This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost
> > code,
> > > which causes the guest to hang once any packets are enqueued when
> > mrg_rxbuf
> > > is turned on.
> > 
> > This commit log lacks two important pieces: why does the hang happen and
> > how does your patch fix it.
> 
> Okay, I'll add it in v5.
> 
> > 
> > > How to test?
> > >
> > >  1. Start testpmd in the host with a vhost port.
> > >
> > >  2. Start a Windows VM image with qemu and connect to the vhost port.
> > >
> > >  3. Start io forwarding with tx_first in host testpmd.
> > >
> > > For 16.07 code, the Windows VM will hang once any packets are enqueued.
> > >
> > > Cc: <stable@dpdk.org>
> > > Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> > > ---
> > >  lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> > > index 08a73fd..5806f99 100644
> > > --- a/lib/librte_vhost/vhost_rxtx.c
> > > +++ b/lib/librte_vhost/vhost_rxtx.c
> > > @@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net
> > *dev, struct vhost_virtqueue *vq,
> > >  	uint16_t start_idx = vq->last_used_idx;
> > >  	uint16_t cur_idx = start_idx;
> > >  	uint64_t desc_addr;
> > > +	uint32_t desc_chain_head;
> > > +	uint32_t desc_chain_len;
> > 
> > What's the point of introducing "desc_chain_len"? It has the same value
> > of desc_offset.
> 
> No it's not, desc_offset is the offset of the current desc only.
> That's where the old code goes wrong.

Oh, right.

	--yliu

[dpdk-stable] [PATCH v5 1/6] vhost: fix windows vm hang

[Zhihong Wang]

This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost code
which causes the guest to hang once any packets are enqueued when mrg_rxbuf
is turned on by setting the right id and len in the used ring.

As defined in virtio spec 0.95 and 1.0, in each used ring element, id means
index of start of used descriptor chain, and len means total length of the
descriptor chain which was written to. While in 16.07 code, index of the
last descriptor is assigned to id, and the length of the last descriptor is
assigned to len.

How to test?

 1. Start testpmd in the host with a vhost port.

 2. Start a Windows VM image with qemu and connect to the vhost port.

 3. Start io forwarding with tx_first in host testpmd.

For 16.07 code, the Windows VM will hang once any packets are enqueued.

Cc: <stable@dpdk.org>
Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
Changes in v5:

 1. Add details in commit log.

 lib/librte_vhost/virtio_net.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 8a151af..0d6e7d9 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	uint16_t start_idx = vq->last_used_idx;
 	uint16_t cur_idx = start_idx;
 	uint64_t desc_addr;
+	uint32_t desc_chain_head;
+	uint32_t desc_chain_len;
 	uint32_t mbuf_offset, mbuf_avail;
 	uint32_t desc_offset, desc_avail;
 	uint32_t cpy_len;
@@ -412,6 +414,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	desc_avail  = buf_vec[vec_idx].buf_len - dev->vhost_hlen;
 	desc_offset = dev->vhost_hlen;
+	desc_chain_head = buf_vec[vec_idx].desc_idx;
+	desc_chain_len = desc_offset;
 
 	mbuf_avail  = rte_pktmbuf_data_len(m);
 	mbuf_offset = 0;
@@ -419,19 +423,21 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		/* done with current desc buf, get the next one */
 		if (desc_avail == 0) {
 			desc_idx = buf_vec[vec_idx].desc_idx;
+			vec_idx++;
 
 			if (!(vq->desc[desc_idx].flags & VRING_DESC_F_NEXT)) {
 				/* Update used ring with desc information */
 				used_idx = cur_idx++ & (vq->size - 1);
-				vq->used->ring[used_idx].id  = desc_idx;
-				vq->used->ring[used_idx].len = desc_offset;
+				vq->used->ring[used_idx].id = desc_chain_head;
+				vq->used->ring[used_idx].len = desc_chain_len;
 				vhost_log_used_vring(dev, vq,
 					offsetof(struct vring_used,
 						 ring[used_idx]),
 					sizeof(vq->used->ring[used_idx]));
+				desc_chain_head = buf_vec[vec_idx].desc_idx;
+				desc_chain_len = 0;
 			}
 
-			vec_idx++;
 			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
 			if (unlikely(!desc_addr))
 				return 0;
@@ -463,11 +469,12 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		mbuf_offset += cpy_len;
 		desc_avail  -= cpy_len;
 		desc_offset += cpy_len;
+		desc_chain_len += cpy_len;
 	}
 
 	used_idx = cur_idx & (vq->size - 1);
-	vq->used->ring[used_idx].id = buf_vec[vec_idx].desc_idx;
-	vq->used->ring[used_idx].len = desc_offset;
+	vq->used->ring[used_idx].id = desc_chain_head;
+	vq->used->ring[used_idx].len = desc_chain_len;
 	vhost_log_used_vring(dev, vq,
 		offsetof(struct vring_used, ring[used_idx]),
 		sizeof(vq->used->ring[used_idx]));
-- 
2.7.4

[dpdk-stable] [PATCH v6 1/6] vhost: fix windows vm hang

[Zhihong Wang]

This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost code
which causes the guest to hang once any packets are enqueued when mrg_rxbuf
is turned on by setting the right id and len in the used ring.

As defined in virtio spec 0.95 and 1.0, in each used ring element, id means
index of start of used descriptor chain, and len means total length of the
descriptor chain which was written to. While in 16.07 code, index of the
last descriptor is assigned to id, and the length of the last descriptor is
assigned to len.

How to test?

 1. Start testpmd in the host with a vhost port.

 2. Start a Windows VM image with qemu and connect to the vhost port.

 3. Start io forwarding with tx_first in host testpmd.

For 16.07 code, the Windows VM will hang once any packets are enqueued.

Cc: <stable@dpdk.org>
Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
Changes in v5:

 1. Add details in commit log.

 lib/librte_vhost/virtio_net.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 8a151af..0d6e7d9 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -384,6 +384,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	uint16_t start_idx = vq->last_used_idx;
 	uint16_t cur_idx = start_idx;
 	uint64_t desc_addr;
+	uint32_t desc_chain_head;
+	uint32_t desc_chain_len;
 	uint32_t mbuf_offset, mbuf_avail;
 	uint32_t desc_offset, desc_avail;
 	uint32_t cpy_len;
@@ -412,6 +414,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	desc_avail  = buf_vec[vec_idx].buf_len - dev->vhost_hlen;
 	desc_offset = dev->vhost_hlen;
+	desc_chain_head = buf_vec[vec_idx].desc_idx;
+	desc_chain_len = desc_offset;
 
 	mbuf_avail  = rte_pktmbuf_data_len(m);
 	mbuf_offset = 0;
@@ -419,19 +423,21 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		/* done with current desc buf, get the next one */
 		if (desc_avail == 0) {
 			desc_idx = buf_vec[vec_idx].desc_idx;
+			vec_idx++;
 
 			if (!(vq->desc[desc_idx].flags & VRING_DESC_F_NEXT)) {
 				/* Update used ring with desc information */
 				used_idx = cur_idx++ & (vq->size - 1);
-				vq->used->ring[used_idx].id  = desc_idx;
-				vq->used->ring[used_idx].len = desc_offset;
+				vq->used->ring[used_idx].id = desc_chain_head;
+				vq->used->ring[used_idx].len = desc_chain_len;
 				vhost_log_used_vring(dev, vq,
 					offsetof(struct vring_used,
 						 ring[used_idx]),
 					sizeof(vq->used->ring[used_idx]));
+				desc_chain_head = buf_vec[vec_idx].desc_idx;
+				desc_chain_len = 0;
 			}
 
-			vec_idx++;
 			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
 			if (unlikely(!desc_addr))
 				return 0;
@@ -463,11 +469,12 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		mbuf_offset += cpy_len;
 		desc_avail  -= cpy_len;
 		desc_offset += cpy_len;
+		desc_chain_len += cpy_len;
 	}
 
 	used_idx = cur_idx & (vq->size - 1);
-	vq->used->ring[used_idx].id = buf_vec[vec_idx].desc_idx;
-	vq->used->ring[used_idx].len = desc_offset;
+	vq->used->ring[used_idx].id = desc_chain_head;
+	vq->used->ring[used_idx].len = desc_chain_len;
 	vhost_log_used_vring(dev, vq,
 		offsetof(struct vring_used, ring[used_idx]),
 		sizeof(vq->used->ring[used_idx]));
-- 
2.7.4

Re: [dpdk-stable] [PATCH v6 1/6] vhost: fix windows vm hang

[Yuanhan Liu]

On Mon, Sep 19, 2016 at 10:00:12PM -0400, Zhihong Wang wrote:
> This patch fixes a Windows VM compatibility issue in DPDK 16.07 vhost code
> which causes the guest to hang once any packets are enqueued when mrg_rxbuf
> is turned on by setting the right id and len in the used ring.
> 
> As defined in virtio spec 0.95 and 1.0, in each used ring element, id means
> index of start of used descriptor chain, and len means total length of the
> descriptor chain which was written to. While in 16.07 code, index of the
> last descriptor is assigned to id, and the length of the last descriptor is
> assigned to len.
> 
> How to test?
> 
>  1. Start testpmd in the host with a vhost port.
> 
>  2. Start a Windows VM image with qemu and connect to the vhost port.
> 
>  3. Start io forwarding with tx_first in host testpmd.
> 
> For 16.07 code, the Windows VM will hang once any packets are enqueued.
> 
> Cc: <stable@dpdk.org>
> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>

Applied to dpdk-next-virtio (this patch only).

Thanks.

	--yliu