| Message ID | 20190122152151.16139-51-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: no need to check return value of debugfs_create functions | expand |
On Tue, Jan 22, 2019 at 04:21:50PM +0100, Greg Kroah-Hartman wrote: > 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. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > Cc: kvm@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > virt/kvm/kvm_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 5ecea812cb6a..4f96450ecdfc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -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) { > -- > 2.20.1 >
On Tue, Jan 22, 2019 at 09:21:02AM -0800, Sean Christopherson wrote: > On Tue, Jan 22, 2019 at 04:21:50PM +0100, Greg Kroah-Hartman wrote: > > 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. 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.
On Tue, Jan 22, 2019 at 09:29:07AM -0800, Sean Christopherson wrote: > On Tue, Jan 22, 2019 at 09:21:02AM -0800, Sean Christopherson wrote: > > On Tue, Jan 22, 2019 at 04:21:50PM +0100, Greg Kroah-Hartman wrote: > > > 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. > > 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. Oh, nice, want me to tack this onto the end of there, or just do some follow-on patches after this gets merged? thanks, greg k-h
On 22.01.2019 16:21, Greg Kroah-Hartman wrote: > 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 <pbonzini@redhat.com> > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > Cc: kvm@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > virt/kvm/kvm_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 5ecea812cb6a..4f96450ecdfc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -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? Christian
On Tue, Jan 22, 2019 at 09:39:24PM +0100, Christian Borntraeger wrote: > > > On 22.01.2019 16:21, Greg Kroah-Hartman wrote: > > 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 <pbonzini@redhat.com> > > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > > Cc: kvm@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > virt/kvm/kvm_main.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 5ecea812cb6a..4f96450ecdfc 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -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. No you should not as any other part of the kernel can randomly create the same debugfs files and keep your code from being able to create them :) > 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. thanks, greg k-h
On 22/01/19 21:48, Greg Kroah-Hartman wrote: >> 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. 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. Paolo
On 23.01.2019 00:11, Paolo Bonzini wrote: > On 22/01/19 21:48, Greg Kroah-Hartman wrote: >>> 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. > > Yes, we know that and tracepoints are already one replacement. However, > they are slower that just a lock-free "vcpu->stats.foo_happened++". Yes, the tracepoints are not a proper replacement for the counters (especially the capability to get numbers after-the-fact. So I would really like to keep both. > 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. I think that would make a lot of sense to have a common filesystem to avoid code duplication bugs.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 5ecea812cb6a..4f96450ecdfc 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -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) {
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 <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: kvm@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- virt/kvm/kvm_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)