> > 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/[email protected] > > You put this link, but you didn't explain why... > > >Link: https://lore.kernel.org/r/[email protected] > > 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 <[email protected]> > >Acked-by: Eugenio Pérez <[email protected]> > >--- > >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 > > >
