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

[David Hildenbrand <david@redhat.com>]

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;

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;
}


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.

-- 
Cheers,

David / dhildenb