Re: [PATCH v2] KVM: make uevents configurable

[Marc Zyngier <maz@kernel.org>]

On Mon, 02 Dec 2024 09:06:28 +0000,
Bernhard Kauer <bk@alpico.io> wrote:
> 
> Handling of uevents in userlevel is a bottleneck for tiny VMs.
> 
> Running 10_000 VMs keeps one and a half cores busy for 5.4 seconds to let
> systemd-udevd handle all messages.  That is roughly 27x longer than
> the 0.2 seconds needed for running the VMs without them.
> 
> We choose a module parameter here due to its simplicity and ease of
> maintenance.
> 
> v1->v2:  make the parameter read-write to avoid reboots on ARM

That's also to avoid killing all VMs on *any* architecture, modular or
not, just to switch a parameter.

Also, please don't include this sort of changelogs in the commit
message...

> 
> Signed-off-by: Bernhard Kauer <bk@alpico.io>
> ---

... but instead place them *here*.

>  virt/kvm/kvm_main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 38620c16739b..9e714cf45617 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -97,6 +97,9 @@ EXPORT_SYMBOL_GPL(halt_poll_ns_shrink);
>  bool debugfs_per_vm = true;
>  module_param(debugfs_per_vm, bool, 0644);
>  
> +bool disable_uevent_notify;

static?

> +module_param(disable_uevent_notify, bool, 0644);
> +
>  /*
>   * Allow direct access (from KVM or the CPU) without MMU notifier protection
>   * to unpinned pages.
> @@ -6141,7 +6144,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
>  	struct kobj_uevent_env *env;
>  	unsigned long long created, active;
>  
> -	if (!kvm_dev.this_device || !kvm)
> +	if (!kvm_dev.this_device || !kvm || disable_uevent_notify)
>  		return;
>  
>  	mutex_lock(&kvm_lock);

<bikeshed-time>
I'd rather have a positive logic. Something like:

+static bool uevent_notify = true;
+module_param(uevent_notify, bool, 0644);
+
 /*
  * Allow direct access (from KVM or the CPU) without MMU notifier protection
  * to unpinned pages.
@@ -6141,7 +6144,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
 	struct kobj_uevent_env *env;
 	unsigned long long created, active;
 
-	if (!kvm_dev.this_device || !kvm)
+	if (!kvm_dev.this_device || !kvm || !uevent_notify)
 		return;
 
 	mutex_lock(&kvm_lock);

which is overall more readable.
</bikeshed-time>

I would also expect some form of documentation in
Documentation/admin-guide/kernel-parameters.txt.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.