[PATCH v2 1/2] virtio_ring: add virtqueue_enable_cb_delayed_max

[PATCH v2 1/2] virtio_ring: add virtqueue_enable_cb_delayed_max

[Michael S. Tsirkin]

virtqueue_enable_cb_delayed places the used event at 3/4 of the
currently queued buffers, so with VIRTIO_RING_F_EVENT_IDX the device
interrupts early and often - this is a good fit for drivers that poll
and process buffers as they trickle in, like virtio net.

Some drivers queue a batch of buffers and then sleep waiting for the
whole batch to complete.  For these, an interrupt before the last
buffer is used is pure waste.

To fix, add virtqueue_enable_cb_delayed_max: same API, but the used
event is placed at the last currently queued buffer, so the device
only notifies us once it has used everything.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    changes since v1:
        fix off-by-one: place the used event one entry before the
        last queued buffer.  The device only signals once it passes
        the event, so with the event at the last buffer it never
        fired and callers waiting for the batch to complete hung.

 drivers/virtio/virtio_ring.c | 127 +++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |   2 +
 2 files changed, 129 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbca7ce1c6bf..4c0bb860b703 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -180,6 +180,7 @@ struct virtqueue_ops {
 	bool (*kick_prepare)(struct vring_virtqueue *vq);
 	void (*disable_cb)(struct vring_virtqueue *vq);
 	bool (*enable_cb_delayed)(struct vring_virtqueue *vq);
+	bool (*enable_cb_delayed_max)(struct vring_virtqueue *vq);
 	unsigned int (*enable_cb_prepare)(struct vring_virtqueue *vq);
 	bool (*poll)(const struct vring_virtqueue *vq,
 		     unsigned int last_used_idx);
@@ -1122,6 +1123,43 @@ static bool virtqueue_enable_cb_delayed_split(struct vring_virtqueue *vq)
 	return true;
 }
 
+static bool virtqueue_enable_cb_delayed_max_split(struct vring_virtqueue *vq)
+{
+	u16 bufs;
+
+	START_USE(vq);
+
+	/* We optimistically turn back on interrupts, then check if there was
+	 * more to do. */
+	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
+	 * either clear the flags bit or point the event index at the next
+	 * entry. Always update the event index to keep code simple. */
+	if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
+		vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+		if (!vq->event)
+			vq->split.vring.avail->flags =
+				cpu_to_virtio16(vq->vq.vdev,
+						vq->split.avail_flags_shadow);
+	}
+	/* Point the used event one entry before the last queued buffer:
+	 * the device signals when it passes the event, so this makes it
+	 * notify us exactly once it has used everything. */
+	bufs = (u16)(vq->split.avail_idx_shadow - vq->last_used_idx) - 1;
+
+	virtio_store_mb(vq->weak_barriers,
+			&vring_used_event(&vq->split.vring),
+			cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs));
+
+	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
+						- vq->last_used_idx) > bufs)) {
+		END_USE(vq);
+		return false;
+	}
+
+	END_USE(vq);
+	return true;
+}
+
 static void *virtqueue_detach_unused_buf_split(struct vring_virtqueue *vq)
 {
 	unsigned int i;
@@ -2318,6 +2356,68 @@ static bool virtqueue_enable_cb_delayed_packed(struct vring_virtqueue *vq)
 	return true;
 }
 
+static bool virtqueue_enable_cb_delayed_max_packed(struct vring_virtqueue *vq)
+{
+	u16 used_idx, wrap_counter, last_used_idx;
+	u16 bufs;
+
+	START_USE(vq);
+
+	/*
+	 * We optimistically turn back on interrupts, then check if there was
+	 * more to do.
+	 */
+
+	if (vq->event) {
+		/* Point the event one entry before the last queued buffer:
+		 * the device signals when it passes the event, so this
+		 * makes it notify us exactly once it has used everything. */
+		bufs = vq->packed.vring.num - vq->vq.num_free - 1;
+		last_used_idx = READ_ONCE(vq->last_used_idx);
+		wrap_counter = packed_used_wrap_counter(last_used_idx);
+
+		used_idx = packed_last_used(last_used_idx) + bufs;
+		if (used_idx >= vq->packed.vring.num) {
+			used_idx -= vq->packed.vring.num;
+			wrap_counter ^= 1;
+		}
+
+		vq->packed.vring.driver->off_wrap = cpu_to_le16(used_idx |
+			(wrap_counter << VRING_PACKED_EVENT_F_WRAP_CTR));
+
+		/*
+		 * We need to update event offset and event wrap
+		 * counter first before updating event flags.
+		 */
+		virtio_wmb(vq->weak_barriers);
+	}
+
+	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DISABLE) {
+		vq->packed.event_flags_shadow = vq->event ?
+				VRING_PACKED_EVENT_FLAG_DESC :
+				VRING_PACKED_EVENT_FLAG_ENABLE;
+		vq->packed.vring.driver->flags =
+				cpu_to_le16(vq->packed.event_flags_shadow);
+	}
+
+	/*
+	 * We need to update event suppression structure first
+	 * before re-checking for more used buffers.
+	 */
+	virtio_mb(vq->weak_barriers);
+
+	last_used_idx = READ_ONCE(vq->last_used_idx);
+	wrap_counter = packed_used_wrap_counter(last_used_idx);
+	used_idx = packed_last_used(last_used_idx);
+	if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
+		END_USE(vq);
+		return false;
+	}
+
+	END_USE(vq);
+	return true;
+}
+
 static void *virtqueue_detach_unused_buf_packed(struct vring_virtqueue *vq)
 {
 	unsigned int i;
@@ -2640,6 +2740,7 @@ static const struct virtqueue_ops split_ops = {
 	.kick_prepare = virtqueue_kick_prepare_split,
 	.disable_cb = virtqueue_disable_cb_split,
 	.enable_cb_delayed = virtqueue_enable_cb_delayed_split,
+	.enable_cb_delayed_max = virtqueue_enable_cb_delayed_max_split,
 	.enable_cb_prepare = virtqueue_enable_cb_prepare_split,
 	.poll = virtqueue_poll_split,
 	.detach_unused_buf = virtqueue_detach_unused_buf_split,
@@ -2654,6 +2755,7 @@ static const struct virtqueue_ops packed_ops = {
 	.kick_prepare = virtqueue_kick_prepare_packed,
 	.disable_cb = virtqueue_disable_cb_packed,
 	.enable_cb_delayed = virtqueue_enable_cb_delayed_packed,
+	.enable_cb_delayed_max = virtqueue_enable_cb_delayed_max_packed,
 	.enable_cb_prepare = virtqueue_enable_cb_prepare_packed,
 	.poll = virtqueue_poll_packed,
 	.detach_unused_buf = virtqueue_detach_unused_buf_packed,
@@ -2668,6 +2770,7 @@ static const struct virtqueue_ops split_in_order_ops = {
 	.kick_prepare = virtqueue_kick_prepare_split,
 	.disable_cb = virtqueue_disable_cb_split,
 	.enable_cb_delayed = virtqueue_enable_cb_delayed_split,
+	.enable_cb_delayed_max = virtqueue_enable_cb_delayed_max_split,
 	.enable_cb_prepare = virtqueue_enable_cb_prepare_split,
 	.poll = virtqueue_poll_split,
 	.detach_unused_buf = virtqueue_detach_unused_buf_split,
@@ -2682,6 +2785,7 @@ static const struct virtqueue_ops packed_in_order_ops = {
 	.kick_prepare = virtqueue_kick_prepare_packed,
 	.disable_cb = virtqueue_disable_cb_packed,
 	.enable_cb_delayed = virtqueue_enable_cb_delayed_packed,
+	.enable_cb_delayed_max = virtqueue_enable_cb_delayed_max_packed,
 	.enable_cb_prepare = virtqueue_enable_cb_prepare_packed,
 	.poll = virtqueue_poll_packed,
 	.detach_unused_buf = virtqueue_detach_unused_buf_packed,
@@ -3197,6 +3301,29 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_enable_cb_delayed);
 
+/**
+ * virtqueue_enable_cb_delayed_max - callback for all queued buffers
+ * @_vq: the struct virtqueue we're talking about.
+ *
+ * Like virtqueue_enable_cb_delayed, but with VIRTIO_RING_F_EVENT_IDX
+ * the used event is placed at the last currently queued buffer, so the
+ * device only notifies us once it has used everything we queued.
+ *
+ * Returns "true" if the callback is armed and we can sleep waiting
+ * for the notification, or "false" if some buffers were already used
+ * and we should poll instead.
+ */
+bool virtqueue_enable_cb_delayed_max(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	if (vq->event_triggered)
+		data_race(vq->event_triggered = false);
+
+	return VIRTQUEUE_CALL(vq, enable_cb_delayed_max);
+}
+EXPORT_SYMBOL_GPL(virtqueue_enable_cb_delayed_max);
+
 /**
  * virtqueue_detach_unused_buf - detach first unused buffer
  * @_vq: the struct virtqueue we're talking about.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3bbc4cb6a672..214fa36b8eed 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -114,6 +114,8 @@ bool virtqueue_poll(struct virtqueue *vq, unsigned);
 
 bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
 
+bool virtqueue_enable_cb_delayed_max(struct virtqueue *vq);
+
 void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
 unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
-- 
MST

[PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Michael S. Tsirkin]

The virtio_balloon driver reports a scatter-gather list of free pages
to the VMM so it may discard unused pages.

For an arm64 kernel with 64 KiB pages, the default reporting and
allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
report contains a sufficient number of large entries, we will end up
with a descriptor chain that represents over 4 GiB of memory. This
violates virtio spec, which says:
	Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;

Specifically, submitting $2^{32}$ or more breaks Rust-VMM.

Submit each reported page as a separate virtqueue buffer instead of one
buffer with an sg list of all pages.  This also avoids indirect
descriptor allocation (kmalloc in the reporting path).

To avoid multiple buffers causing interrupt storms, keep callbacks
disabled while we wait for an event.

TODO: as an alternative, keep using virtqueue_enable_cb_delayed
but add buffers after there's space in the ring.

Note: if the virtqueue is broken, wait_event on
virtqueue_get_buf hangs.  This is a pre-existing issue:
the old single-buffer code had the same hang.

Reported-by: Ruben Hakobyan <hruben@meta.com>
Cc: Ruben Hakobyan <hruben@meta.com>
Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    changes since v1:
        keep callbacks disabled while draining: get_buf re-arms the used
        event for the next buffer, reintroducing an interrupt per page.
        Disable the reporting vq callbacks at probe, and only arm the
        delayed callback to wait.

 drivers/virtio/virtio_balloon.c | 59 +++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f6c2dff33f8a..b88b463488e0 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -202,25 +202,53 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
 	struct virtio_balloon *vb =
 		container_of(pr_dev_info, struct virtio_balloon, pr_dev_info);
 	struct virtqueue *vq = vb->reporting_vq;
-	unsigned int unused, err;
+	unsigned int i, err = 0;
 
 	/* We should always be able to add these buffers to an empty queue. */
-	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT);
+	for (i = 0; i < nents; i++) {
+		struct scatterlist one;
 
-	/*
-	 * In the extremely unlikely case that something has occurred and we
-	 * are able to trigger an error we will simply display a warning
-	 * and exit without actually processing the pages.
-	 */
-	if (WARN_ON_ONCE(err))
-		return err;
+		sg_init_table(&one, 1);
+		sg_set_page(&one, sg_page(&sg[i]), sg[i].length,
+			    sg[i].offset);
+		err = virtqueue_add_inbuf(vq, &one, 1, &sg[i], GFP_NOWAIT);
 
-	virtqueue_kick(vq);
+		/*
+		 * In the extremely unlikely case that something has occurred
+		 * and we are able to trigger an error we will simply display
+		 * a warning and exit without actually processing the pages.
+		 */
+		if (WARN_ON_ONCE(err)) {
+			nents = i;
+			break;
+		}
+	}
 
-	/* When host has read buffer, this completes via balloon_ack */
-	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
+	if (nents) {
+		unsigned int unused;
 
-	return 0;
+		virtqueue_kick(vq);
+
+		/* When host has read buffer, this completes via balloon_ack. */
+		i = 0;
+		wait_event(vb->acked, ({
+			for (;;) {
+				virtqueue_disable_cb(vq);
+				while (virtqueue_get_buf(vq, &unused))
+					i++;
+
+				/* Stop waiting if device used all that we added. */
+				if (i >= nents)
+					break;
+
+				if (virtqueue_enable_cb_delayed_max(vq))
+					break;
+			}
+			i >= nents;
+		}));
+	}
+
+	return err;
 }
 
 static void set_page_pfns(struct virtio_balloon *vb,
@@ -635,8 +663,11 @@ static int init_vqs(struct virtio_balloon *vb)
 	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		vb->free_page_vq = vqs[VIRTIO_BALLOON_VQ_FREE_PAGE];
 
-	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING))
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) {
 		vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING];
+		/* We re-enable callbacks after add, when we want them. */
+		virtqueue_disable_cb(vb->reporting_vq);
+	}
 
 	return 0;
 }
-- 
MST

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Ruben Hakobyan]

On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> The virtio_balloon driver reports a scatter-gather list of free pages
> to the VMM so it may discard unused pages.
>
> For an arm64 kernel with 64 KiB pages, the default reporting and
> allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> report contains a sufficient number of large entries, we will end up
> with a descriptor chain that represents over 4 GiB of memory. This
> violates virtio spec, which says:
>         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
>
> Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
>
> Submit each reported page as a separate virtqueue buffer instead of one
> buffer with an sg list of all pages.  This also avoids indirect
> descriptor allocation (kmalloc in the reporting path).
>
> To avoid multiple buffers causing interrupt storms, keep callbacks
> disabled while we wait for an event.
>
> TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> but add buffers after there's space in the ring.
>
> Note: if the virtqueue is broken, wait_event on
> virtqueue_get_buf hangs.  This is a pre-existing issue:
> the old single-buffer code had the same hang.
>
> Reported-by: Ruben Hakobyan <hruben@meta.com>
> Cc: Ruben Hakobyan <hruben@meta.com>
> Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Notes (changelog):
>     changes since v1:
>         keep callbacks disabled while draining: get_buf re-arms the used
>         event for the next buffer, reintroducing an interrupt per page.
>         Disable the reporting vq callbacks at probe, and only arm the
>         delayed callback to wait.
>

Thanks for the patches! I think sashiko-bot found some issues on broken
queues in the first patch that are worth looking into for correctness.

Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
both on 4 KiB and 64 KiB guests, and this works correctly.

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Michael S. Tsirkin]

On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > The virtio_balloon driver reports a scatter-gather list of free pages
> > to the VMM so it may discard unused pages.
> >
> > For an arm64 kernel with 64 KiB pages, the default reporting and
> > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > report contains a sufficient number of large entries, we will end up
> > with a descriptor chain that represents over 4 GiB of memory. This
> > violates virtio spec, which says:
> >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> >
> > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> >
> > Submit each reported page as a separate virtqueue buffer instead of one
> > buffer with an sg list of all pages.  This also avoids indirect
> > descriptor allocation (kmalloc in the reporting path).
> >
> > To avoid multiple buffers causing interrupt storms, keep callbacks
> > disabled while we wait for an event.
> >
> > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > but add buffers after there's space in the ring.
> >
> > Note: if the virtqueue is broken, wait_event on
> > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > the old single-buffer code had the same hang.
> >
> > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > Cc: Ruben Hakobyan <hruben@meta.com>
> > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Notes (changelog):
> >     changes since v1:
> >         keep callbacks disabled while draining: get_buf re-arms the used
> >         event for the next buffer, reintroducing an interrupt per page.
> >         Disable the reporting vq callbacks at probe, and only arm the
> >         delayed callback to wait.
> >
> 
> Thanks for the patches! I think sashiko-bot found some issues on broken
> queues in the first patch that are worth looking into for correctness.

Surprise removal of the balloon? Nope, it's a pre-existing issue that
it's broken.  Besides it's not really practical, no one removes
the balloon - no need.

> Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> both on 4 KiB and 64 KiB guests, and this works correctly.

Thanks a lot! Can you pls confirm whether the performance is about the
same/better/worse?

-- 
MST

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Ruben Hakobyan]

On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> > On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > to the VMM so it may discard unused pages.
> > >
> > > For an arm64 kernel with 64 KiB pages, the default reporting and
> > > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > > report contains a sufficient number of large entries, we will end up
> > > with a descriptor chain that represents over 4 GiB of memory. This
> > > violates virtio spec, which says:
> > >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > >
> > > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> > >
> > > Submit each reported page as a separate virtqueue buffer instead of one
> > > buffer with an sg list of all pages.  This also avoids indirect
> > > descriptor allocation (kmalloc in the reporting path).
> > >
> > > To avoid multiple buffers causing interrupt storms, keep callbacks
> > > disabled while we wait for an event.
> > >
> > > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > > but add buffers after there's space in the ring.
> > >
> > > Note: if the virtqueue is broken, wait_event on
> > > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > > the old single-buffer code had the same hang.
> > >
> > > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > > Cc: Ruben Hakobyan <hruben@meta.com>
> > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > Notes (changelog):
> > >     changes since v1:
> > >         keep callbacks disabled while draining: get_buf re-arms the used
> > >         event for the next buffer, reintroducing an interrupt per page.
> > >         Disable the reporting vq callbacks at probe, and only arm the
> > >         delayed callback to wait.
> > >
> >
> > Thanks for the patches! I think sashiko-bot found some issues on broken
> > queues in the first patch that are worth looking into for correctness.
>
> Surprise removal of the balloon? Nope, it's a pre-existing issue that
> it's broken.  Besides it's not really practical, no one removes
> the balloon - no need.

Fair point about that. But I meant the broken queue check.
In virtqueue_enable_cb_delayed() there is:
```
/*
 * When the device is broken there is no point in polling used->idx,
 * the backend will never update it. Return true to let callers
 * exit their cleanup loops instead of spinning forever.
 */
if (unlikely(vq->broken))
    return true;
```

But virtqueue_enable_cb_delayed_max() doesn't have this. So I was just
thinking the same reasoning should apply here?

>
> > Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> > both on 4 KiB and 64 KiB guests, and this works correctly.
>
> Thanks a lot! Can you pls confirm whether the performance is about the
> same/better/worse?

From my tests on QEMU, CPU time was about the same as my original patch
and unmodified Linux.

To bring a concrete example, with these patches it took ~437 ms of CPU to
reclaim a whole 8 GiB with 4 KiB pages (vs ~434 ms on unmodified Linux).

With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
patch).

(I tested this on Nvidia Grace)

>
> --
> MST
>

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Michael S. Tsirkin]

On Tue, Oct 27, 2026 at 05:32:04AM -0700, Ruben Hakobyan wrote:
> On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> > > On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > to the VMM so it may discard unused pages.
> > > >
> > > > For an arm64 kernel with 64 KiB pages, the default reporting and
> > > > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > > > report contains a sufficient number of large entries, we will end up
> > > > with a descriptor chain that represents over 4 GiB of memory. This
> > > > violates virtio spec, which says:
> > > >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > > >
> > > > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> > > >
> > > > Submit each reported page as a separate virtqueue buffer instead of one
> > > > buffer with an sg list of all pages.  This also avoids indirect
> > > > descriptor allocation (kmalloc in the reporting path).
> > > >
> > > > To avoid multiple buffers causing interrupt storms, keep callbacks
> > > > disabled while we wait for an event.
> > > >
> > > > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > > > but add buffers after there's space in the ring.
> > > >
> > > > Note: if the virtqueue is broken, wait_event on
> > > > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > > > the old single-buffer code had the same hang.
> > > >
> > > > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > > > Cc: Ruben Hakobyan <hruben@meta.com>
> > > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > > > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >
> > > > Notes (changelog):
> > > >     changes since v1:
> > > >         keep callbacks disabled while draining: get_buf re-arms the used
> > > >         event for the next buffer, reintroducing an interrupt per page.
> > > >         Disable the reporting vq callbacks at probe, and only arm the
> > > >         delayed callback to wait.
> > > >
> > >
> > > Thanks for the patches! I think sashiko-bot found some issues on broken
> > > queues in the first patch that are worth looking into for correctness.
> >
> > Surprise removal of the balloon? Nope, it's a pre-existing issue that
> > it's broken.  Besides it's not really practical, no one removes
> > the balloon - no need.
> 
> Fair point about that. But I meant the broken queue check.
> In virtqueue_enable_cb_delayed() there is:
> ```
> /*
>  * When the device is broken there is no point in polling used->idx,
>  * the backend will never update it. Return true to let callers
>  * exit their cleanup loops instead of spinning forever.
>  */
> if (unlikely(vq->broken))
>     return true;
> ```
> 
> But virtqueue_enable_cb_delayed_max() doesn't have this. So I was just
> thinking the same reasoning should apply here?

It's broken there, too.  E.g. what is the "cleanup loop" in balloon?

> >
> > > Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> > > both on 4 KiB and 64 KiB guests, and this works correctly.
> >
> > Thanks a lot! Can you pls confirm whether the performance is about the
> > same/better/worse?
> 
> >From my tests on QEMU, CPU time was about the same as my original patch
> and unmodified Linux.
> 
> To bring a concrete example, with these patches it took ~437 ms of CPU to
> reclaim a whole 8 GiB with 4 KiB pages (vs ~434 ms on unmodified Linux).

OK so that's just noise, there's no difference.

> 
> With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
> patch).

Is this measurable, or also noise?

> 
> (I tested this on Nvidia Grace)


Thanks!

> >
> > --
> > MST
> >

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Ruben Hakobyan]

On Tue, Oct 27, 2026 at 12:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Oct 27, 2026 at 05:32:04AM -0700, Ruben Hakobyan wrote:
> > On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> > > > On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > > to the VMM so it may discard unused pages.
> > > > >
> > > > > For an arm64 kernel with 64 KiB pages, the default reporting and
> > > > > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > > > > report contains a sufficient number of large entries, we will end up
> > > > > with a descriptor chain that represents over 4 GiB of memory. This
> > > > > violates virtio spec, which says:
> > > > >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > > > >
> > > > > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> > > > >
> > > > > Submit each reported page as a separate virtqueue buffer instead of one
> > > > > buffer with an sg list of all pages.  This also avoids indirect
> > > > > descriptor allocation (kmalloc in the reporting path).
> > > > >
> > > > > To avoid multiple buffers causing interrupt storms, keep callbacks
> > > > > disabled while we wait for an event.
> > > > >
> > > > > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > > > > but add buffers after there's space in the ring.
> > > > >
> > > > > Note: if the virtqueue is broken, wait_event on
> > > > > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > > > > the old single-buffer code had the same hang.
> > > > >
> > > > > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > > > > Cc: Ruben Hakobyan <hruben@meta.com>
> > > > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > > > > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >
> > > > > Notes (changelog):
> > > > >     changes since v1:
> > > > >         keep callbacks disabled while draining: get_buf re-arms the used
> > > > >         event for the next buffer, reintroducing an interrupt per page.
> > > > >         Disable the reporting vq callbacks at probe, and only arm the
> > > > >         delayed callback to wait.
> > > > >
> > > >
> > > > Thanks for the patches! I think sashiko-bot found some issues on broken
> > > > queues in the first patch that are worth looking into for correctness.
> > >
> > > Surprise removal of the balloon? Nope, it's a pre-existing issue that
> > > it's broken.  Besides it's not really practical, no one removes
> > > the balloon - no need.
> >
> > Fair point about that. But I meant the broken queue check.
> > In virtqueue_enable_cb_delayed() there is:
> > ```
> > /*
> >  * When the device is broken there is no point in polling used->idx,
> >  * the backend will never update it. Return true to let callers
> >  * exit their cleanup loops instead of spinning forever.
> >  */
> > if (unlikely(vq->broken))
> >     return true;
> > ```
> >
> > But virtqueue_enable_cb_delayed_max() doesn't have this. So I was just
> > thinking the same reasoning should apply here?
>
> It's broken there, too.  E.g. what is the "cleanup loop" in balloon?

The loop in virtballoon_free_page_report():
```
wait_event(vb->acked, ({
    for (;;) {
        virtqueue_disable_cb(vq);
        while (virtqueue_get_buf(vq, &unused))
            i++;

        /* Stop waiting if device used all that we added. */
        if (i >= nents)
            break;

        if (virtqueue_enable_cb_delayed_max(vq))
            break;
    }
    i >= nents;
}));
```

For a broken queue, you could exit the for loop early and avoid busy looping.
It still hangs, but I think it can sleep rather than busy loop?

Anyway, I'm not sure how practical a broken queue is, so I don't have a
strong opinion.

>
> > >
> > > > Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> > > > both on 4 KiB and 64 KiB guests, and this works correctly.
> > >
> > > Thanks a lot! Can you pls confirm whether the performance is about the
> > > same/better/worse?
> >
> > >From my tests on QEMU, CPU time was about the same as my original patch
> > and unmodified Linux.
> >
> > To bring a concrete example, with these patches it took ~437 ms of CPU to
> > reclaim a whole 8 GiB with 4 KiB pages (vs ~434 ms on unmodified Linux).
>
> OK so that's just noise, there's no difference.
>
> >
> > With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
> > patch).
>
> Is this measurable, or also noise?
>
> >
> > (I tested this on Nvidia Grace)
>
>
> Thanks!

Thanks, yeah I agree and my point was that performance looks on par.
I just wanted to share some concrete numbers of what I've tested.

>
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Michael S. Tsirkin]

On Tue, Oct 27, 2026 at 07:39:57AM -0700, Ruben Hakobyan wrote:
> On Tue, Oct 27, 2026 at 12:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Oct 27, 2026 at 05:32:04AM -0700, Ruben Hakobyan wrote:
> > > On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> > > > > On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > > > to the VMM so it may discard unused pages.
> > > > > >
> > > > > > For an arm64 kernel with 64 KiB pages, the default reporting and
> > > > > > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > > > > > report contains a sufficient number of large entries, we will end up
> > > > > > with a descriptor chain that represents over 4 GiB of memory. This
> > > > > > violates virtio spec, which says:
> > > > > >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > > > > >
> > > > > > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> > > > > >
> > > > > > Submit each reported page as a separate virtqueue buffer instead of one
> > > > > > buffer with an sg list of all pages.  This also avoids indirect
> > > > > > descriptor allocation (kmalloc in the reporting path).
> > > > > >
> > > > > > To avoid multiple buffers causing interrupt storms, keep callbacks
> > > > > > disabled while we wait for an event.
> > > > > >
> > > > > > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > > > > > but add buffers after there's space in the ring.
> > > > > >
> > > > > > Note: if the virtqueue is broken, wait_event on
> > > > > > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > > > > > the old single-buffer code had the same hang.
> > > > > >
> > > > > > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > > > > > Cc: Ruben Hakobyan <hruben@meta.com>
> > > > > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > > > > > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Notes (changelog):
> > > > > >     changes since v1:
> > > > > >         keep callbacks disabled while draining: get_buf re-arms the used
> > > > > >         event for the next buffer, reintroducing an interrupt per page.
> > > > > >         Disable the reporting vq callbacks at probe, and only arm the
> > > > > >         delayed callback to wait.
> > > > > >
> > > > >
> > > > > Thanks for the patches! I think sashiko-bot found some issues on broken
> > > > > queues in the first patch that are worth looking into for correctness.
> > > >
> > > > Surprise removal of the balloon? Nope, it's a pre-existing issue that
> > > > it's broken.  Besides it's not really practical, no one removes
> > > > the balloon - no need.
> > >
> > > Fair point about that. But I meant the broken queue check.
> > > In virtqueue_enable_cb_delayed() there is:
> > > ```
> > > /*
> > >  * When the device is broken there is no point in polling used->idx,
> > >  * the backend will never update it. Return true to let callers
> > >  * exit their cleanup loops instead of spinning forever.
> > >  */
> > > if (unlikely(vq->broken))
> > >     return true;
> > > ```
> > >
> > > But virtqueue_enable_cb_delayed_max() doesn't have this. So I was just
> > > thinking the same reasoning should apply here?
> >
> > It's broken there, too.  E.g. what is the "cleanup loop" in balloon?
> 
> The loop in virtballoon_free_page_report():
> ```
> wait_event(vb->acked, ({
>     for (;;) {
>         virtqueue_disable_cb(vq);
>         while (virtqueue_get_buf(vq, &unused))
>             i++;
> 
>         /* Stop waiting if device used all that we added. */
>         if (i >= nents)
>             break;
> 
>         if (virtqueue_enable_cb_delayed_max(vq))
>             break;
>     }
>     i >= nents;
> }));
> ```
> 
> For a broken queue, you could exit the for loop early and avoid busy looping.
> It still hangs, but I think it can sleep rather than busy loop?
> 
> Anyway, I'm not sure how practical a broken queue is, so I don't have a
> strong opinion.

Maybe add the broken queue check explicitly then, then it does
not need to hang:

if (virtqueue_is_broken(vq))
	i = nents;


but the issue is what if queue gets broken while we are waiting.


> >
> > > >
> > > > > Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> > > > > both on 4 KiB and 64 KiB guests, and this works correctly.
> > > >
> > > > Thanks a lot! Can you pls confirm whether the performance is about the
> > > > same/better/worse?
> > >
> > > >From my tests on QEMU, CPU time was about the same as my original patch
> > > and unmodified Linux.
> > >
> > > To bring a concrete example, with these patches it took ~437 ms of CPU to
> > > reclaim a whole 8 GiB with 4 KiB pages (vs ~434 ms on unmodified Linux).
> >
> > OK so that's just noise, there's no difference.
> >
> > >
> > > With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
> > > patch).
> >
> > Is this measurable, or also noise?
> >
> > >
> > > (I tested this on Nvidia Grace)
> >
> >
> > Thanks!
> 
> Thanks, yeah I agree and my point was that performance looks on par.
> I just wanted to share some concrete numbers of what I've tested.

Thanks a lot!

> >
> > > >
> > > > --
> > > > MST
> > > >
> >

Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Ruben Hakobyan]

On Tue, Oct 27, 2026 at 7:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Oct 27, 2026 at 07:39:57AM -0700, Ruben Hakobyan wrote:
> > On Tue, Oct 27, 2026 at 12:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Tue, Oct 27, 2026 at 05:32:04AM -0700, Ruben Hakobyan wrote:
> > > > On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Tue, Oct 27, 2026 at 02:26:33AM -0700, Ruben Hakobyan wrote:
> > > > > > On Mon, Oct 26, 2026 at 11:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > > > > to the VMM so it may discard unused pages.
> > > > > > >
> > > > > > > For an arm64 kernel with 64 KiB pages, the default reporting and
> > > > > > > allocation orders allow entries ranging from 2 MiB to 512 MiB. If the
> > > > > > > report contains a sufficient number of large entries, we will end up
> > > > > > > with a descriptor chain that represents over 4 GiB of memory. This
> > > > > > > violates virtio spec, which says:
> > > > > > >         Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > > > > > >
> > > > > > > Specifically, submitting $2^{32}$ or more breaks Rust-VMM.
> > > > > > >
> > > > > > > Submit each reported page as a separate virtqueue buffer instead of one
> > > > > > > buffer with an sg list of all pages.  This also avoids indirect
> > > > > > > descriptor allocation (kmalloc in the reporting path).
> > > > > > >
> > > > > > > To avoid multiple buffers causing interrupt storms, keep callbacks
> > > > > > > disabled while we wait for an event.
> > > > > > >
> > > > > > > TODO: as an alternative, keep using virtqueue_enable_cb_delayed
> > > > > > > but add buffers after there's space in the ring.
> > > > > > >
> > > > > > > Note: if the virtqueue is broken, wait_event on
> > > > > > > virtqueue_get_buf hangs.  This is a pre-existing issue:
> > > > > > > the old single-buffer code had the same hang.
> > > > > > >
> > > > > > > Reported-by: Ruben Hakobyan <hruben@meta.com>
> > > > > > > Cc: Ruben Hakobyan <hruben@meta.com>
> > > > > > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> > > > > > > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes (changelog):
> > > > > > >     changes since v1:
> > > > > > >         keep callbacks disabled while draining: get_buf re-arms the used
> > > > > > >         event for the next buffer, reintroducing an interrupt per page.
> > > > > > >         Disable the reporting vq callbacks at probe, and only arm the
> > > > > > >         delayed callback to wait.
> > > > > > >
> > > > > >
> > > > > > Thanks for the patches! I think sashiko-bot found some issues on broken
> > > > > > queues in the first patch that are worth looking into for correctness.
> > > > >
> > > > > Surprise removal of the balloon? Nope, it's a pre-existing issue that
> > > > > it's broken.  Besides it's not really practical, no one removes
> > > > > the balloon - no need.
> > > >
> > > > Fair point about that. But I meant the broken queue check.
> > > > In virtqueue_enable_cb_delayed() there is:
> > > > ```
> > > > /*
> > > >  * When the device is broken there is no point in polling used->idx,
> > > >  * the backend will never update it. Return true to let callers
> > > >  * exit their cleanup loops instead of spinning forever.
> > > >  */
> > > > if (unlikely(vq->broken))
> > > >     return true;
> > > > ```
> > > >
> > > > But virtqueue_enable_cb_delayed_max() doesn't have this. So I was just
> > > > thinking the same reasoning should apply here?
> > >
> > > It's broken there, too.  E.g. what is the "cleanup loop" in balloon?
> >
> > The loop in virtballoon_free_page_report():
> > ```
> > wait_event(vb->acked, ({
> >     for (;;) {
> >         virtqueue_disable_cb(vq);
> >         while (virtqueue_get_buf(vq, &unused))
> >             i++;
> >
> >         /* Stop waiting if device used all that we added. */
> >         if (i >= nents)
> >             break;
> >
> >         if (virtqueue_enable_cb_delayed_max(vq))
> >             break;
> >     }
> >     i >= nents;
> > }));
> > ```
> >
> > For a broken queue, you could exit the for loop early and avoid busy looping.
> > It still hangs, but I think it can sleep rather than busy loop?
> >
> > Anyway, I'm not sure how practical a broken queue is, so I don't have a
> > strong opinion.
>
> Maybe add the broken queue check explicitly then, then it does
> not need to hang:
>
> if (virtqueue_is_broken(vq))
>         i = nents;
>

Ah yeah, indeed that would avoid the hang on the broken queue.

>
> but the issue is what if queue gets broken while we are waiting.
>

Hmm, yeah I see. I guess that's already outside of scope here.

>
> > >
> > > > >
> > > > > > Other than that, I tested the "normal path" on QEMU and Cloud Hypervisor,
> > > > > > both on 4 KiB and 64 KiB guests, and this works correctly.
> > > > >
> > > > > Thanks a lot! Can you pls confirm whether the performance is about the
> > > > > same/better/worse?
> > > >
> > > > >From my tests on QEMU, CPU time was about the same as my original patch
> > > > and unmodified Linux.
> > > >
> > > > To bring a concrete example, with these patches it took ~437 ms of CPU to
> > > > reclaim a whole 8 GiB with 4 KiB pages (vs ~434 ms on unmodified Linux).
> > >
> > > OK so that's just noise, there's no difference.
> > >
> > > >
> > > > With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
> > > > patch).
> > >
> > > Is this measurable, or also noise?
> > >
> > > >
> > > > (I tested this on Nvidia Grace)
> > >
> > >
> > > Thanks!
> >
> > Thanks, yeah I agree and my point was that performance looks on par.
> > I just wanted to share some concrete numbers of what I've tested.
>
> Thanks a lot!

Thanks again for the patches and for looking into this!

>
> > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > >