[PATCH 0/4] KVM: Dynamic sized memslots array

[PATCH 0/4] KVM: Dynamic sized memslots array

[Peter Xu]

This series make KVM memslots to be allocated dynamically in QEMU.  It
fixes a migration performance regression that I observed, reducing precopy
dirty sync process from ~86ms to ~3ms each time.

Patch 1,2,4 are mostly small cleanups, the major fix is in patch 3.

Thanks,

Peter Xu (4):
  KVM: Rename KVMState->nr_slots to nr_slots_max
  KVM: Define KVM_MEMSLOTS_NUM_MAX_DEFAULT
  KVM: Dynamic sized kvm memslots array
  KVM: Rename KVMMemoryListener.nr_used_slots to nr_slots_used

 include/sysemu/kvm_int.h |   7 +--
 accel/kvm/kvm-all.c      | 106 ++++++++++++++++++++++++++++++---------
 accel/kvm/trace-events   |   1 +
 3 files changed, 88 insertions(+), 26 deletions(-)

-- 
2.45.0

[PATCH 1/4] KVM: Rename KVMState->nr_slots to nr_slots_max

[Peter Xu]

This value used to reflect the maximum supported memslots from KVM kernel.
Rename it to be clearer, preparing for dynamic sized memslot allocations.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/kvm_int.h |  4 ++--
 accel/kvm/kvm-all.c      | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 1d8fb1473b..e5de43619e 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -102,8 +102,8 @@ struct KVMDirtyRingReaper {
 struct KVMState
 {
     AccelState parent_obj;
-
-    int nr_slots;
+    /* Max number of KVM slots supported */
+    int nr_slots_max;
     int fd;
     int vmfd;
     int coalesced_mmio;
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 75d11a07b2..991c389adc 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -169,7 +169,7 @@ unsigned int kvm_get_max_memslots(void)
 {
     KVMState *s = KVM_STATE(current_accel());
 
-    return s->nr_slots;
+    return s->nr_slots_max;
 }
 
 unsigned int kvm_get_free_memslots(void)
@@ -187,7 +187,7 @@ unsigned int kvm_get_free_memslots(void)
     }
     kvm_slots_unlock();
 
-    return s->nr_slots - used_slots;
+    return s->nr_slots_max - used_slots;
 }
 
 /* Called with KVMMemoryListener.slots_lock held */
@@ -196,7 +196,7 @@ static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
     KVMState *s = kvm_state;
     int i;
 
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         if (kml->slots[i].memory_size == 0) {
             return &kml->slots[i];
         }
@@ -225,7 +225,7 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
     KVMState *s = kvm_state;
     int i;
 
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         KVMSlot *mem = &kml->slots[i];
 
         if (start_addr == mem->start_addr && size == mem->memory_size) {
@@ -267,7 +267,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
     int i, ret = 0;
 
     kvm_slots_lock();
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         KVMSlot *mem = &kml->slots[i];
 
         if (ram >= mem->ram && ram < mem->ram + mem->memory_size) {
@@ -1071,7 +1071,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
 
     kvm_slots_lock();
 
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         mem = &kml->slots[i];
         /* Discard slots that are empty or do not overlap the section */
         if (!mem->memory_size ||
@@ -1720,11 +1720,11 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage)
     kvm_dirty_ring_flush();
 
     /*
-     * TODO: make this faster when nr_slots is big while there are
+     * TODO: make this faster when nr_slots_max is big while there are
      * only a few used slots (small VMs).
      */
     kvm_slots_lock();
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         mem = &kml->slots[i];
         if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
             kvm_slot_sync_dirty_pages(mem);
@@ -1839,10 +1839,10 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
 {
     int i;
 
-    kml->slots = g_new0(KVMSlot, s->nr_slots);
+    kml->slots = g_new0(KVMSlot, s->nr_slots_max);
     kml->as_id = as_id;
 
-    for (i = 0; i < s->nr_slots; i++) {
+    for (i = 0; i < s->nr_slots_max; i++) {
         kml->slots[i].slot = i;
     }
 
@@ -2454,11 +2454,11 @@ static int kvm_init(MachineState *ms)
         (kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE);
 
     kvm_immediate_exit = kvm_check_extension(s, KVM_CAP_IMMEDIATE_EXIT);
-    s->nr_slots = kvm_check_extension(s, KVM_CAP_NR_MEMSLOTS);
+    s->nr_slots_max = kvm_check_extension(s, KVM_CAP_NR_MEMSLOTS);
 
     /* If unspecified, use the default value */
-    if (!s->nr_slots) {
-        s->nr_slots = 32;
+    if (!s->nr_slots_max) {
+        s->nr_slots_max = 32;
     }
 
     s->nr_as = kvm_check_extension(s, KVM_CAP_MULTI_ADDRESS_SPACE);
-- 
2.45.0

[PATCH 2/4] KVM: Define KVM_MEMSLOTS_NUM_MAX_DEFAULT

[Peter Xu]

Make the default max nr_slots a macro, it's only used when KVM reports
nothing.  Then we put all the rest macros together later soon.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/kvm/kvm-all.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 991c389adc..e408dbb753 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -69,6 +69,9 @@
 #define KVM_GUESTDBG_BLOCKIRQ 0
 #endif
 
+/* Default max allowed memslots if kernel reported nothing */
+#define  KVM_MEMSLOTS_NUM_MAX_DEFAULT                       32
+
 struct KVMParkedVcpu {
     unsigned long vcpu_id;
     int kvm_fd;
@@ -2458,7 +2461,7 @@ static int kvm_init(MachineState *ms)
 
     /* If unspecified, use the default value */
     if (!s->nr_slots_max) {
-        s->nr_slots_max = 32;
+        s->nr_slots_max = KVM_MEMSLOTS_NUM_MAX_DEFAULT;
     }
 
     s->nr_as = kvm_check_extension(s, KVM_CAP_MULTI_ADDRESS_SPACE);
-- 
2.45.0

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

[Peter Xu]

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>
---
 include/sysemu/kvm_int.h |  1 +
 accel/kvm/kvm-all.c      | 87 +++++++++++++++++++++++++++++++++-------
 accel/kvm/trace-events   |  1 +
 3 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index e5de43619e..e67b2e5a68 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -46,6 +46,7 @@ typedef struct KVMMemoryListener {
     MemoryListener listener;
     KVMSlot *slots;
     unsigned int nr_used_slots;
+    unsigned int nr_slots_allocated;
     int as_id;
     QSIMPLEQ_HEAD(, KVMMemoryUpdate) transaction_add;
     QSIMPLEQ_HEAD(, KVMMemoryUpdate) transaction_del;
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e408dbb753..0d379606e4 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -71,6 +71,8 @@
 
 /* Default max allowed memslots if kernel reported nothing */
 #define  KVM_MEMSLOTS_NUM_MAX_DEFAULT                       32
+/* Default num of memslots to be allocated when VM starts */
+#define  KVM_MEMSLOTS_NUM_ALLOC_DEFAULT                     16
 
 struct KVMParkedVcpu {
     unsigned long vcpu_id;
@@ -168,6 +170,52 @@ void kvm_resample_fd_notify(int gsi)
     }
 }
 
+/**
+ * kvm_slots_grow(): Grow the slots[] array in the KVMMemoryListener
+ *
+ * @kml: The KVMMemoryListener* to grow the slots[] array
+ * @nr_slots_new: The new size of slots[] array
+ *
+ * Returns: True if the array grows larger, false otherwise.
+ */
+static bool kvm_slots_grow(KVMMemoryListener *kml, unsigned int nr_slots_new)
+{
+    unsigned int i, cur = kml->nr_slots_allocated;
+    KVMSlot *slots;
+
+    if (nr_slots_new > kvm_state->nr_slots_max) {
+        nr_slots_new = kvm_state->nr_slots_max;
+    }
+
+    if (cur >= nr_slots_new) {
+        /* Big enough, no need to grow, or we reached max */
+        return false;
+    }
+
+    if (cur == 0) {
+        slots = g_new0(KVMSlot, nr_slots_new);
+    } else {
+        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;
+}
+
 unsigned int kvm_get_max_memslots(void)
 {
     KVMState *s = KVM_STATE(current_accel());
@@ -196,15 +244,20 @@ unsigned int kvm_get_free_memslots(void)
 /* Called with KVMMemoryListener.slots_lock held */
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
-    KVMState *s = kvm_state;
     int i;
 
-    for (i = 0; i < s->nr_slots_max; i++) {
+retry:
+    for (i = 0; i < kml->nr_slots_allocated; i++) {
         if (kml->slots[i].memory_size == 0) {
             return &kml->slots[i];
         }
     }
 
+    /* If no free slots, try to grow first by doubling */
+    if (kvm_slots_grow(kml, kml->nr_slots_allocated * 2)) {
+        goto retry;
+    }
+
     return NULL;
 }
 
@@ -225,10 +278,9 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
                                          hwaddr start_addr,
                                          hwaddr size)
 {
-    KVMState *s = kvm_state;
     int i;
 
-    for (i = 0; i < s->nr_slots_max; i++) {
+    for (i = 0; i < kml->nr_slots_allocated; i++) {
         KVMSlot *mem = &kml->slots[i];
 
         if (start_addr == mem->start_addr && size == mem->memory_size) {
@@ -270,7 +322,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
     int i, ret = 0;
 
     kvm_slots_lock();
-    for (i = 0; i < s->nr_slots_max; i++) {
+    for (i = 0; i < kml->nr_slots_allocated; i++) {
         KVMSlot *mem = &kml->slots[i];
 
         if (ram >= mem->ram && ram < mem->ram + mem->memory_size) {
@@ -1074,7 +1126,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
 
     kvm_slots_lock();
 
-    for (i = 0; i < s->nr_slots_max; i++) {
+    for (i = 0; i < kml->nr_slots_allocated; i++) {
         mem = &kml->slots[i];
         /* Discard slots that are empty or do not overlap the section */
         if (!mem->memory_size ||
@@ -1722,12 +1774,8 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage)
     /* Flush all kernel dirty addresses into KVMSlot dirty bitmap */
     kvm_dirty_ring_flush();
 
-    /*
-     * TODO: make this faster when nr_slots_max is big while there are
-     * only a few used slots (small VMs).
-     */
     kvm_slots_lock();
-    for (i = 0; i < s->nr_slots_max; i++) {
+    for (i = 0; i < kml->nr_slots_allocated; i++) {
         mem = &kml->slots[i];
         if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
             kvm_slot_sync_dirty_pages(mem);
@@ -1842,12 +1890,9 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
 {
     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);
 
     QSIMPLEQ_INIT(&kml->transaction_add);
     QSIMPLEQ_INIT(&kml->transaction_del);
@@ -2464,6 +2509,18 @@ static int kvm_init(MachineState *ms)
         s->nr_slots_max = KVM_MEMSLOTS_NUM_MAX_DEFAULT;
     }
 
+    /*
+     * A VM will at least require a few memslots to work, or it can even
+     * fail to boot.  Make sure the supported value is always at least
+     * larger than what we will initially allocate.
+     */
+    if (s->nr_slots_max < KVM_MEMSLOTS_NUM_ALLOC_DEFAULT) {
+        ret = -EINVAL;
+        fprintf(stderr, "KVM max supported number of slots (%d) too small\n",
+                s->nr_slots_max);
+        goto err;
+    }
+
     s->nr_as = kvm_check_extension(s, KVM_CAP_MULTI_ADDRESS_SPACE);
     if (s->nr_as <= 1) {
         s->nr_as = 1;
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 37626c1ac5..ad2ae6fca5 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -36,3 +36,4 @@ kvm_io_window_exit(void) ""
 kvm_run_exit_system_event(int cpu_index, uint32_t event_type) "cpu_index %d, system_even_type %"PRIu32
 kvm_convert_memory(uint64_t start, uint64_t size, const char *msg) "start 0x%" PRIx64 " size 0x%" PRIx64 " %s"
 kvm_memory_fault(uint64_t start, uint64_t size, uint64_t flags) "start 0x%" PRIx64 " size 0x%" PRIx64 " flags 0x%" PRIx64
+kvm_slots_grow(unsigned int old, unsigned int new) "%u -> %u"
-- 
2.45.0

[PATCH 4/4] KVM: Rename KVMMemoryListener.nr_used_slots to nr_slots_used

[Peter Xu]

This will make all nr_slots counters to be named in the same manner.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/kvm_int.h | 2 +-
 accel/kvm/kvm-all.c      | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index e67b2e5a68..2c57194b6b 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -45,7 +45,7 @@ typedef struct KVMMemoryUpdate {
 typedef struct KVMMemoryListener {
     MemoryListener listener;
     KVMSlot *slots;
-    unsigned int nr_used_slots;
+    unsigned int nr_slots_used;
     unsigned int nr_slots_allocated;
     int as_id;
     QSIMPLEQ_HEAD(, KVMMemoryUpdate) transaction_add;
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0d379606e4..0990d090cb 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -234,7 +234,7 @@ unsigned int kvm_get_free_memslots(void)
         if (!s->as[i].ml) {
             continue;
         }
-        used_slots = MAX(used_slots, s->as[i].ml->nr_used_slots);
+        used_slots = MAX(used_slots, s->as[i].ml->nr_slots_used);
     }
     kvm_slots_unlock();
 
@@ -1505,7 +1505,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             }
             start_addr += slot_size;
             size -= slot_size;
-            kml->nr_used_slots--;
+            kml->nr_slots_used--;
         } while (size);
         return;
     }
@@ -1544,7 +1544,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         ram_start_offset += slot_size;
         ram += slot_size;
         size -= slot_size;
-        kml->nr_used_slots++;
+        kml->nr_slots_used++;
     } while (size);
 }
 
-- 
2.45.0

Re: [PATCH 1/4] KVM: Rename KVMState->nr_slots to nr_slots_max

[David Hildenbrand]

On 04.09.24 21:16, Peter Xu wrote:
> This value used to reflect the maximum supported memslots from KVM kernel.
> Rename it to be clearer, preparing for dynamic sized memslot allocations.

Well, it matches "KVM_CAP_NR_MEMSLOTS" :)

Reviewed-by: David Hildenbrand <david@redhat.com>

> -    for (i = 0; i < s->nr_slots; i++) {
> +    for (i = 0; i < s->nr_slots_max; i++) {
>           mem = &kml->slots[i];
>           /* Discard slots that are empty or do not overlap the section */
>           if (!mem->memory_size ||
> @@ -1720,11 +1720,11 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage)
>       kvm_dirty_ring_flush();
>   
>       /*
> -     * TODO: make this faster when nr_slots is big while there are
> +     * TODO: make this faster when nr_slots_max is big while there are
>        * only a few used slots (small VMs).
>        */

Interesting TODO!

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] KVM: Define KVM_MEMSLOTS_NUM_MAX_DEFAULT

[David Hildenbrand]

On 04.09.24 21:16, Peter Xu wrote:
> Make the default max nr_slots a macro, it's only used when KVM reports
> nothing.  Then we put all the rest macros together later soon.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   accel/kvm/kvm-all.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 991c389adc..e408dbb753 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -69,6 +69,9 @@
>   #define KVM_GUESTDBG_BLOCKIRQ 0
>   #endif
>   
> +/* Default max allowed memslots if kernel reported nothing */
> +#define  KVM_MEMSLOTS_NUM_MAX_DEFAULT                       32
> +

Any reason for the "NUM" vs. "NR" in there?

Something that resembles KVM_CAP_NR_MEMSLOTS would be a bit ore 
consistent, because the 32 is essentially the fallback if the capability 
is not supported.

Apart from that makes sense

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 4/4] KVM: Rename KVMMemoryListener.nr_used_slots to nr_slots_used

[David Hildenbrand]

On 04.09.24 21:16, Peter Xu wrote:
> This will make all nr_slots counters to be named in the same manner.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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.

Wouldn't it be sufficient to limit the walk to the actually used slots?

I know, the large allocation might sound scary at first, but memory 
overcommit+populate-on-demand should handle that, assuming nobody 
touches the yet-unused slots.

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

On 04.09.24 22:43, 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.
> 
> Wouldn't it be sufficient to limit the walk to the actually used slots?

Ah, I remember that kvm_get_free_slot() is also rather inefficient 
because we don't "close holes" when removing slots. So we would have to 
walk up to the "highest slot ever used". Let me take a look at the patch.

-- 
Cheers,

David / dhildenb

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

[Peter Xu]

On Wed, Sep 04, 2024 at 10:43:23PM +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.
> 
> Wouldn't it be sufficient to limit the walk to the actually used slots?
> 
> I know, the large allocation might sound scary at first, but memory
> overcommit+populate-on-demand should handle that, assuming nobody touches
> the yet-unused slots.

I thought we can have holes within the array?

I meant e.g. when 10 slots populated, but then one of them got removed,
then nr_slots_used would be 9 even if slots[9] is still in use?

Thanks,

-- 
Peter Xu

Re: [PATCH 2/4] KVM: Define KVM_MEMSLOTS_NUM_MAX_DEFAULT

[Peter Xu]

On Wed, Sep 04, 2024 at 10:39:19PM +0200, David Hildenbrand wrote:
> On 04.09.24 21:16, Peter Xu wrote:
> > Make the default max nr_slots a macro, it's only used when KVM reports
> > nothing.  Then we put all the rest macros together later soon.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   accel/kvm/kvm-all.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 991c389adc..e408dbb753 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -69,6 +69,9 @@
> >   #define KVM_GUESTDBG_BLOCKIRQ 0
> >   #endif
> > +/* Default max allowed memslots if kernel reported nothing */
> > +#define  KVM_MEMSLOTS_NUM_MAX_DEFAULT                       32
> > +
> 
> Any reason for the "NUM" vs. "NR" in there?

Sure, let me do s/NUM/NR/ all over.

> 
> Something that resembles KVM_CAP_NR_MEMSLOTS would be a bit ore consistent,
> because the 32 is essentially the fallback if the capability is not
> supported.
> 
> Apart from that makes sense
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks,

-- 
Peter Xu

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

[David Hildenbrand]

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

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

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

[David Hildenbrand]

>>
>> 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.

Why would one need that? If any, at some point we would want to shrink 
or rather "compact".

> 
> 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..

I think the simplicity of kvm_slots_double() speaks for itself, but I 
won't fight for it.

-- 
Cheers,

David / dhildenb

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

[Peter Xu]

On Wed, Sep 04, 2024 at 11:23:33PM +0200, David Hildenbrand wrote:
> 
> > > 
> > > 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.
> 
> Why would one need that? If any, at some point we would want to shrink or
> rather "compact".
> 
> > 
> > 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..
> 
> I think the simplicity of kvm_slots_double() speaks for itself, but I won't
> fight for it.

Using kvm_slots_double() won't be able to share the same code when
initialize (to e.g. avoid hard-coded initialize of "slots[i].slot").

Thanks,

-- 
Peter Xu

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

[David Hildenbrand]

On 04.09.24 23:34, Peter Xu wrote:
> On Wed, Sep 04, 2024 at 11:23:33PM +0200, David Hildenbrand wrote:
>>
>>>>
>>>> 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.
>>
>> Why would one need that? If any, at some point we would want to shrink or
>> rather "compact".
>>
>>>
>>> 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..
>>
>> I think the simplicity of kvm_slots_double() speaks for itself, but I won't
>> fight for it.
> 
> Using kvm_slots_double() won't be able to share the same code when
> initialize (to e.g. avoid hard-coded initialize of "slots[i].slot").

I don't see that as any problem and if you really care you could factor 
exactly that part out in a helper. Anyhow, I learned that I am not good 
at convincing you, so do what you think is best. The code itself should 
get the job done.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

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

[Peter Xu]

On Wed, Sep 04, 2024 at 11:38:28PM +0200, David Hildenbrand wrote:
> On 04.09.24 23:34, Peter Xu wrote:
> > On Wed, Sep 04, 2024 at 11:23:33PM +0200, David Hildenbrand wrote:
> > > 
> > > > > 
> > > > > 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.
> > > 
> > > Why would one need that? If any, at some point we would want to shrink or
> > > rather "compact".
> > > 
> > > > 
> > > > 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..
> > > 
> > > I think the simplicity of kvm_slots_double() speaks for itself, but I won't
> > > fight for it.
> > 
> > Using kvm_slots_double() won't be able to share the same code when
> > initialize (to e.g. avoid hard-coded initialize of "slots[i].slot").
> 
> I don't see that as any problem and if you really care you could factor
> exactly that part out in a helper. Anyhow, I learned that I am not good at
> convincing you, so do what you think is best. The code itself should get the
> job done.

It's only about that's the simplest for all of us, and I noticed it only
because I already planned to switch to kvm_slots_double(); that's normally
what I do when I don't strongly insist something. So you succeeded already
making me go there. :)

It's just that as you said it either requires more changes, or I'll need to
duplicate some code which I want to avoid.

> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks a lot for the late night reviews, David.  I'll attach all your tags
when repost, though just to mention there'll be slight touch ups here and
there due to reordering.  Feel free to double check when it's there.

-- 
Peter Xu

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

[Peter Xu]

On Wed, Sep 04, 2024 at 05:46:55PM -0400, Peter Xu wrote:
> On Wed, Sep 04, 2024 at 11:38:28PM +0200, David Hildenbrand wrote:
> > On 04.09.24 23:34, Peter Xu wrote:
> > > On Wed, Sep 04, 2024 at 11:23:33PM +0200, David Hildenbrand wrote:
> > > > 
> > > > > > 
> > > > > > 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.
> > > > 
> > > > Why would one need that? If any, at some point we would want to shrink or
> > > > rather "compact".
> > > > 
> > > > > 
> > > > > 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..
> > > > 
> > > > I think the simplicity of kvm_slots_double() speaks for itself, but I won't
> > > > fight for it.
> > > 
> > > Using kvm_slots_double() won't be able to share the same code when
> > > initialize (to e.g. avoid hard-coded initialize of "slots[i].slot").
> > 
> > I don't see that as any problem and if you really care you could factor
> > exactly that part out in a helper. Anyhow, I learned that I am not good at
> > convincing you, so do what you think is best. The code itself should get the
> > job done.
> 
> It's only about that's the simplest for all of us, and I noticed it only
> because I already planned to switch to kvm_slots_double(); that's normally
> what I do when I don't strongly insist something. So you succeeded already
> making me go there. :)
> 
> It's just that as you said it either requires more changes, or I'll need to
> duplicate some code which I want to avoid.
> 
> > 
> > Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks a lot for the late night reviews, David.  I'll attach all your tags
> when repost, though just to mention there'll be slight touch ups here and
> there due to reordering.  Feel free to double check when it's there.

So I plan to squash this in, assuming this looks better to you:

===8<===
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 78f2d8b80f..020fd16ab8 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -216,6 +216,11 @@ static bool kvm_slots_grow(KVMMemoryListener *kml, unsigned int nr_slots_new)
     return true;
 }
 
+static bool kvm_slots_double(KVMMemoryListener *kml)
+{
+    return kvm_slots_grow(kml, kml->nr_slots_allocated * 2);
+}
+
 unsigned int kvm_get_max_memslots(void)
 {
     KVMState *s = KVM_STATE(current_accel());
@@ -254,7 +259,7 @@ retry:
     }
 
     /* If no free slots, try to grow first by doubling */
-    if (kvm_slots_grow(kml, kml->nr_slots_allocated * 2)) {
+    if (kvm_slots_double(kml)) {
         goto retry;
     }
===8<===

Please let me know if otherwise.

-- 
Peter Xu