[PATCH] virtio_balloon: batch large descriptor chains for page reporting

[PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Ruben Hakobyan]

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 section 2.7.5.2, which requires that the total
buffer length of a descriptor chain be at most 4 GiB [1]. A VMM that
faithfully follows the spec may stop processing the descriptor chain
when it exceeds 4 GiB, and discard less memory than it could. For
example, this is the case for the virtio-queue in Rust-VMM [2].

Batch the requests into descriptor chains of at most U32_MAX bytes
(4 GiB - 1 byte). We use U32_MAX here so we can conveniently represent
the batch length in a u32.

[1] https://docs.oasis-open.org/virtio/virtio/v1.4/virtio-v1.4.html
[2]
https://github.com/rust-vmm/rust-vmm/blob/9f7250b1555ebe022ea4133fda676e6311040738/vm-virtio/virtio-queue/src/chain.rs#L183-L190

Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
Signed-off-by: Ruben Hakobyan <hruben@meta.com>
---
The wording in the spec is a bit ambiguous because it uses "chain
length" for both descriptor counts and the total size of the memory
ranges referenced by the descriptors. My read is that the byte limit
applies to the sum of descriptor lengths, which is also what rust-vmm
checks.

Another option I considered was to limit reporting capacity based
on the largest possible block. E.g. for 64 KiB pages, the largest
block is 512 MiB, so limit the reporting capacity to 7 to stay
below 4GiB. Although simpler, I found that the time to reclaim
was quite a bit longer.

I tested this patch on Cloud Hypervisor (which uses Rust-VMM)
and QEMU (which doesn't enforce this limit).

 drivers/virtio/virtio_balloon.c | 47 ++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index ab3e3d887e00..68ce627c8e04 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/balloon.h>
+#include <linux/limits.h>
 #include <linux/oom.h>
 #include <linux/wait.h>
 #include <linux/mm.h>
@@ -207,21 +208,43 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
 	struct virtqueue *vq = vb->reporting_vq;
 	unsigned int unused, err;
 
-	/* We should always be able to add these buffers to an empty queue. */
-	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT);
+	while (nents) {
+		bool unmark_end;
+		struct scatterlist *sg_batch_start = sg;
+		u32 batch_len = 0;
+		unsigned int batch_nents = 0;
 
-	/*
-	 * 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;
+		/*
+		 * Virtio spec 2.7.5.2 limits the total buffer length of a
+		 * descriptor chain to 2^32 bytes (4 GiB), so split larger
+		 * reports into batches no bigger than that size.
+		 */
+		while (batch_nents < nents &&
+		       sg->length <= U32_MAX - batch_len) {
+			batch_len += sg->length;
+			batch_nents += 1;
+			sg = sg_next(sg);
+		}
+		unmark_end = !sg_is_last(&sg_batch_start[batch_nents - 1]);
 
-	virtqueue_kick(vq);
+		if (unmark_end)
+			sg_mark_end(&sg_batch_start[batch_nents - 1]);
 
-	/* When host has read buffer, this completes via balloon_ack */
-	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
+		err = virtqueue_add_inbuf(vq, sg_batch_start, batch_nents, vb,
+					  GFP_NOWAIT);
+
+		if (unmark_end)
+			sg_unmark_end(&sg_batch_start[batch_nents - 1]);
+
+		if (WARN_ON_ONCE(err))
+			return err;
+
+		virtqueue_kick(vq);
+
+		/* When host has read buffer, this completes via balloon_ack */
+		wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
+		nents -= batch_nents;
+	}
 
 	return 0;
 }
-- 
2.53.0-Meta

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Michael S. Tsirkin]

Great patch, thanks!

On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan 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 section 2.7.5.2, which requires that the total
> buffer length of a descriptor chain be at most 4 GiB [1]. A VMM that
> faithfully follows the spec may stop processing the descriptor chain
> when it exceeds 4 GiB, and discard less memory than it could. For
> example, this is the case for the virtio-queue in Rust-VMM [2].
> 
> Batch the requests into descriptor chains of at most U32_MAX bytes
> (4 GiB - 1 byte). We use U32_MAX here so we can conveniently represent
> the batch length in a u32.

It's not just a question of convenient, is it?
Rust is buggy wrt to spec as it allows only U32_MAX.
So it is also to make it work.
So should we maybe fix Rust to be spec compliant and allow 2^32 too?

I think we should definitely recommend U32_MAX in the spec
because otherwise it can not fit in the used length.


> 
> [1] https://docs.oasis-open.org/virtio/virtio/v1.4/virtio-v1.4.html
> [2]
> https://github.com/rust-vmm/rust-vmm/blob/9f7250b1555ebe022ea4133fda676e6311040738/vm-virtio/virtio-queue/src/chain.rs#L183-L190
> 
> Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> Signed-off-by: Ruben Hakobyan <hruben@meta.com>
> ---
> The wording in the spec is a bit ambiguous because it uses "chain
> length" for both descriptor counts and the total size of the memory
> ranges referenced by the descriptors. My read is that the byte limit
> applies to the sum of descriptor lengths, which is also what rust-vmm
> checks.


quote what you are referring to please, also in the commit log
not after ---.

I would also CC virtio-comment@lists.linux.dev - spec should recommend
U32_MAX.

> 
> Another option I considered was to limit reporting capacity based
> on the largest possible block. E.g. for 64 KiB pages, the largest
> block is 512 MiB, so limit the reporting capacity to 7 to stay
> below 4GiB. Although simpler, I found that the time to reclaim
> was quite a bit longer.



So I was going to apply this actually:

https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/

which makes the question completely moot.

Does this have any effect on performance?

Thanks!


> 
> I tested this patch on Cloud Hypervisor (which uses Rust-VMM)
> and QEMU (which doesn't enforce this limit).
> 
>  drivers/virtio/virtio_balloon.c | 47 ++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00..68ce627c8e04 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/balloon.h>
> +#include <linux/limits.h>
>  #include <linux/oom.h>
>  #include <linux/wait.h>
>  #include <linux/mm.h>
> @@ -207,21 +208,43 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
>  	struct virtqueue *vq = vb->reporting_vq;
>  	unsigned int unused, err;
>  
> -	/* We should always be able to add these buffers to an empty queue. */
> -	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT);
> +	while (nents) {
> +		bool unmark_end;
> +		struct scatterlist *sg_batch_start = sg;
> +		u32 batch_len = 0;
> +		unsigned int batch_nents = 0;
>  
> -	/*
> -	 * 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;
> +		/*
> +		 * Virtio spec 2.7.5.2

Please add spec version and chapter title, and quote the
spec text in question verbatim.

> limits the total buffer length of a
> +		 * descriptor chain to 2^32 bytes (4 GiB), so split larger
> +		 * reports into batches no bigger than that size.
> +		 */
> +		while (batch_nents < nents &&
> +		       sg->length <= U32_MAX - batch_len) {

here you actually limit to U32_MAX which is 2^32 - 1 as commit log says.

> +			batch_len += sg->length;
> +			batch_nents += 1;
> +			sg = sg_next(sg);
> +		}
> +		unmark_end = !sg_is_last(&sg_batch_start[batch_nents - 1]);
>  
> -	virtqueue_kick(vq);
> +		if (unmark_end)
> +			sg_mark_end(&sg_batch_start[batch_nents - 1]);
>  
> -	/* When host has read buffer, this completes via balloon_ack */
> -	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +		err = virtqueue_add_inbuf(vq, sg_batch_start, batch_nents, vb,
> +					  GFP_NOWAIT);
> +
> +		if (unmark_end)
> +			sg_unmark_end(&sg_batch_start[batch_nents - 1]);
> +
> +		if (WARN_ON_ONCE(err))
> +			return err;
> +
> +		virtqueue_kick(vq);
> +
> +		/* When host has read buffer, this completes via balloon_ack */
> +		wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +		nents -= batch_nents;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.53.0-Meta

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Ruben Hakobyan]

On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Great patch, thanks!
>
> On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote:
> > The virtio_balloon driver reports a scatter-gather list of free pages
> > to the VMM so it may discard unused pages.
...
>
> It's not just a question of convenient, is it?
> Rust is buggy wrt to spec as it allows only U32_MAX.
> So it is also to make it work.
> So should we maybe fix Rust to be spec compliant and allow 2^32 too?
>
> I think we should definitely recommend U32_MAX in the spec
> because otherwise it can not fit in the used length.
>

Thanks for the review!

Sounds good to me, I will CC virtio-comment@lists.linux.dev as
per your recommendation.

Agreed that Rust-VMM should also be fixed in case 2^32 needs to
stay the hard limit. I think here for the driver, we can be more
conservative as long as we stay below 2^32.

...

> > ---
> > The wording in the spec is a bit ambiguous because it uses "chain
> > length" for both descriptor counts and the total size of the memory
> > ranges referenced by the descriptors. My read is that the byte limit
> > applies to the sum of descriptor lengths, which is also what rust-vmm
> > checks.
>
>
> quote what you are referring to please, also in the commit log
> not after ---.

I will update with quotes in v2, thanks.

>
> I would also CC virtio-comment@lists.linux.dev - spec should recommend
> U32_MAX.
>
> >
> > Another option I considered was to limit reporting capacity based
> > on the largest possible block. E.g. for 64 KiB pages, the largest
> > block is 512 MiB, so limit the reporting capacity to 7 to stay
> > below 4GiB. Although simpler, I found that the time to reclaim
> > was quite a bit longer.
>
>
>
> So I was going to apply this actually:
>
> https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
>
> which makes the question completely moot.
>
> Does this have any effect on performance?
>
> Thanks!

Thanks for sharing this. Yes, this would also make sure we stay below
the limit. Regarding performance, I tested this on QEMU and it calls
virtio_notify() after each completion. Using one chain per entry
creates more chains, so QEMU can use more CPU time to reclaim the same
amount of memory.

If we go with this, I think a QEMU patch to batch notifications
could reduce the overhead. Alternatively, I think we could create
larger chains here but make sure they stay within the 2^32 limit,
and keeping your approach of queuing them all before kicking.

I am happy to share a patch with my thinking here.

...

> > -     if (WARN_ON_ONCE(err))
> > -             return err;
> > +             /*
> > +              * Virtio spec 2.7.5.2
>
> Please add spec version and chapter title, and quote the
> spec text in question verbatim.

Will update, thanks for the note.

>
> > limits the total buffer length of a
> > +              * descriptor chain to 2^32 bytes (4 GiB), so split larger
> > +              * reports into batches no bigger than that size.
> > +              */
> > +             while (batch_nents < nents &&
> > +                    sg->length <= U32_MAX - batch_len) {
>
> here you actually limit to U32_MAX which is 2^32 - 1 as commit log says.

I will update the comment above to be more explicit about this.
...

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Michael S. Tsirkin]

On Sun, Oct 25, 2026 at 04:56:41PM -0700, Ruben Hakobyan wrote:
> On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > Great patch, thanks!
> >
> > On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote:
> > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > to the VMM so it may discard unused pages.
> ...
> >
> > It's not just a question of convenient, is it?
> > Rust is buggy wrt to spec as it allows only U32_MAX.
> > So it is also to make it work.
> > So should we maybe fix Rust to be spec compliant and allow 2^32 too?
> >
> > I think we should definitely recommend U32_MAX in the spec
> > because otherwise it can not fit in the used length.
> >
> 
> Thanks for the review!
> 
> Sounds good to me, I will CC virtio-comment@lists.linux.dev as
> per your recommendation.
> 
> Agreed that Rust-VMM should also be fixed in case 2^32 needs to
> stay the hard limit. I think here for the driver, we can be more
> conservative as long as we stay below 2^32.
> 
> ...
> 
> > > ---
> > > The wording in the spec is a bit ambiguous because it uses "chain
> > > length" for both descriptor counts and the total size of the memory
> > > ranges referenced by the descriptors. My read is that the byte limit
> > > applies to the sum of descriptor lengths, which is also what rust-vmm
> > > checks.
> >
> >
> > quote what you are referring to please, also in the commit log
> > not after ---.
> 
> I will update with quotes in v2, thanks.
> 
> >
> > I would also CC virtio-comment@lists.linux.dev - spec should recommend
> > U32_MAX.
> >
> > >
> > > Another option I considered was to limit reporting capacity based
> > > on the largest possible block. E.g. for 64 KiB pages, the largest
> > > block is 512 MiB, so limit the reporting capacity to 7 to stay
> > > below 4GiB. Although simpler, I found that the time to reclaim
> > > was quite a bit longer.
> >
> >
> >
> > So I was going to apply this actually:
> >
> > https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
> >
> > which makes the question completely moot.
> >
> > Does this have any effect on performance?
> >
> > Thanks!
> 
> Thanks for sharing this. Yes, this would also make sure we stay below
> the limit. Regarding performance, I tested this on QEMU and it calls
> virtio_notify() after each completion.

Who, QEMU does?

> Using one chain per entry
> creates more chains, so QEMU can use more CPU time to reclaim the same
> amount of memory.
> 
> If we go with this, I think a QEMU patch to batch notifications
> could reduce the overhead. Alternatively, I think we could create
> larger chains here but make sure they stay within the 2^32 limit,
> and keeping your approach of queuing them all before kicking.
> 
> I am happy to share a patch with my thinking here.
> 
> ...

Oh. It's just the driver being silly and never disabling
callbacks.

pls just add virtqueue_enable_cb_delayed and this will fix it.

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Ruben Hakobyan]

On Mon, Oct 26, 2026 at 12:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Oct 25, 2026 at 04:56:41PM -0700, Ruben Hakobyan wrote:
> > On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > Great patch, thanks!
> > >
> > > On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote:
> > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > to the VMM so it may discard unused pages.
> > ...
> > >
> > > It's not just a question of convenient, is it?
> > > Rust is buggy wrt to spec as it allows only U32_MAX.
> > > So it is also to make it work.
> > > So should we maybe fix Rust to be spec compliant and allow 2^32 too?
> > >
> > > I think we should definitely recommend U32_MAX in the spec
> > > because otherwise it can not fit in the used length.
> > >
> >
> > Thanks for the review!
> >
> > Sounds good to me, I will CC virtio-comment@lists.linux.dev as
> > per your recommendation.
> >
> > Agreed that Rust-VMM should also be fixed in case 2^32 needs to
> > stay the hard limit. I think here for the driver, we can be more
> > conservative as long as we stay below 2^32.
> >
> > ...
> >
> > > > ---
> > > > The wording in the spec is a bit ambiguous because it uses "chain
> > > > length" for both descriptor counts and the total size of the memory
> > > > ranges referenced by the descriptors. My read is that the byte limit
> > > > applies to the sum of descriptor lengths, which is also what rust-vmm
> > > > checks.
> > >
> > >
> > > quote what you are referring to please, also in the commit log
> > > not after ---.
> >
> > I will update with quotes in v2, thanks.
> >
> > >
> > > I would also CC virtio-comment@lists.linux.dev - spec should recommend
> > > U32_MAX.
> > >
> > > >
> > > > Another option I considered was to limit reporting capacity based
> > > > on the largest possible block. E.g. for 64 KiB pages, the largest
> > > > block is 512 MiB, so limit the reporting capacity to 7 to stay
> > > > below 4GiB. Although simpler, I found that the time to reclaim
> > > > was quite a bit longer.
> > >
> > >
> > >
> > > So I was going to apply this actually:
> > >
> > > https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
> > >
> > > which makes the question completely moot.
> > >
> > > Does this have any effect on performance?
> > >
> > > Thanks!
> >
> > Thanks for sharing this. Yes, this would also make sure we stay below
> > the limit. Regarding performance, I tested this on QEMU and it calls
> > virtio_notify() after each completion.
>
> Who, QEMU does?

Yes QEMU, sorry for the ambiguous wording.

>
> > Using one chain per entry
> > creates more chains, so QEMU can use more CPU time to reclaim the same
> > amount of memory.
> >
> > If we go with this, I think a QEMU patch to batch notifications
> > could reduce the overhead. Alternatively, I think we could create
> > larger chains here but make sure they stay within the 2^32 limit,
> > and keeping your approach of queuing them all before kicking.
> >
> > I am happy to share a patch with my thinking here.
> >
> > ...
>
> Oh. It's just the driver being silly and never disabling
> callbacks.
>
> pls just add virtqueue_enable_cb_delayed and this will fix it.
>

Ah great, I tried with virtqueue_enable_cb_delayed() and yeah it
largely fixed it.

So regarding the patch in
https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/

I see it's in a larger series, so just wanted to check if you're
planning to send this patch separately? Or let me know if you'd
like me to do it. Thanks!

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Ruben Hakobyan]

Sorry, I just realised the other reply went on to the wrong thread, so just
reposting it here.

On Mon, Oct 26, 2026 at 12:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Oct 25, 2026 at 04:56:41PM -0700, Ruben Hakobyan wrote:
> > On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > Great patch, thanks!
> > >
> > > On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote:
> > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > to the VMM so it may discard unused pages.
> > ...
> > >
> > > It's not just a question of convenient, is it?
> > > Rust is buggy wrt to spec as it allows only U32_MAX.
> > > So it is also to make it work.
> > > So should we maybe fix Rust to be spec compliant and allow 2^32 too?
> > >
> > > I think we should definitely recommend U32_MAX in the spec
> > > because otherwise it can not fit in the used length.
> > >
> >
> > Thanks for the review!
> >
> > Sounds good to me, I will CC virtio-comment@lists.linux.dev as
> > per your recommendation.
> >
> > Agreed that Rust-VMM should also be fixed in case 2^32 needs to
> > stay the hard limit. I think here for the driver, we can be more
> > conservative as long as we stay below 2^32.
> >
> > ...
> >
> > > > ---
> > > > The wording in the spec is a bit ambiguous because it uses "chain
> > > > length" for both descriptor counts and the total size of the memory
> > > > ranges referenced by the descriptors. My read is that the byte limit
> > > > applies to the sum of descriptor lengths, which is also what rust-vmm
> > > > checks.
> > >
> > >
> > > quote what you are referring to please, also in the commit log
> > > not after ---.
> >
> > I will update with quotes in v2, thanks.
> >
> > >
> > > I would also CC virtio-comment@lists.linux.dev - spec should recommend
> > > U32_MAX.
> > >
> > > >
> > > > Another option I considered was to limit reporting capacity based
> > > > on the largest possible block. E.g. for 64 KiB pages, the largest
> > > > block is 512 MiB, so limit the reporting capacity to 7 to stay
> > > > below 4GiB. Although simpler, I found that the time to reclaim
> > > > was quite a bit longer.
> > >
> > >
> > >
> > > So I was going to apply this actually:
> > >
> > > https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
> > >
> > > which makes the question completely moot.
> > >
> > > Does this have any effect on performance?
> > >
> > > Thanks!
> >
> > Thanks for sharing this. Yes, this would also make sure we stay below
> > the limit. Regarding performance, I tested this on QEMU and it calls
> > virtio_notify() after each completion.
>
> Who, QEMU does?

Yes QEMU, sorry for the ambiguous wording.

>
> > Using one chain per entry
> > creates more chains, so QEMU can use more CPU time to reclaim the same
> > amount of memory.
> >
> > If we go with this, I think a QEMU patch to batch notifications
> > could reduce the overhead. Alternatively, I think we could create
> > larger chains here but make sure they stay within the 2^32 limit,
> > and keeping your approach of queuing them all before kicking.
> >
> > I am happy to share a patch with my thinking here.
> >
> > ...
>
> Oh. It's just the driver being silly and never disabling
> callbacks.
>
> pls just add virtqueue_enable_cb_delayed and this will fix it.
>

Ah great, I tried with virtqueue_enable_cb_delayed() and yeah it
largely fixed it.

So regarding the patch in
https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/

I see it's in a larger series, so just wanted to check if you're
planning to send this patch separately? Or let me know if you'd
like me to do it. Thanks!

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Michael S. Tsirkin]

On Mon, Oct 26, 2026 at 05:59:11AM -0700, Ruben Hakobyan wrote:
> Sorry, I just realised the other reply went on to the wrong thread, so just
> reposting it here.
> 
> On Mon, Oct 26, 2026 at 12:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Oct 25, 2026 at 04:56:41PM -0700, Ruben Hakobyan wrote:
> > > On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > Great patch, thanks!
> > > >
> > > > On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote:
> > > > > The virtio_balloon driver reports a scatter-gather list of free pages
> > > > > to the VMM so it may discard unused pages.
> > > ...
> > > >
> > > > It's not just a question of convenient, is it?
> > > > Rust is buggy wrt to spec as it allows only U32_MAX.
> > > > So it is also to make it work.
> > > > So should we maybe fix Rust to be spec compliant and allow 2^32 too?
> > > >
> > > > I think we should definitely recommend U32_MAX in the spec
> > > > because otherwise it can not fit in the used length.
> > > >
> > >
> > > Thanks for the review!
> > >
> > > Sounds good to me, I will CC virtio-comment@lists.linux.dev as
> > > per your recommendation.
> > >
> > > Agreed that Rust-VMM should also be fixed in case 2^32 needs to
> > > stay the hard limit. I think here for the driver, we can be more
> > > conservative as long as we stay below 2^32.
> > >
> > > ...
> > >
> > > > > ---
> > > > > The wording in the spec is a bit ambiguous because it uses "chain
> > > > > length" for both descriptor counts and the total size of the memory
> > > > > ranges referenced by the descriptors. My read is that the byte limit
> > > > > applies to the sum of descriptor lengths, which is also what rust-vmm
> > > > > checks.
> > > >
> > > >
> > > > quote what you are referring to please, also in the commit log
> > > > not after ---.
> > >
> > > I will update with quotes in v2, thanks.
> > >
> > > >
> > > > I would also CC virtio-comment@lists.linux.dev - spec should recommend
> > > > U32_MAX.
> > > >
> > > > >
> > > > > Another option I considered was to limit reporting capacity based
> > > > > on the largest possible block. E.g. for 64 KiB pages, the largest
> > > > > block is 512 MiB, so limit the reporting capacity to 7 to stay
> > > > > below 4GiB. Although simpler, I found that the time to reclaim
> > > > > was quite a bit longer.
> > > >
> > > >
> > > >
> > > > So I was going to apply this actually:
> > > >
> > > > https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
> > > >
> > > > which makes the question completely moot.
> > > >
> > > > Does this have any effect on performance?
> > > >
> > > > Thanks!
> > >
> > > Thanks for sharing this. Yes, this would also make sure we stay below
> > > the limit. Regarding performance, I tested this on QEMU and it calls
> > > virtio_notify() after each completion.
> >
> > Who, QEMU does?
> 
> Yes QEMU, sorry for the ambiguous wording.
> 
> >
> > > Using one chain per entry
> > > creates more chains, so QEMU can use more CPU time to reclaim the same
> > > amount of memory.
> > >
> > > If we go with this, I think a QEMU patch to batch notifications
> > > could reduce the overhead. Alternatively, I think we could create
> > > larger chains here but make sure they stay within the 2^32 limit,
> > > and keeping your approach of queuing them all before kicking.
> > >
> > > I am happy to share a patch with my thinking here.
> > >
> > > ...
> >
> > Oh. It's just the driver being silly and never disabling
> > callbacks.
> >
> > pls just add virtqueue_enable_cb_delayed and this will fix it.
> >
> 
> Ah great, I tried with virtqueue_enable_cb_delayed() and yeah it
> largely fixed it.
> 
> So regarding the patch in
> https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git.mst@redhat.com/
> 
> I see it's in a larger series, so just wanted to check if you're
> planning to send this patch separately? Or let me know if you'd
> like me to do it. Thanks!

Yes please do send it separately. That work stalled, the mm guys said
they will have someone look into it, then never did.

Or maybe even simpler, as below. BTW we might want
virtqueue_enable_cb_last, instead of delayed will be more efficient for
event_idx.

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 799b575a3828..dc94b417b14c 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -246,22 +246,23 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
 	if (nents) {
 		virtqueue_kick(vq);
 
 		/* When host has read buffer, this completes via balloon_ack */
 		for (i = 0; i < nents; i++) {
 			struct scatterlist *entry;
 			unsigned int used_len;
 
 			virtqueue_enable_cb_delayed(vq);
 			wait_event(vb->acked,
 				   (entry = virtqueue_get_buf(vq, &used_len)));
 			virtqueue_disable_cb(vq);
 			if (used_len == entry->length)
 				set_bit(entry - sg,
 					pr_dev_info->zeroed_bitmap);
 		}
 	}
 
 	return err;


-- 
MST

Re: [PATCH] virtio_balloon: batch large descriptor chains for page reporting

[Michael S. Tsirkin]

On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan 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 section 2.7.5.2, which requires that the total
> buffer length of a descriptor chain be at most 4 GiB [1]. A VMM that
> faithfully follows the spec may stop processing the descriptor chain
> when it exceeds 4 GiB, and discard less memory than it could. For
> example, this is the case for the virtio-queue in Rust-VMM [2].
> 
> Batch the requests into descriptor chains of at most U32_MAX bytes
> (4 GiB - 1 byte). We use U32_MAX here so we can conveniently represent
> the batch length in a u32.
> 
> [1] https://docs.oasis-open.org/virtio/virtio/v1.4/virtio-v1.4.html
> [2]
> https://github.com/rust-vmm/rust-vmm/blob/9f7250b1555ebe022ea4133fda676e6311040738/vm-virtio/virtio-queue/src/chain.rs#L183-L190
> 
> Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
> Signed-off-by: Ruben Hakobyan <hruben@meta.com>
> ---


thanks a lot for the report. I posted a patchset for your
consideration. if you test it, pls let us know the results.
if you see a need for more changes, pls feel free to take
over the work on it.

> The wording in the spec is a bit ambiguous because it uses "chain
> length" for both descriptor counts and the total size of the memory
> ranges referenced by the descriptors. My read is that the byte limit
> applies to the sum of descriptor lengths, which is also what rust-vmm
> checks.
> 
> Another option I considered was to limit reporting capacity based
> on the largest possible block. E.g. for 64 KiB pages, the largest
> block is 512 MiB, so limit the reporting capacity to 7 to stay
> below 4GiB. Although simpler, I found that the time to reclaim
> was quite a bit longer.
> 
> I tested this patch on Cloud Hypervisor (which uses Rust-VMM)
> and QEMU (which doesn't enforce this limit).
> 
>  drivers/virtio/virtio_balloon.c | 47 ++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00..68ce627c8e04 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/balloon.h>
> +#include <linux/limits.h>
>  #include <linux/oom.h>
>  #include <linux/wait.h>
>  #include <linux/mm.h>
> @@ -207,21 +208,43 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
>  	struct virtqueue *vq = vb->reporting_vq;
>  	unsigned int unused, err;
>  
> -	/* We should always be able to add these buffers to an empty queue. */
> -	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT);
> +	while (nents) {
> +		bool unmark_end;
> +		struct scatterlist *sg_batch_start = sg;
> +		u32 batch_len = 0;
> +		unsigned int batch_nents = 0;
>  
> -	/*
> -	 * 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;
> +		/*
> +		 * Virtio spec 2.7.5.2 limits the total buffer length of a
> +		 * descriptor chain to 2^32 bytes (4 GiB), so split larger
> +		 * reports into batches no bigger than that size.
> +		 */
> +		while (batch_nents < nents &&
> +		       sg->length <= U32_MAX - batch_len) {
> +			batch_len += sg->length;
> +			batch_nents += 1;
> +			sg = sg_next(sg);
> +		}
> +		unmark_end = !sg_is_last(&sg_batch_start[batch_nents - 1]);
>  
> -	virtqueue_kick(vq);
> +		if (unmark_end)
> +			sg_mark_end(&sg_batch_start[batch_nents - 1]);
>  
> -	/* When host has read buffer, this completes via balloon_ack */
> -	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +		err = virtqueue_add_inbuf(vq, sg_batch_start, batch_nents, vb,
> +					  GFP_NOWAIT);
> +
> +		if (unmark_end)
> +			sg_unmark_end(&sg_batch_start[batch_nents - 1]);
> +
> +		if (WARN_ON_ONCE(err))
> +			return err;
> +
> +		virtqueue_kick(vq);
> +
> +		/* When host has read buffer, this completes via balloon_ack */
> +		wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +		nents -= batch_nents;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.53.0-Meta