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