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

[Peter Xu <peterx@redhat.com>]

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