On Tue, Oct 27, 2026 at 7:17 PM Michael S. Tsirkin <[email protected]> wrote: > On Tue, Oct 27, 2026 at 07:39:57AM -0700, Ruben Hakobyan wrote: > > 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. > > Maybe add the broken queue check explicitly then, then it does > not need to hang: > > if (virtqueue_is_broken(vq)) > i = nents; >
Ah yeah, indeed that would avoid the hang on the broken queue. > > but the issue is what if queue gets broken while we are waiting. > Hmm, yeah I see. I guess that's already outside of scope 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). > > > > > > 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. > > Thanks a lot! Thanks again for the patches and for looking into this! > > > > > > > > > > > > > > -- > > > > > MST > > > > > > > >
