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

[Peter Maydell]

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.

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

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

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

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