Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[Ruben Hakobyan <hruben@meta.com>]

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
>