Re: [PATCH v4 1/4] KVM: Dynamic sized kvm memslots array

[Michael Tokarev]

17.09.2024 19:38, Peter Xu wrote:
> Zhiyi reported an infinite loop issue in VFIO use case.  The cause of that
> was a separate discussion, however during that I found a regression of
> dirty sync slowness when profiling.
>
> Each KVMMemoryListerner maintains an array of kvm memslots.  Currently it's
> statically allocated to be the max supported by the kernel.  However after
> Linux commit 4fc096a99e ("KVM: Raise the maximum number of user memslots"),
> the max supported memslots reported now grows to some number large enough
> so that it may not be wise to always statically allocate with the max
> reported.
>
> What's worse, QEMU kvm code still walks all the allocated memslots entries
> to do any form of lookups.  It can drastically slow down all memslot
> operations because each of such loop can run over 32K times on the new
> kernels.
>
> Fix this issue by making the memslots to be allocated dynamically.
>
> Here the initial size was set to 16 because it should cover the basic VM
> usages, so that the hope is the majority VM use case may not even need to
> grow at all (e.g. if one starts a VM with ./qemu-system-x86_64 by default
> it'll consume 9 memslots), however not too large to waste memory.
>
> There can also be even better way to address this, but so far this is the
> simplest and should be already better even than before we grow the max
> supported memslots.  For example, in the case of above issue when VFIO was
> attached on a 32GB system, there are only ~10 memslots used.  So it could
> be good enough as of now.
>
> In the above VFIO context, measurement shows that the precopy dirty sync
> shrinked from ~86ms to ~3ms after this patch applied.  It should also apply
> to any KVM enabled VM even without VFIO.
>
> NOTE: we don't have a FIXES tag for this patch because there's no real
> commit that regressed this in QEMU. Such behavior existed for a long time,
> but only start to be a problem when the kernel reports very large
> nr_slots_max value.  However that's pretty common now (the kernel change
> was merged in 2021) so we attached cc:stable because we'll want this change
> to be backported to stable branches.

Looking at this from qemu-stable PoV, I'm not 100% sure this change is good
for stable-7.2 series, because 7.2 lacks v8.1.0-1571-g5b23186a95
"kvm: Return number of free memslots" commit, which was a preparation for
for memory devices that consume multiple memslots.

I did a backport of this change (currently it is at the tip of staging-7.2
branch of https://gitlab.com/mjt0k/qemu.git) - I had to tweak context and
also to remove now-unused local variable in kvm-all.c.  It builds and the
tests run fine, but I'm not really sure it does what it is intended to do.

Should anything else be picked up for 7.2 for all this to work, or should
this change not be back-ported to 7.2 ?

(for more recent releases, everything looks ok).

Thanks,

/mjt