On Tue, 25 Jul 2023 at 16:01, Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> 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@xxxxxxxxx/ 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
