On Tue, Oct 27, 2026 at 12:40 PM Michael S. Tsirkin <[email protected]> wrote:
> On Tue, Oct 27, 2026 at 05:32:04AM -0700, Ruben Hakobyan wrote:
> > On Tue, Oct 27, 2026 at 10:35 AM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
> > > > 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 <[email protected]>
> > > > > Cc: Ruben Hakobyan <[email protected]>
> > > > > Fixes: b0c504f15471 ("virtio-balloon: add support for providing free
> > > > > page reports to host")
> > > > > Cc: Alexander Duyck <[email protected]>
> > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > > ---
> > > > >
> > > > > 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?
>
> It's broken there, too. E.g. what is the "cleanup loop" in balloon?
The loop in virtballoon_free_page_report():
```
wait_event(vb->acked, ({
for (;;) {
virtqueue_disable_cb(vq);
while (virtqueue_get_buf(vq, &unused))
i++;
/* Stop waiting if device used all that we added. */
if (i >= nents)
break;
if (virtqueue_enable_cb_delayed_max(vq))
break;
}
i >= nents;
}));
```
For a broken queue, you could exit the for loop early and avoid busy looping.
It still hangs, but I think it can sleep rather than busy loop?
Anyway, I'm not sure how practical a broken queue is, so I don't have a
strong opinion.
>
> > >
> > > > 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).
>
> OK so that's just noise, there's no difference.
>
> >
> > With 64 KiB pages, it averaged 121 ms CPU (vs ~112 ms with my original
> > patch).
>
> Is this measurable, or also noise?
>
> >
> > (I tested this on Nvidia Grace)
>
>
> Thanks!
Thanks, yeah I agree and my point was that performance looks on par.
I just wanted to share some concrete numbers of what I've tested.
>
> > >
> > > --
> > > MST
> > >
>
