RE: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: 27 July 2023 16:43
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; ricarkol@google.com;
> kvm@vger.kernel.org; Jonathan Cameron <jonathan.cameron@huawei.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH] arm/kvm: Enable support for
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
> 
> On Tue, 25 Jul 2023 at 16:01, Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > Now that we have Eager Page Split support added for ARM in the kernel[0],
> > enable it in Qemu. This adds,
> >  -eager-split-size to Qemu options to set the eager page split chunk size.
> >  -enable KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE.
> 
> It looks from the code like you've added a new sub-option
> to -accel, not a new global option. This is the right thing,
> but your commit message should document the actual option syntax
> to avoid confusion.

Ok. Will update the commit message.

> > The chunk size specifies how many pages to break at a time, using a
> > single allocation. Bigger the chunk size, more pages need to be
> > allocated ahead of time.
> >
> > Notes:
> >  - I am not sure whether we need to call kvm_vm_check_extension() for
> >    KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE or not as kernel seems to
> disable
> >    eager page size by default and it will return zero always.
> >
> >   -ToDo: Update qemu-options.hx
> >
> > [0]:
> https://lore.kernel.org/all/168426111477.3193133.1074810619984378093
> 0.b4-ty@linux.dev/
> 
> Speaking of confusion, this message says "It's an optimization used
> in Google Cloud since 2016 on x86, and for the last couple of months
> on ARM." so I'm not sure why we've ended up with an Arm-specific
> KVM_CAP and code in target/arm/kvm.c rather than something more
> generic ?
> 
> If this is going to arrive for other architectures in the future
> we should probably think about whether some of this code should
> be generic, not arm-specific.
> 
> Also this seems to be an obscure tuning parameter -- it could
> use good documentation so users have some idea when it can help.
> 
> As a more specific case of that: the kernel patchset says it
> makes Arm do the same thing that x86 already does, and split
> the huge pages automatically based on use of the dirty log.
> If the kernel can do this automatically and we never felt
> the need to provide a manual tuning knob for x86, do we even
> need to expose the Arm manual control via QEMU?

From the history of the above series, it looks like, the main argument
for making this a user adjustable knob for ARM is because of the upfront
extra memory allocations required in kernel associated with splitting the
block page. 

https://lore.kernel.org/kvmarm/86v8ktkqfx.wl-maz@kernel.org/

https://lore.kernel.org/kvmarm/86h6w70zhc.wl-maz@kernel.org/

And the knob for x86 case is a kvm module_param(eager_page_split).
Not clear to me why x86 opted for a module_param per KVM but not
per VM user space one. The discussion can be found here,
https://lore.kernel.org/kvm/YaDrmNVsXSMXR72Z@xz-m1.local/#t


> Other than that, I have a few minor coding things below.
> 
> > +static bool kvm_arm_eager_split_size_valid(uint64_t req_size, uint32_t
> sizes)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < sizeof(uint32_t) * BITS_PER_BYTE; i++) {
> > +        if (!(sizes & (1 << i))) {
> > +            continue;
> > +        }
> > +
> > +        if (req_size == (1 << i)) {
> > +            return true;
> > +        }
> > +    }
> 
> We know req_size is a power of 2 here, so if you also explicitly
> rule out 0 then you can do
>      return sizes & (1 << ctz64(req_size));
> rather than having to loop through. (Need to rule out 0
> because otherwise ctz64() returns 64 and the shift is UB.)

Yes, missed that we already handled the != power of 2 case.
Will update as per your next comment on this patch. That
is much simpler. Thanks.

> 
> > +
> > +    return false;
> > +}
> > +
> >  int kvm_arch_init(MachineState *ms, KVMState *s)
> >  {
> >      int ret = 0;
> > @@ -280,6 +298,21 @@ int kvm_arch_init(MachineState *ms, KVMState
> *s)
> >          }
> >      }
> >
> > +    if (s->kvm_eager_split_size) {
> > +        uint32_t sizes;
> > +
> > +        sizes = kvm_vm_check_extension(s,
> KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES);
> > +        if (!sizes) {
> > +            error_report("Eager Page Split not supported on host");
> > +        } else if
> (!kvm_arm_eager_split_size_valid(s->kvm_eager_split_size,
> > +                                                   sizes)) {
> > +            error_report("Eager Page Split requested chunk size not
> valid");
> > +        } else if (kvm_vm_enable_cap(s,
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
> > +                                     s->kvm_eager_split_size)) {
> > +            error_report("Failed to set Eager Page Split chunk size");
> > +        }
> > +    }
> > +
> >      kvm_arm_init_debug(s);
> >
> >      return ret;
> > @@ -1062,6 +1095,46 @@ bool
> kvm_arch_cpu_check_are_resettable(void)
> >      return true;
> >  }
> >
> > +static void kvm_arch_get_eager_split_size(Object *obj, Visitor *v,
> > +                                          const char *name, void
> *opaque,
> > +                                          Error **errp)
> > +{
> > +    KVMState *s = KVM_STATE(obj);
> > +    uint64_t value = s->kvm_eager_split_size;
> > +
> > +    visit_type_size(v, name, &value, errp);
> > +}
> > +
> > +static void kvm_arch_set_eager_split_size(Object *obj, Visitor *v,
> > +                                          const char *name, void
> *opaque,
> > +                                          Error **errp)
> > +{
> > +    KVMState *s = KVM_STATE(obj);
> > +    uint64_t value;
> > +
> > +    if (s->fd != -1) {
> > +        error_setg(errp, "Cannot set properties after the accelerator has
> been initialized");
> > +        return;
> > +    }
> > +
> > +    if (!visit_type_size(v, name, &value, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (value & (value - 1)) {
> 
> "if (!is_power_of_2(value))" is a clearer way to write this.

Ok. Will update in next.

Thanks for taking a look and sorry for late reply, was away.

Shameer

> 
> > +        error_setg(errp, "early-split-size must be a power of two.");
> > +        return;
> > +    }
> > +
> > +    s->kvm_eager_split_size = value;
> > +}
> > +
> >  void kvm_arch_accel_class_init(ObjectClass *oc)
> >  {
> > +    object_class_property_add(oc, "eager-split-size", "size",
> > +                              kvm_arch_get_eager_split_size,
> > +                              kvm_arch_set_eager_split_size, NULL,
> NULL);
> > +
> > +    object_class_property_set_description(oc, "eager-split-size",
> > +        "Configure Eager Page Split chunk size for hugepages. (default: 0,
> disabled)");
> >  }
> > --
> 
> thanks
> -- PMM