| Message ID | 1563780839-14739-1-git-send-email-wang.yi59@zte.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: x86: Use DEFINE_DEBUGFS_ATTRIBUTE for debugfs files | expand |
On 22/07/19 09:33, Yi Wang wrote: > We got these coccinelle warning: > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should > be defined with DEFINE_DEBUGFS_ATTRIBUTE > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should > be defined with DEFINE_DEBUGFS_ATTRIBUTE > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE() > to fix this. > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> It sucks though that you have to use a function with "unsafe" in the name. Greg, is the patch doing the right thing? Paolo > --- > arch/x86/kvm/debugfs.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c > index 329361b..24016fb 100644 > --- a/arch/x86/kvm/debugfs.c > +++ b/arch/x86/kvm/debugfs.c > @@ -20,7 +20,7 @@ static int vcpu_get_timer_advance_ns(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n"); > > static int vcpu_get_tsc_offset(void *data, u64 *val) > { > @@ -29,7 +29,7 @@ static int vcpu_get_tsc_offset(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n"); > > static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val) > { > @@ -38,7 +38,7 @@ static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n"); > > static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val) > { > @@ -46,20 +46,20 @@ static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n"); > > int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu) > { > struct dentry *ret; > > - ret = debugfs_create_file("tsc-offset", 0444, > + ret = debugfs_create_file_unsafe("tsc-offset", 0444, > vcpu->debugfs_dentry, > vcpu, &vcpu_tsc_offset_fops); > if (!ret) > return -ENOMEM; > > if (lapic_in_kernel(vcpu)) { > - ret = debugfs_create_file("lapic_timer_advance_ns", 0444, > + ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444, > vcpu->debugfs_dentry, > vcpu, &vcpu_timer_advance_ns_fops); > if (!ret) > @@ -67,12 +67,12 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu) > } > > if (kvm_has_tsc_control) { > - ret = debugfs_create_file("tsc-scaling-ratio", 0444, > + ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444, > vcpu->debugfs_dentry, > vcpu, &vcpu_tsc_scaling_fops); > if (!ret) > return -ENOMEM; > - ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444, > + ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444, > vcpu->debugfs_dentry, > vcpu, &vcpu_tsc_scaling_frac_fops); > if (!ret) >
On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote: > On 22/07/19 09:33, Yi Wang wrote: > > We got these coccinelle warning: > > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should > > be defined with DEFINE_DEBUGFS_ATTRIBUTE > > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should > > be defined with DEFINE_DEBUGFS_ATTRIBUTE > > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE() > > to fix this. > > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> > > It sucks though that you have to use a function with "unsafe" in the name. I agree, why make this change? > Greg, is the patch doing the right thing? I can't tell. What coccinelle script generated this patch? thanks, greg k-h
On 17/09/19 20:12, Greg Kroah-Hartman wrote: > On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote: >> On 22/07/19 09:33, Yi Wang wrote: >>> We got these coccinelle warning: >>> ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops >>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE >>> ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should >>> be defined with DEFINE_DEBUGFS_ATTRIBUTE >>> ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should >>> be defined with DEFINE_DEBUGFS_ATTRIBUTE >>> ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops >>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE >>> >>> Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE() >>> to fix this. >>> >>> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> >> >> It sucks though that you have to use a function with "unsafe" in the name. > > I agree, why make this change? > >> Greg, is the patch doing the right thing? > > I can't tell. What coccinelle script generated this patch? Seems to be scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci. //# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() //# imposes some significant overhead as compared to //# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). Paolo > thanks, > > greg k-h >
On Wed, Sep 18, 2019 at 08:19:44AM +0800, wang.yi59@zte.com.cn wrote: > Hi Paolo, > > > On 22/07/19 09:33, Yi Wang wrote: > > > We got these coccinelle warning: > > > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should > > > be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should > > > be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE() > > > to fix this. > > > > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> > > > > It sucks though that you have to use a function with "unsafe" in the name. > > Yes, it does, but I found some patches in the git log: > https://git.kernel.org/pub/scm/virt/kvm/kvm.git/log/?qt=grep&q=DEFINE_DEBUGFS_ATTRIBUTE+ > > And, do you think the function name "debugfs_create_file_unsafe" is not proper? Only if you _KNOW_ you are creating/removing these files in a way that is safe is it ok to use these calls. Hint, what is your locking strategy for when these files are removed? Is that the case here? If not, please stick with what is there today, as we know it works, and it is "safe" to do so. thanks, greg k-h
diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c index 329361b..24016fb 100644 --- a/arch/x86/kvm/debugfs.c +++ b/arch/x86/kvm/debugfs.c @@ -20,7 +20,7 @@ static int vcpu_get_timer_advance_ns(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n"); static int vcpu_get_tsc_offset(void *data, u64 *val) { @@ -29,7 +29,7 @@ static int vcpu_get_tsc_offset(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n"); +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n"); static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val) { @@ -38,7 +38,7 @@ static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n"); static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val) { @@ -46,20 +46,20 @@ static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n"); int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu) { struct dentry *ret; - ret = debugfs_create_file("tsc-offset", 0444, + ret = debugfs_create_file_unsafe("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu, &vcpu_tsc_offset_fops); if (!ret) return -ENOMEM; if (lapic_in_kernel(vcpu)) { - ret = debugfs_create_file("lapic_timer_advance_ns", 0444, + ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444, vcpu->debugfs_dentry, vcpu, &vcpu_timer_advance_ns_fops); if (!ret) @@ -67,12 +67,12 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu) } if (kvm_has_tsc_control) { - ret = debugfs_create_file("tsc-scaling-ratio", 0444, + ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444, vcpu->debugfs_dentry, vcpu, &vcpu_tsc_scaling_fops); if (!ret) return -ENOMEM; - ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444, + ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444, vcpu->debugfs_dentry, vcpu, &vcpu_tsc_scaling_frac_fops); if (!ret)
We got these coccinelle warning: ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE() to fix this. Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> --- arch/x86/kvm/debugfs.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)