[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] virtio_balloon: infinite spin in virtballoon_free_page_report() on broken vq
- [Medium] virtio_balloon: callback toggle side effects in wait_event() condition
--

commit bced8a77808ca2850eb132d7f72c3d6fc5ae5935
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_balloon: submit reported pages as individual buffers

This commit submits each reported free page as an individual virtqueue buffer
rather than a single scatter-gather chain to avoid exceeding the virtio 4 GiB
descriptor chain limit. To prevent interrupt storms from per-page buffers, it
disables reporting virtqueue callbacks at initialization and uses
virtqueue_enable_cb_delayed_max() while waiting for completion in
virtballoon_free_page_report().

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..0a331acf2c8ef 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -205,25 +205,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;
> +		}));

[Severity: High]
Can this inner for (;;) loop spin indefinitely in kernel workqueue context
when vq->broken becomes true before all nents buffers are drained?

In page_reporting_process() -> page_reporting_process_zone() ->
page_reporting_cycle() -> virtballoon_free_page_report(), if the device marks
buffers as used in the ring and vq->broken becomes true before all nents
buffers are consumed (either because virtqueue_get_buf() encounters a bad
descriptor and calls BAD_RING(), or because virtio_break_device() runs
concurrently during surprise removal or reset), virtqueue_get_buf() returns
NULL without advancing vq->last_used_idx or incrementing i.

Because i < nents, the loop then calls virtqueue_enable_cb_delayed_max(vq).
Unlike virtqueue_enable_cb_delayed(), which checks vq->broken:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed() {
	struct vring_virtqueue *vq = to_vvq(_vq);

	if (vq->event_triggered)
		data_race(vq->event_triggered = false);

	if (unlikely(vq->broken))
		return true;
...
}

virtqueue_enable_cb_delayed_max() omits that check:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max() {
	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);
}

When unconsumed used entries remain in the ring,
virtqueue_enable_cb_delayed_max_split() and
virtqueue_enable_cb_delayed_max_packed() both return false on every call:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_split() {
...
	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
						- vq->last_used_idx) > bufs)) {
		END_USE(vq);
		return false;
	}
	return true;
}

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_packed() {
...
	if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
		END_USE(vq);
		return false;
	}
	return true;
}

As a result, neither break condition in the for (;;) loop is ever met, and the
thread spins in a tight non-sleeping loop inside the wait_event() condition
rather than sleeping as noted in the commit message. Also, should
virtqueue_enable_cb_delayed_max_split() read used->idx via
vring_read_split_used_idx(vq) instead of a plain access?

[Severity: Medium]
Does placing virtqueue_disable_cb() and virtqueue_enable_cb_delayed_max()
inside the wait_event() condition cause unintended callback state changes when
wait_event() evaluates the condition multiple times?

In include/linux/wait.h, wait_event() evaluates the condition once before
calling __wait_event(), and ___wait_event() calls prepare_to_wait_event()
followed immediately by a second evaluation of the condition before calling
schedule():

include/linux/wait.h:wait_event() {
	might_sleep();
	if (condition)
		break;
	__wait_event(wq_head, condition);
}

When the host has not yet consumed all nents buffers on the first check,
virtqueue_enable_cb_delayed_max(vq) clears VRING_AVAIL_F_NO_INTERRUPT in
vq->split.avail_flags_shadow, sets vq->event_triggered = false, writes
vring_used_event = vq->last_used_idx + bufs, and issues virtio_store_mb().

Immediately afterwards, before sleeping in schedule(), the second condition
check in ___wait_event() runs virtqueue_disable_cb(vq):

drivers/virtio/virtio_ring.c:virtqueue_disable_cb_split() {
	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;

		if (vq->event_triggered)
			return;

		if (vq->event)
			/* TODO: this is a hack. Figure out a cleaner value to write. */
			vring_used_event(&vq->split.vring) = 0x0;
...
}

Because the first check enabled callbacks and vq->event_triggered is still
false, virtqueue_disable_cb_split() writes vring_used_event = 0x0 into shared
ring memory while the kicked device is actively processing buffers, right
before virtqueue_enable_cb_delayed_max(vq) overwrites it again. When
vq->last_used_idx == 0 (for example, on the first reporting batch after probe
or on 16-bit wrap), transiently writing vring_used_event = 0x0 while the
device processes the first buffer (used->idx advancing from 0 to 1) satisfies
vring_need_event(0x0, 1, 0) on the host and triggers an immediate interrupt on
the first page. The same double evaluation also happens on any wakeup of the
shared vb->acked waitqueue from inflate_vq or deflate_vq.

In addition, on a packed virtqueue, when vring_interrupt() fires and sets
vq->event_triggered = true, waking wait_event() runs
virtqueue_disable_cb_packed(vq):

drivers/virtio/virtio_ring.c:virtqueue_disable_cb_packed() {
	if (vq->packed.event_flags_shadow != VRING_PACKED_EVENT_FLAG_DISABLE) {
		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;

		if (vq->event_triggered)
			return;

		vq->packed.vring.driver->flags =
			cpu_to_le16(vq->packed.event_flags_shadow);
	}
}

Because vq->event_triggered is true, virtqueue_disable_cb_packed() updates
event_flags_shadow to VRING_PACKED_EVENT_FLAG_DISABLE and returns early
without updating vq->packed.vring.driver->flags in shared memory. When
i >= nents then breaks out of the loop without calling
virtqueue_enable_cb_delayed_max(vq), driver->flags remains
VRING_PACKED_EVENT_FLAG_DESC after virtballoon_free_page_report() returns, and
subsequent virtqueue_disable_cb(vq) calls become no-ops because
event_flags_shadow is already VRING_PACKED_EVENT_FLAG_DISABLE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/32e21059adb6cb1fd08c3f4426a313c77cf450ec.1793057483.git.mst@redhat.com?part=2

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
> > > > >
> > >

Re: [PATCH v2 1/2] virtio_ring: add virtqueue_enable_cb_delayed_max

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] virtio_ring: u16 underflow in virtqueue_enable_cb_delayed_max() when empty
- [High] virtio_ring: wrong used check in virtqueue_enable_cb_delayed_max_packed()
- [High] virtio_ring: missing broken check in virtqueue_enable_cb_delayed_max()
- [Low] virtio_ring: missing READ_ONCE() in virtqueue_enable_cb_delayed_max_split()
--

commit 3f9c8ee0d4b1a9cf9ff6d7ca78fdc78d2db0aa97
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_ring: add virtqueue_enable_cb_delayed_max

This commit introduces virtqueue_enable_cb_delayed_max() to place the used
event at the last currently queued buffer so the device only notifies the
driver once the entire queued batch has been used.

> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index db678f5a80e03..06cb2964590d5 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c

[ ... ]

> @@ -1165,6 +1166,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));

[Severity: High]
Can bufs underflow in virtqueue_enable_cb_delayed_max_split() when a caller
invokes virtqueue_enable_cb_delayed_max() after draining all queued buffers
with virtqueue_get_buf() so vq->split.avail_idx_shadow == vq->last_used_idx?

When no buffers are in flight, bufs evaluates to 0xffff, which sets
vring_used_event(&vq->split.vring) to vq->last_used_idx - 1 (delaying device
notifications until 65,536 buffers have been used) and makes the subsequent
(u16)(... - vq->last_used_idx) > bufs check impossible to satisfy.

The same u16 underflow also happens in
virtqueue_enable_cb_delayed_max_packed() when vq->vq.num_free ==
vq->packed.vring.num:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_packed() {
	if (vq->event) {
		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));
}

When packed_last_used(last_used_idx) is 0, used_idx becomes 0xffff and
subtracting vq->packed.vring.num once leaves an out-of-range descriptor
offset with bit 15 (VRING_PACKED_EVENT_F_WRAP_CTR) still set, corrupting
vq->packed.vring.driver->off_wrap. When packed_last_used(last_used_idx) > 0,
used_idx wraps in u16 arithmetic to packed_last_used(last_used_idx) - 1,
skipping the wrap_counter toggle and pointing driver->off_wrap at an
already-used descriptor with the wrong wrap counter.

> +
> +	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
> +						- vq->last_used_idx) > bufs)) {

[Severity: Low]
Should virtqueue_enable_cb_delayed_max_split() use
vring_read_split_used_idx(vq) here instead of reading
vq->split.vring.used->idx directly?

vq->split.vring.used->idx is concurrently written by the device in shared
memory, and commit 32fe1de5c124 ("virtio_ring: Add READ_ONCE annotations for
device-writable fields") updated virtqueue_enable_cb_delayed_split() to use
vring_read_split_used_idx(vq) to avoid KCSAN data-race warnings and compiler
load optimizations.

[ ... ]

> @@ -2391,6 +2429,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;
> +	}

[Severity: High]
Why does virtqueue_enable_cb_delayed_max_packed() overwrite used_idx and
wrap_counter with last_used_idx here instead of checking the target descriptor
at last_used_idx + bufs?

In virtqueue_enable_cb_delayed_max_split(), the function returns false only
once all queued buffers up through bufs have been used. Here, after arming
vq->packed.vring.driver->off_wrap for the end of the batch at + bufs, the
post-barrier check tests the very first unconsumed descriptor at
last_used_idx. If the device has completed the first buffer while the rest of
the batch is still in flight, virtqueue_enable_cb_delayed_max_packed() will
return false even though the notification is only armed for the last buffer,
causing callers waiting on the batch to busy-poll instead of sleeping.

In addition, packed_in_order_ops wires .enable_cb_delayed_max to
virtqueue_enable_cb_delayed_max_packed(). With VQ_LAYOUT_PACKED_IN_ORDER, the
device writes only a single used descriptor for an entire in-order batch and
leaves intermediate descriptors untouched (tracked via
vq->batch_last.id != UINT_MAX in more_used_packed_in_order()). If a caller
partially drains an in-order batch, last_used_idx points to an unwritten
intermediate descriptor where is_used_desc_packed() returns false even when
all buffers have already been completed, which can cause the caller to sleep
forever waiting for an interrupt that will never arrive.

[ ... ]

> @@ -3278,6 +3382,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);
> +}

[Severity: High]
What happens in virtqueue_enable_cb_delayed_max() if vq->broken becomes true
while used buffers are still present in the ring?

When a device is broken (via virtio_break_device() or BAD_RING()),
virtqueue_get_buf_ctx_split() and virtqueue_get_buf_ctx_packed() immediately
return NULL without advancing vq->last_used_idx.

Commit 0d0eff39ceb3 ("virtio_ring: fix infinite loop in virtnet_poll_cleantx
when device is broken") added a vq->broken check to
virtqueue_enable_cb_delayed():

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed() {
	struct vring_virtqueue *vq = to_vvq(_vq);

	if (unlikely(vq->broken))
		return true;
}

Without the same check in virtqueue_enable_cb_delayed_max(), won't callers
polling in a loop spin forever because virtqueue_get_buf() returns NULL while
virtqueue_enable_cb_delayed_max() repeatedly sees the unconsumed used entries
and returns false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/32e21059adb6cb1fd08c3f4426a313c77cf450ec.1793057483.git.mst@redhat.com?part=1