kvm: no need to check return value of debugfs_create functions

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 virt/kvm/kvm_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

virt/kvm/kvm_main.c +1 -3

index 5ecea812cb6a..4f96450ecdfc 100644
@@ -2528,9 +2528,7 @@ static int kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 
 	snprintf(dir_name, sizeof(dir_name), "vcpu%d", vcpu->vcpu_id);
 	vcpu->debugfs_dentry = debugfs_create_dir(dir_name,
-								vcpu->kvm->debugfs_dentry);
-	if (!vcpu->debugfs_dentry)
-		return -ENOMEM;
+						  vcpu->kvm->debugfs_dentry);
 
 	ret = kvm_arch_create_vcpu_debugfs(vcpu);
 	if (ret < 0) {

Signature · 1 line

2.20.1

Quoted 3 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

What about wanting to make the debugfs all-or-nothing? That seems like a legitimate usage of checking the return value.

E.g. KVM removes the debugfs if kvm_arch_create_vcpu_debugfs() fails, and the arch/x86/kvm/debugfs.c implementation of kvm_arch_create_vcpu_debugfs() returns an error if any of its debugfs_create_file() calls fail.

If you're adamant about removing all debugfs create return value checks, the aforementioned debugfs_create_file() calls should also be removed. And at that point kvm_create_vcpu_debugfs() should have a 'void' return value.

Quoted 12 lines · Sean Christopherson

Quoted 3 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

What about wanting to make the debugfs all-or-nothing? That seems like a legitimate usage of checking the return value.

E.g. KVM removes the debugfs if kvm_arch_create_vcpu_debugfs() fails, and the arch/x86/kvm/debugfs.c implementation of kvm_arch_create_vcpu_debugfs() returns an error if any of its debugfs_create_file() calls fail.

Quoted 4 lines

If you're adamant about removing all debugfs create return value checks, the aforementioned debugfs_create_file() calls should also be removed. And at that point kvm_create_vcpu_debugfs() should have a 'void' return value.

Belatedly saw the other series. It'll require a bit more coordination, but folding this into the x86 series would allow for converting the KVM call stack to have 'void' returns.

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 virt/kvm/kvm_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

virt/kvm/kvm_main.c +0 -0

index 5ecea812cb6a..4f96450ecdfc 100644

Quoted 18 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 virt/kvm/kvm_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

virt/kvm/kvm_main.c +0 -0

index 5ecea812cb6a..4f96450ecdfc 100644

virt/kvm/kvm_main.c static int kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu) · line 2534

Quoted 10 lines

virt/kvm/kvm_main.c +1 -3

@@ -2528,9 +2528,7 @@ static int kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 
 	snprintf(dir_name, sizeof(dir_name), "vcpu%d", vcpu->vcpu_id);
 	vcpu->debugfs_dentry = debugfs_create_dir(dir_name,
-								vcpu->kvm->debugfs_dentry);
-	if (!vcpu->debugfs_dentry)
-		return -ENOMEM;
+						  vcpu->kvm->debugfs_dentry);
 
 	ret = kvm_arch_create_vcpu_debugfs(vcpu);
 	if (ret < 0) {

The interesting part of these debugfs entries is that they export an interface that is used by the kvm_stat tool. (and all distributions that I checked have debugfs enabled).

I think it is pretty unlikely that things will fail, but the question is: do we want to reject VM creation if that VM cannot be observed by instrumentation or not? No idea.

This also brings the question: shall we move these counters out of debugfs into something else?

Quoted 1 line

This also brings the question: shall we move these counters out of debugfs into something else?

If you have code that relies on debugfs, yes, you need to move that out of debugfs because more and more systems are trying to disable it due to the obvious problems with it (i.e. leaking tons of debugging information).

debugfs is for DEBUG information, not for "statistics about how my VM is working". That sounds like something you need to rely on, so debugfs is not the place for it.

Quoted 6 lines · Greg Kroah-Hartman

Quoted 1 line

This also brings the question: shall we move these counters out of debugfs into something else?

If you have code that relies on debugfs, yes, you need to move that out of debugfs because more and more systems are trying to disable it due to the obvious problems with it (i.e. leaking tons of debugging information).

Quoted 3 lines

debugfs is for DEBUG information, not for "statistics about how my VM is working". That sounds like something you need to rely on, so debugfs is not the place for it.

Yes, we know that and tracepoints are already one replacement. However, they are slower that just a lock-free "vcpu->stats.foo_happened++".

Another idea that Steven Rostedt and I discussed a while ago is some kind of "statfs" which would already provide some code, similar to the one that KVM uses to accumulate statistics from multiple VMs or multiple VCPUs into a single counter.