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

[Peter Xu]

On Wed, Sep 04, 2024 at 11:07:44PM +0200, David Hildenbrand wrote:
> On 04.09.24 21:16, 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.
> > 
> > Reported-by: Zhiyi Guo <zhguo@redhat.com>
> > Tested-by: Zhiyi Guo <zhguo@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> 
> 
> 
> >   {
> >       int i;
> > -    kml->slots = g_new0(KVMSlot, s->nr_slots_max);
> >       kml->as_id = as_id;
> > -    for (i = 0; i < s->nr_slots_max; i++) {
> > -        kml->slots[i].slot = i;
> > -    }
> > +    kvm_slots_grow(kml, KVM_MEMSLOTS_NUM_ALLOC_DEFAULT);
> 
> I would just keep the static initialization here, and add the additional
> 
>       kml->nr_slots_allocated = KVM_MEMSLOTS_NUM_ALLOC_DEFAULT;

IMHO it'll be cleaner to always allocate in the grow() so as to avoid
details on e.g. initializations of kml->slots[].slot above.

> 
> here.
> 
> Then, you can remove the parameter from kvm_slots_grow() completely and just 
> call it
> kvm_slots_double() and simplify a bit:
> 
> static bool kvm_slots_double(KVMMemoryListener *kml)
> {
>     unsigned int i, nr_slots_new, cur = kml->nr_slots_allocated;
>     KVMSlot *slots;
> 
>     nr_slots_new = MIN(cur * 2, kvm_state->nr_slots_max);
>     if (nr_slots_new == kvm_state->nr_slots_max) {
>         /* We reached the maximum */
>       return false;
>     }
> 
>     assert(kml->slots);
>     slots = g_renew(KVMSlot, kml->slots, nr_slots_new);
>     /*
>      * g_renew() doesn't initialize extended buffers, however kvm
>      * memslots require fields to be zero-initialized. E.g. pointers,
>      * memory_size field, etc.
>      */
>     memset(&slots[cur], 0x0, sizeof(slots[0]) * (nr_slots_new - cur));
> 
>     for (i = cur; i < nr_slots_new; i++) {
>         slots[i].slot = i;
>     }
> 
>     kml->slots = slots;
>     kml->nr_slots_allocated = nr_slots_new;
>     trace_kvm_slots_grow(cur, nr_slots_new);
> 
>     return true;
> }

Personally I still think it cleaner to allow setting whatever size.

We only have one place growing so far, which is pretty trivial to double
there, IMO.  I'll wait for a second opinion, or let me know if you have
strong feelings..

> 
> 
> Apart from that looks sane. On the slot freeing/allocation path, there is 
> certainly
> more optimization potential :)
> 
> I'm surprised this 32k loop wasn't found earlier.

Yes, it's in the range where it isn't too big to be discovered I guess, but
large enough to affect many things, so better fix it sooner than later.

This reminded me we should probably copy stable for this patch.  I think it
means I'll try to move this patch to the 1st patch to make Michael's life
and downstream easier.

-- 
Peter Xu