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

[Ruben Hakobyan <hruben@meta.com>]

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!