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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx - 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@xxxxxxxxxx/
> >
> > 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.