> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@xxxxxxxxxx] > Sent: 27 July 2023 16:43 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx> > Cc: qemu-devel@xxxxxxxxxx; qemu-arm@xxxxxxxxxx; ricarkol@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>; > Linuxarm <linuxarm@xxxxxxxxxx> > 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@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. 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@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? >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@xxxxxxxxxx/ https://lore.kernel.org/kvmarm/86h6w70zhc.wl-maz@xxxxxxxxxx/ 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
