[dpdk-dev] [PATCH] vhost: fix packed ring descriptor update in async enqueue

[dpdk-dev] [PATCH] vhost: fix packed ring descriptor update in async enqueue

[Jiayu Hu]

For the packet using multiple descriptors, the flag of first
descriptor cannot be updated until finishing updating the flag
of remaining descriptors. However, if the packet's descriptor
index is wrapped, the first descriptor's flag is updated earlier
than the others.

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 68 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index cef4bcf15c..b3d954aab4 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1549,60 +1549,6 @@ virtio_dev_rx_async_submit_split(struct virtio_net *dev,
 	return pkt_idx;
 }
 
-static __rte_always_inline void
-vhost_update_used_packed(struct vhost_virtqueue *vq,
-			struct vring_used_elem_packed *shadow_ring,
-			uint16_t count)
-{
-	int i;
-	uint16_t used_idx = vq->last_used_idx;
-	uint16_t head_idx = vq->last_used_idx;
-	uint16_t head_flags = 0;
-
-	if (count == 0)
-		return;
-
-	/* Split loop in two to save memory barriers */
-	for (i = 0; i < count; i++) {
-		vq->desc_packed[used_idx].id = shadow_ring[i].id;
-		vq->desc_packed[used_idx].len = shadow_ring[i].len;
-
-		used_idx += shadow_ring[i].count;
-		if (used_idx >= vq->size)
-			used_idx -= vq->size;
-	}
-
-	/* The ordering for storing desc flags needs to be enforced. */
-	rte_atomic_thread_fence(__ATOMIC_RELEASE);
-
-	for (i = 0; i < count; i++) {
-		uint16_t flags;
-
-		if (vq->shadow_used_packed[i].len)
-			flags = VRING_DESC_F_WRITE;
-		else
-			flags = 0;
-
-		if (vq->used_wrap_counter) {
-			flags |= VRING_DESC_F_USED;
-			flags |= VRING_DESC_F_AVAIL;
-		} else {
-			flags &= ~VRING_DESC_F_USED;
-			flags &= ~VRING_DESC_F_AVAIL;
-		}
-
-		if (i > 0) {
-			vq->desc_packed[vq->last_used_idx].flags = flags;
-		} else {
-			head_idx = vq->last_used_idx;
-			head_flags = flags;
-		}
-
-		vq_inc_last_used_packed(vq, shadow_ring[i].count);
-	}
-
-	vq->desc_packed[head_idx].flags = head_flags;
-}
 
 static __rte_always_inline int
 vhost_enqueue_async_packed(struct virtio_net *dev,
@@ -1819,23 +1765,63 @@ write_back_completed_descs_packed(struct vhost_virtqueue *vq,
 				uint16_t n_buffers)
 {
 	struct vhost_async *async = vq->async;
-	uint16_t nr_left = n_buffers;
-	uint16_t from, to;
+	uint16_t from = async->last_buffer_idx_packed;
+	uint16_t used_idx = vq->last_used_idx;
+	uint16_t head_idx = vq->last_used_idx;
+	uint16_t head_flags = 0;
+	uint16_t i;
 
-	do {
-		from = async->last_buffer_idx_packed;
-		to = (from + nr_left) % vq->size;
-		if (to > from) {
-			vhost_update_used_packed(vq, async->buffers_packed + from, to - from);
-			async->last_buffer_idx_packed += nr_left;
-			nr_left = 0;
+	/* Split loop in two to save memory barriers */
+	for (i = 0; i < n_buffers; i++) {
+		vq->desc_packed[used_idx].id = async->buffers_packed[from].id;
+		vq->desc_packed[used_idx].len = async->buffers_packed[from].len;
+
+		used_idx += async->buffers_packed[from].count;
+		if (used_idx >= vq->size)
+			used_idx -= vq->size;
+
+		from++;
+		if (from >= vq->size)
+			from = 0;
+	}
+
+	/* The ordering for storing desc flags needs to be enforced. */
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
+
+	from = async->last_buffer_idx_packed;
+
+	for (i = 0; i < n_buffers; i++) {
+		uint16_t flags;
+
+		if (async->buffers_packed[from].len)
+			flags = VRING_DESC_F_WRITE;
+		else
+			flags = 0;
+
+		if (vq->used_wrap_counter) {
+			flags |= VRING_DESC_F_USED;
+			flags |= VRING_DESC_F_AVAIL;
 		} else {
-			vhost_update_used_packed(vq, async->buffers_packed + from,
-				vq->size - from);
-			async->last_buffer_idx_packed = 0;
-			nr_left -= vq->size - from;
+			flags &= ~VRING_DESC_F_USED;
+			flags &= ~VRING_DESC_F_AVAIL;
 		}
-	} while (nr_left > 0);
+
+		if (i > 0) {
+			vq->desc_packed[vq->last_used_idx].flags = flags;
+		} else {
+			head_idx = vq->last_used_idx;
+			head_flags = flags;
+		}
+
+		vq_inc_last_used_packed(vq, async->buffers_packed[from].count);
+
+		from++;
+		if (from == vq->size)
+			from = 0;
+	}
+
+	vq->desc_packed[head_idx].flags = head_flags;
+	async->last_buffer_idx_packed = from;
 }
 
 static __rte_always_inline uint16_t
-- 
2.25.1

Re: [dpdk-dev] [PATCH] vhost: fix packed ring descriptor update in async enqueue

[Maxime Coquelin]

On 11/4/21 11:19, Jiayu Hu wrote:
> For the packet using multiple descriptors, the flag of first
> descriptor cannot be updated until finishing updating the flag
> of remaining descriptors. However, if the packet's descriptor
> index is wrapped, the first descriptor's flag is updated earlier
> than the others.

The commit message is ambiguous IMHO, it is not clear whether it
describes the bug or the fix. I understand it describes the problem,
as the first descriptor flags always have to be updated last, but it
please reword it to make it clear what iu the issue, and how this patch
fixes it.

> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> Cc: stable@dpdk.org

If you think it is worth backporting it to stable, please take care of
preparing the backport as I suspect it will not apply cleanly.

> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>   lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
>   1 file changed, 54 insertions(+), 68 deletions(-)
> 

[dpdk-dev] [PATCH v2] vhost: fix packed ring descriptor update in async enqueue

[Jiayu Hu]

If the packet uses multiple descrptors and its descriptor indices are
wrapped, the first descriptor flag is not updated last, which may cause
virtio read the incomplete packet. For example, given a packet uses 64
descriptors, and virtio ring size is 256, and its descriptor indices is
224~255 and 0~31, current implementation will update 224~255 descriptor
flags earlier than 0~31 descriptor flags.

This patch fixes this issue by updating descriptor flags in one loop,
so that the first descriptor flag is always updated last.

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
v2:
* update commit log
---
 lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 68 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index cef4bcf15c..b3d954aab4 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1549,60 +1549,6 @@ virtio_dev_rx_async_submit_split(struct virtio_net *dev,
 	return pkt_idx;
 }
 
-static __rte_always_inline void
-vhost_update_used_packed(struct vhost_virtqueue *vq,
-			struct vring_used_elem_packed *shadow_ring,
-			uint16_t count)
-{
-	int i;
-	uint16_t used_idx = vq->last_used_idx;
-	uint16_t head_idx = vq->last_used_idx;
-	uint16_t head_flags = 0;
-
-	if (count == 0)
-		return;
-
-	/* Split loop in two to save memory barriers */
-	for (i = 0; i < count; i++) {
-		vq->desc_packed[used_idx].id = shadow_ring[i].id;
-		vq->desc_packed[used_idx].len = shadow_ring[i].len;
-
-		used_idx += shadow_ring[i].count;
-		if (used_idx >= vq->size)
-			used_idx -= vq->size;
-	}
-
-	/* The ordering for storing desc flags needs to be enforced. */
-	rte_atomic_thread_fence(__ATOMIC_RELEASE);
-
-	for (i = 0; i < count; i++) {
-		uint16_t flags;
-
-		if (vq->shadow_used_packed[i].len)
-			flags = VRING_DESC_F_WRITE;
-		else
-			flags = 0;
-
-		if (vq->used_wrap_counter) {
-			flags |= VRING_DESC_F_USED;
-			flags |= VRING_DESC_F_AVAIL;
-		} else {
-			flags &= ~VRING_DESC_F_USED;
-			flags &= ~VRING_DESC_F_AVAIL;
-		}
-
-		if (i > 0) {
-			vq->desc_packed[vq->last_used_idx].flags = flags;
-		} else {
-			head_idx = vq->last_used_idx;
-			head_flags = flags;
-		}
-
-		vq_inc_last_used_packed(vq, shadow_ring[i].count);
-	}
-
-	vq->desc_packed[head_idx].flags = head_flags;
-}
 
 static __rte_always_inline int
 vhost_enqueue_async_packed(struct virtio_net *dev,
@@ -1819,23 +1765,63 @@ write_back_completed_descs_packed(struct vhost_virtqueue *vq,
 				uint16_t n_buffers)
 {
 	struct vhost_async *async = vq->async;
-	uint16_t nr_left = n_buffers;
-	uint16_t from, to;
+	uint16_t from = async->last_buffer_idx_packed;
+	uint16_t used_idx = vq->last_used_idx;
+	uint16_t head_idx = vq->last_used_idx;
+	uint16_t head_flags = 0;
+	uint16_t i;
 
-	do {
-		from = async->last_buffer_idx_packed;
-		to = (from + nr_left) % vq->size;
-		if (to > from) {
-			vhost_update_used_packed(vq, async->buffers_packed + from, to - from);
-			async->last_buffer_idx_packed += nr_left;
-			nr_left = 0;
+	/* Split loop in two to save memory barriers */
+	for (i = 0; i < n_buffers; i++) {
+		vq->desc_packed[used_idx].id = async->buffers_packed[from].id;
+		vq->desc_packed[used_idx].len = async->buffers_packed[from].len;
+
+		used_idx += async->buffers_packed[from].count;
+		if (used_idx >= vq->size)
+			used_idx -= vq->size;
+
+		from++;
+		if (from >= vq->size)
+			from = 0;
+	}
+
+	/* The ordering for storing desc flags needs to be enforced. */
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
+
+	from = async->last_buffer_idx_packed;
+
+	for (i = 0; i < n_buffers; i++) {
+		uint16_t flags;
+
+		if (async->buffers_packed[from].len)
+			flags = VRING_DESC_F_WRITE;
+		else
+			flags = 0;
+
+		if (vq->used_wrap_counter) {
+			flags |= VRING_DESC_F_USED;
+			flags |= VRING_DESC_F_AVAIL;
 		} else {
-			vhost_update_used_packed(vq, async->buffers_packed + from,
-				vq->size - from);
-			async->last_buffer_idx_packed = 0;
-			nr_left -= vq->size - from;
+			flags &= ~VRING_DESC_F_USED;
+			flags &= ~VRING_DESC_F_AVAIL;
 		}
-	} while (nr_left > 0);
+
+		if (i > 0) {
+			vq->desc_packed[vq->last_used_idx].flags = flags;
+		} else {
+			head_idx = vq->last_used_idx;
+			head_flags = flags;
+		}
+
+		vq_inc_last_used_packed(vq, async->buffers_packed[from].count);
+
+		from++;
+		if (from == vq->size)
+			from = 0;
+	}
+
+	vq->desc_packed[head_idx].flags = head_flags;
+	async->last_buffer_idx_packed = from;
 }
 
 static __rte_always_inline uint16_t
-- 
2.25.1

RE: [PATCH v2] vhost: fix packed ring descriptor update in async enqueue

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Wednesday, November 10, 2021 8:40 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; He,
> Xingguang <xingguang.he@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: [PATCH v2] vhost: fix packed ring descriptor update in async enqueue
> 
> If the packet uses multiple descrptors and its descriptor indices are

descrptors -> descriptors

With this fixed:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

> wrapped, the first descriptor flag is not updated last, which may cause
> virtio read the incomplete packet. For example, given a packet uses 64
> descriptors, and virtio ring size is 256, and its descriptor indices is
> 224~255 and 0~31, current implementation will update 224~255 descriptor
> flags earlier than 0~31 descriptor flags.
> 
> This patch fixes this issue by updating descriptor flags in one loop,
> so that the first descriptor flag is always updated last.
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
> v2:
> * update commit log
> ---
>  lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
>  1 file changed, 54 insertions(+), 68 deletions(-)

Re: [PATCH v3] vhost: fix packed ring descriptor update in async enqueue

[Maxime Coquelin]

On 11/16/21 16:17, Jiayu Hu wrote:
> If the packet uses multiple descriptors and its descriptor indices are
> wrapped, the first descriptor flag is not updated last, which may cause
> virtio read the incomplete packet. For example, given a packet uses 64
> descriptors, and virtio ring size is 256, and its descriptor indices are
> 224~255 and 0~31, current implementation will update 224~255 descriptor
> flags earlier than 0~31 descriptor flags.
> 
> This patch fixes this issue by updating descriptor flags in one loop,
> so that the first descriptor flag is always updated last.
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
> v3:
> * fix typo
> v2:
> * update commit log
> ---
>   lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
>   1 file changed, 54 insertions(+), 68 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH v3] vhost: fix packed ring descriptor update in async enqueue

[Maxime Coquelin]

On 11/16/21 16:17, Jiayu Hu wrote:
> If the packet uses multiple descriptors and its descriptor indices are
> wrapped, the first descriptor flag is not updated last, which may cause
> virtio read the incomplete packet. For example, given a packet uses 64
> descriptors, and virtio ring size is 256, and its descriptor indices are
> 224~255 and 0~31, current implementation will update 224~255 descriptor
> flags earlier than 0~31 descriptor flags.
> 
> This patch fixes this issue by updating descriptor flags in one loop,
> so that the first descriptor flag is always updated last.
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
> v3:
> * fix typo
> v2:
> * update commit log
> ---
>   lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
>   1 file changed, 54 insertions(+), 68 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

[PATCH v3] vhost: fix packed ring descriptor update in async enqueue

[Jiayu Hu]

If the packet uses multiple descriptors and its descriptor indices are
wrapped, the first descriptor flag is not updated last, which may cause
virtio read the incomplete packet. For example, given a packet uses 64
descriptors, and virtio ring size is 256, and its descriptor indices are
224~255 and 0~31, current implementation will update 224~255 descriptor
flags earlier than 0~31 descriptor flags.

This patch fixes this issue by updating descriptor flags in one loop,
so that the first descriptor flag is always updated last.

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
v3:
* fix typo
v2:
* update commit log
---
 lib/vhost/virtio_net.c | 122 ++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 68 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index cef4bcf15c..b3d954aab4 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1549,60 +1549,6 @@ virtio_dev_rx_async_submit_split(struct virtio_net *dev,
 	return pkt_idx;
 }
 
-static __rte_always_inline void
-vhost_update_used_packed(struct vhost_virtqueue *vq,
-			struct vring_used_elem_packed *shadow_ring,
-			uint16_t count)
-{
-	int i;
-	uint16_t used_idx = vq->last_used_idx;
-	uint16_t head_idx = vq->last_used_idx;
-	uint16_t head_flags = 0;
-
-	if (count == 0)
-		return;
-
-	/* Split loop in two to save memory barriers */
-	for (i = 0; i < count; i++) {
-		vq->desc_packed[used_idx].id = shadow_ring[i].id;
-		vq->desc_packed[used_idx].len = shadow_ring[i].len;
-
-		used_idx += shadow_ring[i].count;
-		if (used_idx >= vq->size)
-			used_idx -= vq->size;
-	}
-
-	/* The ordering for storing desc flags needs to be enforced. */
-	rte_atomic_thread_fence(__ATOMIC_RELEASE);
-
-	for (i = 0; i < count; i++) {
-		uint16_t flags;
-
-		if (vq->shadow_used_packed[i].len)
-			flags = VRING_DESC_F_WRITE;
-		else
-			flags = 0;
-
-		if (vq->used_wrap_counter) {
-			flags |= VRING_DESC_F_USED;
-			flags |= VRING_DESC_F_AVAIL;
-		} else {
-			flags &= ~VRING_DESC_F_USED;
-			flags &= ~VRING_DESC_F_AVAIL;
-		}
-
-		if (i > 0) {
-			vq->desc_packed[vq->last_used_idx].flags = flags;
-		} else {
-			head_idx = vq->last_used_idx;
-			head_flags = flags;
-		}
-
-		vq_inc_last_used_packed(vq, shadow_ring[i].count);
-	}
-
-	vq->desc_packed[head_idx].flags = head_flags;
-}
 
 static __rte_always_inline int
 vhost_enqueue_async_packed(struct virtio_net *dev,
@@ -1819,23 +1765,63 @@ write_back_completed_descs_packed(struct vhost_virtqueue *vq,
 				uint16_t n_buffers)
 {
 	struct vhost_async *async = vq->async;
-	uint16_t nr_left = n_buffers;
-	uint16_t from, to;
+	uint16_t from = async->last_buffer_idx_packed;
+	uint16_t used_idx = vq->last_used_idx;
+	uint16_t head_idx = vq->last_used_idx;
+	uint16_t head_flags = 0;
+	uint16_t i;
 
-	do {
-		from = async->last_buffer_idx_packed;
-		to = (from + nr_left) % vq->size;
-		if (to > from) {
-			vhost_update_used_packed(vq, async->buffers_packed + from, to - from);
-			async->last_buffer_idx_packed += nr_left;
-			nr_left = 0;
+	/* Split loop in two to save memory barriers */
+	for (i = 0; i < n_buffers; i++) {
+		vq->desc_packed[used_idx].id = async->buffers_packed[from].id;
+		vq->desc_packed[used_idx].len = async->buffers_packed[from].len;
+
+		used_idx += async->buffers_packed[from].count;
+		if (used_idx >= vq->size)
+			used_idx -= vq->size;
+
+		from++;
+		if (from >= vq->size)
+			from = 0;
+	}
+
+	/* The ordering for storing desc flags needs to be enforced. */
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
+
+	from = async->last_buffer_idx_packed;
+
+	for (i = 0; i < n_buffers; i++) {
+		uint16_t flags;
+
+		if (async->buffers_packed[from].len)
+			flags = VRING_DESC_F_WRITE;
+		else
+			flags = 0;
+
+		if (vq->used_wrap_counter) {
+			flags |= VRING_DESC_F_USED;
+			flags |= VRING_DESC_F_AVAIL;
 		} else {
-			vhost_update_used_packed(vq, async->buffers_packed + from,
-				vq->size - from);
-			async->last_buffer_idx_packed = 0;
-			nr_left -= vq->size - from;
+			flags &= ~VRING_DESC_F_USED;
+			flags &= ~VRING_DESC_F_AVAIL;
 		}
-	} while (nr_left > 0);
+
+		if (i > 0) {
+			vq->desc_packed[vq->last_used_idx].flags = flags;
+		} else {
+			head_idx = vq->last_used_idx;
+			head_flags = flags;
+		}
+
+		vq_inc_last_used_packed(vq, async->buffers_packed[from].count);
+
+		from++;
+		if (from == vq->size)
+			from = 0;
+	}
+
+	vq->desc_packed[head_idx].flags = head_flags;
+	async->last_buffer_idx_packed = from;
 }
 
 static __rte_always_inline uint16_t
-- 
2.25.1