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

[Peter Xu <peterx@redhat.com>]

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