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

[Ruben Hakobyan <hruben@meta.com>]

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