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

[Jia Jia]

>
> On Sun, Sep 06, 2026 at 09:36:59AM +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 and array capacity.  Flush when the batch reaches its limit or
> > before 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:
> >
> > The test used vsock_perf with a fresh Guest for each state and 20 paired
> > runs. The Guest receiver used:
> >
> >   vsock_perf --port PORT --buf-size 64M --vsk-size 64M --rcvlowat 1
> >
> > The Host sender used:
> >
> >   vsock_perf --sender 3 --port PORT --bytes 1G \
> >       --buf-size SEND_BUF --vsk-size 64M
> >
> > The table reports geometric mean Guest RX throughput.
> >
> >   SEND_BUF   baseline RX   batching RX   change
> >              (Gbit/s)       (Gbit/s)
> >   256 B      0.0795724      0.0831509      +4.497%
> >   512 B      0.1194885      0.1210297      +1.290%
> >   4 KiB      0.7208273      0.7242053      +0.469%
> >   64 KiB     2.1712797      2.1951941      +1.101%
>
>
> I will be frank I don't find the numbers compelling enough
> to bother with this trickery.
>
> vsock wasn't optimized all that much for small packets -
> first of all, it is doing most of its work in a work item
> so we are already at the mercy of the scheduler.
>
> That's more work but you will see a much bigger win for bw and
> latency by just handling small packets directly in the cb
> if you can.
>
> Looking at batching: wakeup per packet in rx_work, lock_sock per packet,
> sk->sk_write_space per rx packet are all high overhead things we are
> doing in the data path that are likely easier to handle
> and will give you more bang for the buck.
>
>
>
>
>

Thank you very much for giving such a clear direction!

> >
> > As supplementary data, perf stat measured the vhost worker cycles and
> > instructions per GiB in 10 paired runs. The patched implementation
> > reduced cycles by 3.846%, 2.162%, and 4.109%, and instructions by
> > 2.548%, 2.084%, and 4.637% for 256 B, 4 KiB, and 64 KiB, respectively.
> >
> > An AF_VSOCK request-response latency test with 10 AB/BA pairs showed no
> > consistent RTT change: -0.102% for 256-byte messages (4/10 pairs lower) and
> > +3.281% for 4-KiB messages (5/10 pairs lower), using arithmetic mean RTTs.
> >
> > Signed-off-by: Jia Jia <physicalmtea@xxxxxxxxx>
> > Acked-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > ---
> > Changes in v4:
> > - Simplify the performance description.
> > - Add a brief AF_VSOCK RTT measurement.
> > - Keep the batch limit within the used ring and scratch arrays, without
> >   duplicating the worker weight limit.
> > - Flush after adding a used entry when the batch reaches the limit, and
> >   remove the redundant flush in the empty-queue path.
> > ---
> >  drivers/vhost/vsock.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 45 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index 9aaab6bb8061..7a13abe73345 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;
> > +}
> > +
>
>
> flush where? add where? return what? these apis don't make it
> easier to read code.
>

Thanks for pointing this out. If I understand correctly, the
responsibilities, call sites, and return values of these helpers make
the control flow harder to follow. I will be careful not to make the
path more complicated for the sake of abstraction in the future.

> >  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,11 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >       if (!vq_meta_prefetch(vq))
> >               goto out;
> >
> > +     /* Keep the batch within the used ring and the scratch arrays. */
>
> what "the batch"? this is the 1st time code mentions any batch.
>

In the future, I will try not to introduce concepts used in a patch
midway through the code. They should be explained when used.

> > +     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);
> >
> > @@ -150,9 +178,16 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> >
> >               if (head == vq->num) {
> >                       virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb);
> > +
> > +                     /* Flush completed buffers before re-enabling notifications. */
>
>
> redundant - i can see this is what it does but why? and what does flush mean here?
>

I will pay attention to making the purpose of comments clear.

> > +                     if (vhost_vsock_flush_used(vq, used_count)) {
> > +                             added = true;
> > +                             used_count = 0;
> > +                     }
> > +
> >                       /* We cannot finish yet if more buffers snuck in while
> >                        * re-enabling notify.
> >                        */
> >                       if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> >                               vhost_disable_notify(&vsock->dev, vq);
> >                               continue;
> > @@ -230,8 +265,13 @@ 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++;
> > +             if (used_count == used_limit) {
> > +                     added |= vhost_vsock_flush_used(vq, used_count);
>
>
> bitwise or on a boolean likely not what was intended.
>

Understood.

> > +                     used_count = 0;
> > +             }
> >
> >               VIRTIO_VSOCK_SKB_CB(skb)->offset += payload_len;
> >               total_len += payload_len;
> > @@ -264,6 +304,9 @@ 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)));
> > +
> > +     added |= vhost_vsock_flush_used(vq, used_count);
> > +
>
>
> and here.
>

I will drop the current patch and try a new optimization direction.
Thank you.

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