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 <zh...@redhat.com>
Tested-by: Zhiyi Guo <zh...@redhat.com>
Signed-off-by: Peter Xu <pet...@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
