[PATCH] KVM: x86: Make the debugfs per VM optional

[PATCH] KVM: x86: Make the debugfs per VM optional

[Bernhard Kauer]

Creating a debugfs directory for each virtual machine is a suprisingly
costly operation as one has to synchronize multiple cores. However, short
living VMs seldom benefit from it.

Since there are valid use-cases we make this feature optional via a
module parameter. Disabling it saves 150us in the hello microbenchmark.

Signed-off-by: Bernhard Kauer <bk@alpico.io>
---
 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 a48861363649..760e39cf86a8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -94,6 +94,9 @@ unsigned int halt_poll_ns_shrink = 2;
 module_param(halt_poll_ns_shrink, uint, 0644);
 EXPORT_SYMBOL_GPL(halt_poll_ns_shrink);
 
+bool debugfs_per_vm = true;
+module_param(debugfs_per_vm, bool, 0644);
+
 /*
  * Ordering of locks:
  *
@@ -1050,7 +1053,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 	int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc +
 				      kvm_vcpu_stats_header.num_desc;
 
-	if (!debugfs_initialized())
+	if (!debugfs_initialized() || !debugfs_per_vm)
 		return 0;
 
 	snprintf(dir_name, sizeof(dir_name), "%d-%s", task_pid_nr(current), fdname);
-- 
2.45.2

Re: [PATCH] KVM: x86: Make the debugfs per VM optional

[Sean Christopherson]

+Paolo and others

Please use scripts/get_maintainer.pl, otherwise your patches are likely to be
missed by key folks (Paolo, in this case).

On Wed, Oct 23, 2024, Bernhard Kauer wrote:
> Creating a debugfs directory for each virtual machine is a suprisingly
> costly operation as one has to synchronize multiple cores. However, short
> living VMs seldom benefit from it.
> 
> Since there are valid use-cases we make this feature optional via a
> module parameter. Disabling it saves 150us in the hello microbenchmark.
> 
> Signed-off-by: Bernhard Kauer <bk@alpico.io>
> ---
>  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 a48861363649..760e39cf86a8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -94,6 +94,9 @@ unsigned int halt_poll_ns_shrink = 2;
>  module_param(halt_poll_ns_shrink, uint, 0644);
>  EXPORT_SYMBOL_GPL(halt_poll_ns_shrink);
>  
> +bool debugfs_per_vm = true;
> +module_param(debugfs_per_vm, bool, 0644);

I'm not opposed to letting userspace say "no debugfs for me", but I don't know
that a module param is the right way to go.  It's obviously quite easy to
implement and maintain (in code), but I'm mildly concerned that it'll have limited
usefulness and/or lead to bad user experiences, e.g. because people turn off debugfs
for startup latency without entirely realizing what they're sacrificing.

One potentially terrible idea would be to setup debugfs asynchronously, so that
the VM is runnable asap, but userspace still gets full debugfs information.  The
two big wrinkles would be the vCPU debugfs creation and kvm_uevent_notify_change()
(or at least the STATS_PATH event) would both need to be asynchronous as well.

Re: [PATCH] KVM: x86: Make the debugfs per VM optional

[Oliver Upton]

On Fri, Nov 01, 2024 at 10:15:00AM -0700, Sean Christopherson wrote:
> On Wed, Oct 23, 2024, Bernhard Kauer wrote:
> > Creating a debugfs directory for each virtual machine is a suprisingly
> > costly operation as one has to synchronize multiple cores. However, short
> > living VMs seldom benefit from it.
> > 
> > Since there are valid use-cases we make this feature optional via a
> > module parameter. Disabling it saves 150us in the hello microbenchmark.
> > 
> > Signed-off-by: Bernhard Kauer <bk@alpico.io>
> > ---
> >  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 a48861363649..760e39cf86a8 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -94,6 +94,9 @@ unsigned int halt_poll_ns_shrink = 2;
> >  module_param(halt_poll_ns_shrink, uint, 0644);
> >  EXPORT_SYMBOL_GPL(halt_poll_ns_shrink);
> >  
> > +bool debugfs_per_vm = true;
> > +module_param(debugfs_per_vm, bool, 0644);
> 
> I'm not opposed to letting userspace say "no debugfs for me", but I don't know
> that a module param is the right way to go.  It's obviously quite easy to
> implement and maintain (in code), but I'm mildly concerned that it'll have limited
> usefulness and/or lead to bad user experiences, e.g. because people turn off debugfs
> for startup latency without entirely realizing what they're sacrificing.

I'd be open to a Kconfig option that disables only KVM debugfs, assuming
there are people out there who want that *and* still need the rest of
debugfs facilities.

Even assuming well-intentioned userspace, a defensive user might want to
hide KVM's debugfs surfaces in case it exposed customer data.

Otherwise !CONFIG_DEBUG_FS would get the job done.

> One potentially terrible idea would be to setup debugfs asynchronously, so that
> the VM is runnable asap, but userspace still gets full debugfs information.  The
> two big wrinkles would be the vCPU debugfs creation and kvm_uevent_notify_change()
> (or at least the STATS_PATH event) would both need to be asynchronous as well.

Sounds like a pile o' bugs waiting to happen in a rather gently tested
part of the KVM, so hopefully we don't need to consider that route :)

-- 
Thanks,
Oliver

Re: [PATCH] KVM: x86: Make the debugfs per VM optional

[Paolo Bonzini]

On Fri, Nov 1, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote:
> I'm not opposed to letting userspace say "no debugfs for me", but I don't know
> that a module param is the right way to go.  It's obviously quite easy to
> implement and maintain (in code), but I'm mildly concerned that it'll have limited
> usefulness and/or lead to bad user experiences, e.g. because people turn off debugfs
> for startup latency without entirely realizing what they're sacrificing.

What are they sacrificing? :) The per-VM statistics information is
also accessible without debugfs, even though kvm_stat does not support
it.

However I'd make the module parameter read-only, so you don't have
half-and-half setups. And maybe even in this mode we should create the
directory anyway to hold the vcpu%d/pid files, which are not
accessible in other ways.

> One potentially terrible idea would be to setup debugfs asynchronously, so that
> the VM is runnable asap, but userspace still gets full debugfs information.  The
> two big wrinkles would be the vCPU debugfs creation and kvm_uevent_notify_change()
> (or at least the STATS_PATH event) would both need to be asynchronous as well.

STATS_PATH is easy because you can create the toplevel directory
synchronously; same for vCPUs. I'd be willing to at least see what a
patch looks like.

Paolo

Re: [PATCH] KVM: x86: Make the debugfs per VM optional

[Sean Christopherson]

On Thu, Nov 07, 2024, Paolo Bonzini wrote:
> On Fri, Nov 1, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote:
> > I'm not opposed to letting userspace say "no debugfs for me", but I don't know
> > that a module param is the right way to go.  It's obviously quite easy to
> > implement and maintain (in code), but I'm mildly concerned that it'll have limited
> > usefulness and/or lead to bad user experiences, e.g. because people turn off debugfs
> > for startup latency without entirely realizing what they're sacrificing.
> 
> What are they sacrificing? :)

For all intents and purposes, the ability to get an per-VM and per-vCPU information
from an arbitrary shell.

> The per-VM statistics information is also accessible without debugfs, even
> though kvm_stat does not support it.

I assume you're referring to KVM_GET_STATS_FD?  That's not easy to get at from
the shell.

If a host is running a single VM, then the per-VM directories aren't needed.  But
I would be very, very surprised if there's a legitimate use case for running a
single VM, with debugfs, that cares deeply about the boot latency of that one VM.

FWIW, I would be wholeheartedly in favor of providing tooling to get at stats
via KVM_GET_STATS_FD, e.g. given a VM's PID.  But then I think it would make sense
to have CONFIG_KVM_DEBUGFS, not a module param.

> However I'd make the module parameter read-only, so you don't have
> half-and-half setups. And maybe even in this mode we should create the
> directory anyway to hold the vcpu%d/pid files, which are not
> accessible in other ways.
> 
> > One potentially terrible idea would be to setup debugfs asynchronously, so that
> > the VM is runnable asap, but userspace still gets full debugfs information.  The
> > two big wrinkles would be the vCPU debugfs creation and kvm_uevent_notify_change()
> > (or at least the STATS_PATH event) would both need to be asynchronous as well.
> 
> STATS_PATH is easy because you can create the toplevel directory
> synchronously; same for vCPUs. I'd be willing to at least see what a
> patch looks like.

Ah, creating the directories synchrously would definitely simplify things.