[PATCH v3] vhost/vsock: batch RX used-ring updates

[PATCH v3] vhost/vsock: batch RX used-ring updates

[Jia Jia]

vhost_transport_do_send_pkt() calls vhost_add_used() for every Guest RX
buffer even though it delays the Guest signal until the worker finishes.
Each call publishes one used entry and updates the used index separately.

Collect the completed buffer heads in the arrays already allocated for the
virtqueue and publish them with vhost_add_used_n().  Bound the batch by the
ring size, array capacity, and worker packet budget.  Flush before
re-enabling notifications or leaving the worker.

Each used entry describes one completed RX buffer and keeps its actual used
length, so set nheads to 1 for every entry.  This patch does not change
negotiated features or compress multiple buffers into one used entry.

This patch is limited to the current skb-based vhost-vsock RX path.

Performance:

Tested with a QEMU/KVM guest on a host with 4 online CPUs, using 2 vCPUs
pinned to host CPUs 2 and 3, QEMU 10.2.1, q35, 1536 MiB, and Linux
7.2.0-rc3-next-20260713-next-debug-kasan.  The vhost-vsock source is based
on linux-next master at 49362394dad7df66c274c867a271394c10ca2bb8.

Current vhost-vsock does not implement VIRTIO_F_IN_ORDER or
VIRTIO_F_RING_PACKED, so both configurations used packed=off and
in_order=off:

  baseline:                 RX batching=off
  vhost-vsock RX batching:  RX batching=on

The test used vsock_perf.  The Guest receiver was started with:

  vsock_perf --port PORT --buf-size 64M --vsk-size 64M --rcvlowat 1

The Host sender was started with:

  vsock_perf --sender 3 --port PORT --bytes BYTES \
      --buf-size SEND_BUF --vsk-size 64M

Each workload transferred BYTES=1 GiB.  The SEND_BUF values were 256 B
(SEND_BUF=256), 512 B (SEND_BUF=512), 4 KiB (SEND_BUF=4K), and 64 KiB
(SEND_BUF=64K).
Each state used a fresh Guest.  Each workload uses 20 paired runs, with 10
runs in each order.  The reported values are
Guest RX throughput in Gbits/s.  The baseline and batching columns are the
geometric means over the 20 runs; change is batching / baseline - 1,
computed from the unrounded values:

  workload   baseline RX    batching RX   change   faster
  256 B      0.0795724      0.0831509      +4.497%  20/20
  512 B      0.1194885      0.1210297      +1.290%  14/20
  4 KiB      0.7208273      0.7242053      +0.469%  11/20
  64 KiB     2.1712797      2.1951941      +1.101%  13/20

For reference, the table below gives the 95% normal-approximation intervals
obtained from the 20 paired log(batching / baseline) values:

  workload   paired 95% interval
  256 B      +3.985% to +5.011%
  512 B      +0.206% to +2.385%
  4 KiB      -1.442% to +2.416%
  64 KiB     -1.474% to +3.745%

All transfers passed byte-count checks, and no kernel errors were observed
in the logs.  The 256-byte workload improved in every pair.  The 512 B
workload was faster in 14 of 20 pairs, with a small gain.  The 4 KiB and
64 KiB workloads showed no material throughput change; the difference
between their results may be due to scheduling and execution variation.

The Guest RX throughput results above are the primary performance
measurement. For additional Host-side context, I measured the vhost
worker thread servicing the vhost-vsock RX queue in a separate set of
10 paired runs, with five runs in each AB/BA order. Counters were
normalized by the verified transferred GiB and summarized using
geometric means. Worker cycles/GiB improved by 3.846%, 2.162%, and
4.109% for 256 B, 4 KiB, and 64 KiB, respectively. The corresponding
worker instructions/GiB improvements were 2.548%, 2.084%, and 4.637%.
The patched implementation used fewer cycles in 10/10, 8/10, and
10/10 paired runs, respectively, and fewer instructions in 10/10
paired runs for all three workloads. This measures the complete vhost
worker thread during the transfer, rather than an individual helper
function, and is supplementary to the Guest RX throughput results.

Link: https://lore.kernel.org/r/20181214082146-mutt-send-email-mst@kernel.org
Link: https://lore.kernel.org/r/20220901055434.824-4-qtxuning1999@sjtu.edu.cn
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
Changes in v3:
- Add supplementary Host-side perf measurements for the complete
  vhost worker thread during the transfer.
---
 drivers/vhost/vsock.c | 53 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 9aaab6bb8061..9e72c67c287f 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -103,12 +103,35 @@ static bool vhost_transport_has_remote_cid(struct vsock_sock *vsk, u32 cid)
 	return found;
 }
 
+static bool vhost_vsock_flush_used(struct vhost_virtqueue *vq,
+				   unsigned int used_count)
+{
+	if (!used_count)
+		return false;
+
+	vhost_add_used_n(vq, vq->heads, vq->nheads, used_count);
+	return true;
+}
+
+static void vhost_vsock_add_used(struct vhost_virtqueue *vq,
+				 unsigned int used_count,
+				 unsigned int head, unsigned int len)
+{
+	struct vring_used_elem *used = &vq->heads[used_count];
+
+	used->id = cpu_to_vhost32(vq, head);
+	used->len = cpu_to_vhost32(vq, len);
+	vq->nheads[used_count] = 1;
+}
+
 static void
 vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 			    struct vhost_virtqueue *vq)
 {
 	struct vhost_virtqueue *tx_vq = &vsock->vqs[VSOCK_VQ_TX];
 	int pkts = 0, total_len = 0;
+	unsigned int used_count = 0;
+	unsigned int used_limit;
 	bool added = false;
 	bool restart_tx = false;
 
@@ -120,6 +143,12 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 	if (!vq_meta_prefetch(vq))
 		goto out;
 
+	used_limit = min_t(unsigned int, vq->num,
+			   min_t(unsigned int, vq->dev->iov_limit,
+				 vq->dev->weight));
+	if (unlikely(!used_limit))
+		goto out;
+
 	/* Avoid further vmexits, we're already processing the virtqueue */
 	vhost_disable_notify(&vsock->dev, vq);
 
@@ -134,9 +163,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 		u32 offset;
 		int head;
 
+		if (used_count == used_limit) {
+			if (vhost_vsock_flush_used(vq, used_count)) {
+				added = true;
+				used_count = 0;
+			}
+		}
+
 		skb = virtio_vsock_skb_dequeue(&vsock->send_pkt_queue);
 
 		if (!skb) {
+			if (vhost_vsock_flush_used(vq, used_count)) {
+				added = true;
+				used_count = 0;
+			}
 			vhost_enable_notify(&vsock->dev, vq);
 			break;
 		}
@@ -153,6 +193,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 			/* We cannot finish yet if more buffers snuck in while
 			 * re-enabling notify.
 			 */
+			if (vhost_vsock_flush_used(vq, used_count)) {
+				added = true;
+				used_count = 0;
+			}
 			if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
 				vhost_disable_notify(&vsock->dev, vq);
 				continue;
@@ -230,8 +274,9 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 		 */
 		virtio_transport_deliver_tap_pkt(skb);
 
-		vhost_add_used(vq, head, sizeof(*hdr) + payload_len);
-		added = true;
+		vhost_vsock_add_used(vq, used_count, head,
+				     sizeof(*hdr) + payload_len);
+		used_count++;
 
 		VIRTIO_VSOCK_SKB_CB(skb)->offset += payload_len;
 		total_len += payload_len;
@@ -264,6 +309,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 			virtio_transport_consume_skb_sent(skb, true);
 		}
 	} while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));
+	if (vhost_vsock_flush_used(vq, used_count)) {
+		added = true;
+		used_count = 0;
+	}
 	if (added)
 		vhost_signal(&vsock->dev, vq);
 
-- 
2.34.1

Re: [PATCH v3] vhost/vsock: batch RX used-ring updates

[Stefano Garzarella]

On Fri, Sep 04, 2026 at 09:25:37AM +0800, Jia Jia wrote:
>vhost_transport_do_send_pkt() calls vhost_add_used() for every Guest RX
>buffer even though it delays the Guest signal until the worker finishes.
>Each call publishes one used entry and updates the used index separately.
>
>Collect the completed buffer heads in the arrays already allocated for the
>virtqueue and publish them with vhost_add_used_n().  Bound the batch by the
>ring size, array capacity, and worker packet budget.  Flush before
>re-enabling notifications or leaving the worker.
>
>Each used entry describes one completed RX buffer and keeps its actual used
>length, so set nheads to 1 for every entry.  This patch does not change
>negotiated features or compress multiple buffers into one used entry.
>
>This patch is limited to the current skb-based vhost-vsock RX path.
>
>Performance:

It's great to include the performance metrics in the commit, and thanks 
for that, but I don't think we need all this AI slop that follows, 
please summarize it.

>
>Tested with a QEMU/KVM guest on a host with 4 online CPUs, using 2 vCPUs
>pinned to host CPUs 2 and 3, QEMU 10.2.1, q35, 1536 MiB, and Linux
>7.2.0-rc3-next-20260713-next-debug-kasan.  The vhost-vsock source is based
>on linux-next master at 49362394dad7df66c274c867a271394c10ca2bb8.
>

e.g. from here...

>Current vhost-vsock does not implement VIRTIO_F_IN_ORDER or
>VIRTIO_F_RING_PACKED, so both configurations used packed=off and
>in_order=off:
>
>  baseline:                 RX batching=off
>  vhost-vsock RX batching:  RX batching=on

... to here, can be removed.

>
>The test used vsock_perf.  The Guest receiver was started with:
>
>  vsock_perf --port PORT --buf-size 64M --vsk-size 64M --rcvlowat 1
>
>The Host sender was started with:
>
>  vsock_perf --sender 3 --port PORT --bytes BYTES \
>      --buf-size SEND_BUF --vsk-size 64M
>
>Each workload transferred BYTES=1 GiB.  The SEND_BUF values were 256 B
>(SEND_BUF=256), 512 B (SEND_BUF=512), 4 KiB (SEND_BUF=4K), and 64 KiB
>(SEND_BUF=64K).

Put `1 GiB` directly after --bytes, no?

About SEND_BUF values, use SEND_BUF in the table header, and remove the
text here.

>Each state used a fresh Guest.  Each workload uses 20 paired runs, with 10
>runs in each order.  The reported values are
>Guest RX throughput in Gbits/s.  The baseline and batching columns are the
>geometric means over the 20 runs; change is batching / baseline - 1,
>computed from the unrounded values:

Ditto, summarize or remove (e.g. Gbits/s can be put in the table 
header).

>
>  workload   baseline RX    batching RX   change   faster
>  256 B      0.0795724      0.0831509      +4.497%  20/20
>  512 B      0.1194885      0.1210297      +1.290%  14/20
>  4 KiB      0.7208273      0.7242053      +0.469%  11/20
>  64 KiB     2.1712797      2.1951941      +1.101%  13/20
>

What about the latency?

>For reference, the table below gives the 95% normal-approximation intervals
>obtained from the 20 paired log(batching / baseline) values:
>
>  workload   paired 95% interval
>  256 B      +3.985% to +5.011%
>  512 B      +0.206% to +2.385%
>  4 KiB      -1.442% to +2.416%
>  64 KiB     -1.474% to +3.745%
>
>All transfers passed byte-count checks, and no kernel errors were observed
>in the logs.  The 256-byte workload improved in every pair.  The 512 B
>workload was faster in 14 of 20 pairs, with a small gain.  The 4 KiB and
>64 KiB workloads showed no material throughput change; the difference
>between their results may be due to scheduling and execution variation.

All this text can be removed, it's clear from the table, no?

>
>The Guest RX throughput results above are the primary performance
>measurement. For additional Host-side context, I measured the vhost
>worker thread servicing the vhost-vsock RX queue in a separate set of
>10 paired runs, with five runs in each AB/BA order. Counters were
>normalized by the verified transferred GiB and summarized using
>geometric means. Worker cycles/GiB improved by 3.846%, 2.162%, and
>4.109% for 256 B, 4 KiB, and 64 KiB, respectively. The corresponding
>worker instructions/GiB improvements were 2.548%, 2.084%, and 4.637%.
>The patched implementation used fewer cycles in 10/10, 8/10, and
>10/10 paired runs, respectively, and fewer instructions in 10/10
>paired runs for all three workloads. This measures the complete vhost
>worker thread during the transfer, rather than an individual helper
>function, and is supplementary to the Guest RX throughput results.

Please, summarize.

>
>Link: https://lore.kernel.org/r/20181214082146-mutt-send-email-mst@kernel.org

You put this link, but you didn't explain why...

>Link: https://lore.kernel.org/r/20220901055434.824-4-qtxuning1999@sjtu.edu.cn

Ditto.

>Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>Acked-by: Eugenio Pérez <eperezma@redhat.com>
>---
>Changes in v3:
>- Add supplementary Host-side perf measurements for the complete
>  vhost worker thread during the transfer.
>---
> drivers/vhost/vsock.c | 53 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 51 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 9aaab6bb8061..9e72c67c287f 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -103,12 +103,35 @@ static bool vhost_transport_has_remote_cid(struct vsock_sock *vsk, u32 cid)
> 	return found;
> }
>
>+static bool vhost_vsock_flush_used(struct vhost_virtqueue *vq,
>+				   unsigned int used_count)
>+{
>+	if (!used_count)
>+		return false;
>+
>+	vhost_add_used_n(vq, vq->heads, vq->nheads, used_count);
>+	return true;
>+}
>+
>+static void vhost_vsock_add_used(struct vhost_virtqueue *vq,
>+				 unsigned int used_count,
>+				 unsigned int head, unsigned int len)
>+{
>+	struct vring_used_elem *used = &vq->heads[used_count];
>+
>+	used->id = cpu_to_vhost32(vq, head);
>+	used->len = cpu_to_vhost32(vq, len);
>+	vq->nheads[used_count] = 1;
>+}

Would it be better to move these functions to vhost.c?
(not a strong opinion)

>+
> static void
> vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 			    struct vhost_virtqueue *vq)
> {
> 	struct vhost_virtqueue *tx_vq = &vsock->vqs[VSOCK_VQ_TX];
> 	int pkts = 0, total_len = 0;
>+	unsigned int used_count = 0;
>+	unsigned int used_limit;
> 	bool added = false;
> 	bool restart_tx = false;
>
>@@ -120,6 +143,12 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 	if (!vq_meta_prefetch(vq))
> 		goto out;
>

Can you add a comment whith the reason of this limit?

>+	used_limit = min_t(unsigned int, vq->num,
>+			   min_t(unsigned int, vq->dev->iov_limit,
>+				 vq->dev->weight));

Why adding `vq->dev->weight` in the limit, the loop is already limited
by that, no?

(this is why a comment here is needed...)


>+	if (unlikely(!used_limit))
>+		goto out;
>+
> 	/* Avoid further vmexits, we're already processing the virtqueue */
> 	vhost_disable_notify(&vsock->dev, vq);
>
>@@ -134,9 +163,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 		u32 offset;
> 		int head;
>
>+		if (used_count == used_limit) {
>+			if (vhost_vsock_flush_used(vq, used_count)) {
>+				added = true;
>+				used_count = 0;
>+			}
>+		}

Can we move this in the vhost_vsock_add_used() or just after calling it?
IMO, it's easier to read: add something, check if I've reached the
limit, then flush.

>+
> 		skb = virtio_vsock_skb_dequeue(&vsock->send_pkt_queue);
>
> 		if (!skb) {
>+			if (vhost_vsock_flush_used(vq, used_count)) {
>+				added = true;
>+				used_count = 0;
>+			}

Why you need this, if after the loop we are calling
vhost_vsock_flush_used() in any case?

> 			vhost_enable_notify(&vsock->dev, vq);
> 			break;
> 		}
>@@ -153,6 +193,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 			/* We cannot finish yet if more buffers snuck in while
> 			 * re-enabling notify.
> 			 */

Move the comment or update it explaining why we are flushing.

>+			if (vhost_vsock_flush_used(vq, used_count)) {
>+				added = true;
>+				used_count = 0;
>+			}
> 			if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> 				vhost_disable_notify(&vsock->dev, vq);
> 				continue;
>@@ -230,8 +274,9 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 		 */
> 		virtio_transport_deliver_tap_pkt(skb);
>
>-		vhost_add_used(vq, head, sizeof(*hdr) + payload_len);
>-		added = true;
>+		vhost_vsock_add_used(vq, used_count, head,
>+				     sizeof(*hdr) + payload_len);
>+		used_count++;
>
> 		VIRTIO_VSOCK_SKB_CB(skb)->offset += payload_len;
> 		total_len += payload_len;
>@@ -264,6 +309,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> 			virtio_transport_consume_skb_sent(skb, true);
> 		}
> 	} while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));

Please leave a blank line here.

>+	if (vhost_vsock_flush_used(vq, used_count)) {
>+		added = true;
>+		used_count = 0;

Why setting this here that we are going to exit?

IMO this can be simplified in:
	added |= vhost_vsock_flush_used(vq, used_count);

or you can collapse this in the check for the vhost_signal:

	if (vhost_vsock_flush_used(vq, used_count) || added)
		vhost_signal(&vsock->dev, vq);

>+	}

Please leave a blank line here.

Stefano

> 	if (added)
> 		vhost_signal(&vsock->dev, vq);
>
>-- 
>2.34.1
>

Re: [PATCH v3] vhost/vsock: batch RX used-ring updates

[Jia Jia]

>
> On Fri, Sep 04, 2026 at 09:25:37AM +0800, Jia Jia wrote:
> >vhost_transport_do_send_pkt() calls vhost_add_used() for every Guest RX
> >buffer even though it delays the Guest signal until the worker finishes.
> >Each call publishes one used entry and updates the used index separately.
> >
> >Collect the completed buffer heads in the arrays already allocated for the
> >virtqueue and publish them with vhost_add_used_n().  Bound the batch by the
> >ring size, array capacity, and worker packet budget.  Flush before
> >re-enabling notifications or leaving the worker.
> >
> >Each used entry describes one completed RX buffer and keeps its actual used
> >length, so set nheads to 1 for every entry.  This patch does not change
> >negotiated features or compress multiple buffers into one used entry.
> >
> >This patch is limited to the current skb-based vhost-vsock RX path.
> >
> >Performance:
>
> It's great to include the performance metrics in the commit, and thanks
> for that, but I don't think we need all this AI slop that follows,
> please summarize it.
>

Thanks for the thorough review.   I'll summarize the performance
section in the next version.

> >
> >Tested with a QEMU/KVM guest on a host with 4 online CPUs, using 2 vCPUs
> >pinned to host CPUs 2 and 3, QEMU 10.2.1, q35, 1536 MiB, and Linux
> >7.2.0-rc3-next-20260713-next-debug-kasan.  The vhost-vsock source is based
> >on linux-next master at 49362394dad7df66c274c867a271394c10ca2bb8.
> >
>
> e.g. from here...
>
> >Current vhost-vsock does not implement VIRTIO_F_IN_ORDER or
> >VIRTIO_F_RING_PACKED, so both configurations used packed=off and
> >in_order=off:
> >
> >  baseline:                 RX batching=off
> >  vhost-vsock RX batching:  RX batching=on
>
> ... to here, can be removed.
>

Understood. I'll remove this setup and configuration description.

> >
> >The test used vsock_perf.  The Guest receiver was started with:
> >
> >  vsock_perf --port PORT --buf-size 64M --vsk-size 64M --rcvlowat 1
> >
> >The Host sender was started with:
> >
> >  vsock_perf --sender 3 --port PORT --bytes BYTES \
> >      --buf-size SEND_BUF --vsk-size 64M
> >
> >Each workload transferred BYTES=1 GiB.  The SEND_BUF values were 256 B
> >(SEND_BUF=256), 512 B (SEND_BUF=512), 4 KiB (SEND_BUF=4K), and 64 KiB
> >(SEND_BUF=64K).
>
> Put `1 GiB` directly after --bytes, no?

I'll change the command to use `--bytes 1G`.

>
> About SEND_BUF values, use SEND_BUF in the table header, and remove the
> text here.
>

Understood. I'll put `SEND_BUF` in the table header and remove the
explanatory text.

> >Each state used a fresh Guest.  Each workload uses 20 paired runs, with 10
> >runs in each order.  The reported values are
> >Guest RX throughput in Gbits/s.  The baseline and batching columns are the
> >geometric means over the 20 runs; change is batching / baseline - 1,
> >computed from the unrounded values:
>
> Ditto, summarize or remove (e.g. Gbits/s can be put in the table
> header).
>

I'll put the unit in the table header and remove the extra explanation.

> >
> >  workload   baseline RX    batching RX   change   faster
> >  256 B      0.0795724      0.0831509      +4.497%  20/20
> >  512 B      0.1194885      0.1210297      +1.290%  14/20
> >  4 KiB      0.7208273      0.7242053      +0.469%  11/20
> >  64 KiB     2.1712797      2.1951941      +1.101%  13/20
> >
>
> What about the latency?
>

I will run an additional userspace request-response test:

Host send -> Guest echo -> Host userspace receive

The host will timestamp before sending and after receiving the echo to
measure the round-trip time. If this approach looks reasonable, I will
include the results in the next version.

> >For reference, the table below gives the 95% normal-approximation intervals
> >obtained from the 20 paired log(batching / baseline) values:
> >
> >  workload   paired 95% interval
> >  256 B      +3.985% to +5.011%
> >  512 B      +0.206% to +2.385%
> >  4 KiB      -1.442% to +2.416%
> >  64 KiB     -1.474% to +3.745%
> >
> >All transfers passed byte-count checks, and no kernel errors were observed
> >in the logs.  The 256-byte workload improved in every pair.  The 512 B
> >workload was faster in 14 of 20 pairs, with a small gain.  The 4 KiB and
> >64 KiB workloads showed no material throughput change; the difference
> >between their results may be due to scheduling and execution variation.
>
> All this text can be removed, it's clear from the table, no?
>

I'll remove it.

> >
> >The Guest RX throughput results above are the primary performance
> >measurement. For additional Host-side context, I measured the vhost
> >worker thread servicing the vhost-vsock RX queue in a separate set of
> >10 paired runs, with five runs in each AB/BA order. Counters were
> >normalized by the verified transferred GiB and summarized using
> >geometric means. Worker cycles/GiB improved by 3.846%, 2.162%, and
> >4.109% for 256 B, 4 KiB, and 64 KiB, respectively. The corresponding
> >worker instructions/GiB improvements were 2.548%, 2.084%, and 4.637%.
> >The patched implementation used fewer cycles in 10/10, 8/10, and
> >10/10 paired runs, respectively, and fewer instructions in 10/10
> >paired runs for all three workloads. This measures the complete vhost
> >worker thread during the transfer, rather than an individual helper
> >function, and is supplementary to the Guest RX throughput results.
>
> Please, summarize.
>

I'll summarize the host-side perf results as supplementary data.

> >
> >Link: https://lore.kernel.org/r/20181214082146-mutt-send-email-mst@kernel.org
>
> You put this link, but you didn't explain why...
>
> >Link: https://lore.kernel.org/r/20220901055434.824-4-qtxuning1999@sjtu.edu.cn
>
> Ditto.
>

The first link points to the earlier vhost-vsock RX batching discussion,
and the second points to related vhost-vsock TX/IN_ORDER batching work.
I included them as background for the design choices, but I agree that
they are not necessary here and will remove both links in the next version.

> >Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> >Acked-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> >Changes in v3:
> >- Add supplementary Host-side perf measurements for the complete
> >  vhost worker thread during the transfer.
> >---
> > drivers/vhost/vsock.c | 53 +++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> >index 9aaab6bb8061..9e72c67c287f 100644
> >--- a/drivers/vhost/vsock.c
> >+++ b/drivers/vhost/vsock.c
> >@@ -103,12 +103,35 @@ static bool vhost_transport_has_remote_cid(struct vsock_sock *vsk, u32 cid)
> >       return found;
> > }
> >
> >+static bool vhost_vsock_flush_used(struct vhost_virtqueue *vq,
> >+                                 unsigned int used_count)
> >+{
> >+      if (!used_count)
> >+              return false;
> >+
> >+      vhost_add_used_n(vq, vq->heads, vq->nheads, used_count);
> >+      return true;
> >+}
> >+
> >+static void vhost_vsock_add_used(struct vhost_virtqueue *vq,
> >+                               unsigned int used_count,
> >+                               unsigned int head, unsigned int len)
> >+{
> >+      struct vring_used_elem *used = &vq->heads[used_count];
> >+
> >+      used->id = cpu_to_vhost32(vq, head);
> >+      used->len = cpu_to_vhost32(vq, len);
> >+      vq->nheads[used_count] = 1;
> >+}
>
> Would it be better to move these functions to vhost.c?
> (not a strong opinion)
>

I'd prefer to keep them in vsock.c for now, since vhost-vsock is
currently the only caller.
I can move them to vhost.c if you prefer.

> >+
> > static void
> > vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >                           struct vhost_virtqueue *vq)
> > {
> >       struct vhost_virtqueue *tx_vq = &vsock->vqs[VSOCK_VQ_TX];
> >       int pkts = 0, total_len = 0;
> >+      unsigned int used_count = 0;
> >+      unsigned int used_limit;
> >       bool added = false;
> >       bool restart_tx = false;
> >
> >@@ -120,6 +143,12 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >       if (!vq_meta_prefetch(vq))
> >               goto out;
> >
>
> Can you add a comment whith the reason of this limit?
>

Sure. I'll add a comment explaining why the limit is bounded by both the
used-ring size and the scratch-array capacity.

> >+      used_limit = min_t(unsigned int, vq->num,
> >+                         min_t(unsigned int, vq->dev->iov_limit,
> >+                               vq->dev->weight));
>
> Why adding `vq->dev->weight` in the limit, the loop is already limited
> by that, no?
>
> (this is why a comment here is needed...)
>
>

I checked this again. weight is already enforced by vhost_exceeds_weight(),
so including it in used_limit is redundant. Thanks for pointing that
out. I'll change it to:

used_limit = min_t(unsigned int, vq->num, vq->dev->iov_limit);

> >+      if (unlikely(!used_limit))
> >+              goto out;
> >+
> >       /* Avoid further vmexits, we're already processing the virtqueue */
> >       vhost_disable_notify(&vsock->dev, vq);
> >
> >@@ -134,9 +163,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >               u32 offset;
> >               int head;
> >
> >+              if (used_count == used_limit) {
> >+                      if (vhost_vsock_flush_used(vq, used_count)) {
> >+                              added = true;
> >+                              used_count = 0;
> >+                      }
> >+              }
>
> Can we move this in the vhost_vsock_add_used() or just after calling it?
> IMO, it's easier to read: add something, check if I've reached the
> limit, then flush.
>

I'll move the check immediately after adding the used entry:

    vhost_vsock_add_used(vq, used_count, head,
                         sizeof(*hdr) + payload_len);
    used_count++;

    if (used_count == used_limit) {
            added |= vhost_vsock_flush_used(vq, used_count);
            used_count = 0;
    }

> >+
> >               skb = virtio_vsock_skb_dequeue(&vsock->send_pkt_queue);
> >
> >               if (!skb) {
> >+                      if (vhost_vsock_flush_used(vq, used_count)) {
> >+                              added = true;
> >+                              used_count = 0;
> >+                      }
>
> Why you need this, if after the loop we are calling
> vhost_vsock_flush_used() in any case?
>

My reason for flushing there was that, when used_count > 0 and
skb is NULL, the batch can still contain completed RX descriptors that
have not yet been published. Flushing there makes those completions
visible sooner. I will follow your suggestion and rely on the flush
after the loop instead, which makes the control flow more uniform. I
will also measure the latency impact.

> >                       vhost_enable_notify(&vsock->dev, vq);
> >                       break;
> >               }
> >@@ -153,6 +193,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >                       /* We cannot finish yet if more buffers snuck in while
> >                        * re-enabling notify.
> >                        */
>
> Move the comment or update it explaining why we are flushing.
>

I'll update the comment to explain why we flush on that path

> >+                      if (vhost_vsock_flush_used(vq, used_count)) {
> >+                              added = true;
> >+                              used_count = 0;
> >+                      }
> >                       if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> >                               vhost_disable_notify(&vsock->dev, vq);
> >                               continue;
> >@@ -230,8 +274,9 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >                */
> >               virtio_transport_deliver_tap_pkt(skb);
> >
> >-              vhost_add_used(vq, head, sizeof(*hdr) + payload_len);
> >-              added = true;
> >+              vhost_vsock_add_used(vq, used_count, head,
> >+                                   sizeof(*hdr) + payload_len);
> >+              used_count++;
> >
> >               VIRTIO_VSOCK_SKB_CB(skb)->offset += payload_len;
> >               total_len += payload_len;
> >@@ -264,6 +309,10 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >                       virtio_transport_consume_skb_sent(skb, true);
> >               }
> >       } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));
>
> Please leave a blank line here.
>

Understood

> >+      if (vhost_vsock_flush_used(vq, used_count)) {
> >+              added = true;
> >+              used_count = 0;
>
> Why setting this here that we are going to exit?
>
> IMO this can be simplified in:
>         added |= vhost_vsock_flush_used(vq, used_count);
>
> or you can collapse this in the check for the vhost_signal:
>
>         if (vhost_vsock_flush_used(vq, used_count) || added)
>                 vhost_signal(&vsock->dev, vq);
>
> >+      }
>

Thanks for pointing this out. Resetting used_count is unnecessary
since the function is about to return. I'll simplify it to:

added |= vhost_vsock_flush_used(vq, used_count);

> Please leave a blank line here.
>

Understood

> Stefano
>
> >       if (added)
> >               vhost_signal(&vsock->dev, vq);
> >
> >--
> >2.34.1
> >
>