Message ID | 1577151674-67949-1-git-send-email-chenwandun@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [next] KVM: Fix debugfs_simple_attr.cocci warnings | expand |
On 24/12/19 02:41, Chen Wandun wrote: > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > for debugfs files. > > Semantic patch information: > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > imposes some significant overhead as compared to > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > > Signed-off-by: Chen Wandun <chenwandun@huawei.com> This patch was sent probably already two or three times, and every time I've not been able to understand what is this significant overhead. With DEFINE_DEBUGFS_ATTRIBUTE: - the fops member is debugfs_open_proxy_file_operations, which calls replace_fops so that the fops->read member is debugfs_attr_read on the opened file - debugfs_attr_read does ret = debugfs_file_get(dentry); if (unlikely(ret)) return ret; ret = simple_attr_read(file, buf, len, ppos); debugfs_file_put(dentry); With DEFINE_SIMPLE_ATTRIBUTE: - the fops member is debugfs_full_proxy_open, and after __full_proxy_fops_init fops->read is initialized to full_proxy_read - full_proxy_read does r = debugfs_file_get(dentry); if (unlikely(r)) return r; real_fops = debugfs_real_fops(filp); r = real_fops->name(args); debugfs_file_put(dentry); return r; where real_fops->name is again just simple_attr_read. So the overhead is really just one kzalloc every time the file is opened. I could just apply the patch, but it wouldn't solve the main issue, which is that there is a function with a scary name ("debugfs_create_file_unsafe") that can be used in very common circumstances (with DEFINE_DEBUGFS_ATTRIBUTE. Therefore, we could instead fix the root cause and avoid using the scary API: - remove from the .cocci patch the change from debugfs_create_file to debugfs_create_file_unsafe. Only switch DEFINE_SIMPLE_ATTRIBUTE to DEFINE_DEBUGFS_ATTRIBUTE - change debugfs_create_file to automatically detect the "easy" case that does not need proxying of fops; something like this: const struct file_operations *proxy_fops; /* * Any struct file_operations defined by means of * DEFINE_DEBUGFS_ATTRIBUTE() is protected against file removals * and thus does not need proxying of read and write fops. */ if (!fops || (fops->llseek == no_llseek && ((!fops->read && !fops->read_iter) || fops->read == debugfs_attr_read) && ((!fops->write && !fops->write_iter) || fops->write == debugfs_attr_write) && !fops->poll && !fops->unlocked_ioctl) return debugfs_create_file_unsafe(name, mode, parent, data, fops); /* These are not supported by __full_proxy_fops_init. */ WARN_ON_ONCE(fops->read_iter || fops->write_iter); return __debugfs_create_file(name, mode, parent, data, &debugfs_full_proxy_file_operations, fops); CCing Nicolai Stange who first introduced debugfs_create_file_unsafe. Thanks, Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 24/12/19 02:41, Chen Wandun wrote: >> Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE >> for debugfs files. >> >> Semantic patch information: >> Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() >> imposes some significant overhead as compared to >> DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). >> >> Signed-off-by: Chen Wandun <chenwandun@huawei.com> > > This patch was sent probably already two or three times, and every time > I've not been able to understand what is this significant overhead. As you correctly stated below, the overhead is one kmalloc(sizeof(struct file_operations)) per opened debugfs file (i.e. one per debugfs struct file instance). struct file_operations is equivalent to 33 unsigned longs, so it might not be seen as that "significant", but it isn't small either. > With DEFINE_DEBUGFS_ATTRIBUTE: > > - the fops member is debugfs_open_proxy_file_operations, which calls > replace_fops so that the fops->read member is debugfs_attr_read on the > opened file > > - debugfs_attr_read does > > ret = debugfs_file_get(dentry); > if (unlikely(ret)) > return ret; > ret = simple_attr_read(file, buf, len, ppos); > debugfs_file_put(dentry); > > With DEFINE_SIMPLE_ATTRIBUTE: > > - the fops member is debugfs_full_proxy_open, and after > __full_proxy_fops_init fops->read is initialized to full_proxy_read > > - full_proxy_read does > > r = debugfs_file_get(dentry); > if (unlikely(r)) > return r; > real_fops = debugfs_real_fops(filp); > r = real_fops->name(args); > debugfs_file_put(dentry); > return r; > > where real_fops->name is again just simple_attr_read. > > So the overhead is really just one kzalloc every time the file is > opened. Yes. > I could just apply the patch, but it wouldn't solve the main issue, > which is that there is a function with a scary name > ("debugfs_create_file_unsafe") that can be used in very common > circumstances (with DEFINE_DEBUGFS_ATTRIBUTE. Agreed, the naming is a bit poor. "debugfs_create_file_no_proxy" or the like would perhaps have been a better choice. > Therefore, we could > instead fix the root cause and avoid using the scary API: > > - remove from the .cocci patch the change from debugfs_create_file to > debugfs_create_file_unsafe. Only switch DEFINE_SIMPLE_ATTRIBUTE to > DEFINE_DEBUGFS_ATTRIBUTE > > - change debugfs_create_file to automatically detect the "easy" case > that does not need proxying of fops; something like this: > > const struct file_operations *proxy_fops; > > /* > * Any struct file_operations defined by means of > * DEFINE_DEBUGFS_ATTRIBUTE() is protected against file removals > * and thus does not need proxying of read and write fops. > */ > if (!fops || > (fops->llseek == no_llseek && > ((!fops->read && !fops->read_iter) || > fops->read == debugfs_attr_read) && > ((!fops->write && !fops->write_iter) || > fops->write == debugfs_attr_write) && > !fops->poll && !fops->unlocked_ioctl) > return debugfs_create_file_unsafe(name, mode, parent, > data, fops); > > /* These are not supported by __full_proxy_fops_init. */ > WARN_ON_ONCE(fops->read_iter || fops->write_iter); > return __debugfs_create_file(name, mode, parent, data, > &debugfs_full_proxy_file_operations, > fops); > > CCing Nicolai Stange who first introduced debugfs_create_file_unsafe. I'm not strictly against your proposal, but I somewhat dislike the idea of adding runtime checks for special cases to work around a historic issue. Also, we'd either have to touch the ~63 existing call sites of debugfs_create_file_unsafe() again or had to live with inconsistent debugfs usage patterns. AFAICT, your approach wouldn't really put a relieve on maintainers wrt. patch count as the cocci check for the DEFINE_SIMPLE_ATTRIBUTE -> DEFINE_DEBUGFS_ATTRIBUTE conversion would still be needed. And then there's grepability: right now it would be possible to find all fully proxied debugfs files by means of "git grep 'debugfs_file_create('". I'm not saying I'm about to convert these, but in theory it could be done easily. How about introducing a #define debugfs_create_attr debugfs_create_file_unsafe instead to make those s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/ patches look less scary? Ideally, for the sake of additional safety, DEFINE_DEBUGFS_ATTRIBUTE could be made to wrap the file_operations within something like a struct debugfs_attr_file_operations and debugfs_create_attr() would take that instead of a plain file_operations. But again, this would require touching the existing users of debugfs_create_file_unsafe()... So I'm not sure it would be worth it. c Thanks, Nicolai
On 08/01/20 11:29, Nicolai Stange wrote: > > How about introducing a > #define debugfs_create_attr debugfs_create_file_unsafe > instead to make those > s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/ > patches look less scary? I agree that could be worthwhile. My main complaint is the "scariness factor" of the patches that convert DEFINE_SIMPLE_ATTRIBUTE to DEFINE_DEBUGFS_ATTRIBUTE, and this change would alleviate that problem. Thanks, Paolo
diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c index 018aebc..0867d7d 100644 --- a/arch/x86/kvm/debugfs.c +++ b/arch/x86/kvm/debugfs.c @@ -15,7 +15,8 @@ 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) { @@ -24,7 +25,8 @@ 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) { @@ -33,7 +35,8 @@ 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) { @@ -41,24 +44,25 @@ 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"); void kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu) { - debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu, - &vcpu_tsc_offset_fops); + debugfs_create_file_unsafe("tsc-offset", 0444, vcpu->debugfs_dentry, + vcpu, &vcpu_tsc_offset_fops); if (lapic_in_kernel(vcpu)) - debugfs_create_file("lapic_timer_advance_ns", 0444, - vcpu->debugfs_dentry, vcpu, - &vcpu_timer_advance_ns_fops); + debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444, + vcpu->debugfs_dentry, vcpu, + &vcpu_timer_advance_ns_fops); if (kvm_has_tsc_control) { - debugfs_create_file("tsc-scaling-ratio", 0444, - vcpu->debugfs_dentry, vcpu, - &vcpu_tsc_scaling_fops); - debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444, - vcpu->debugfs_dentry, vcpu, - &vcpu_tsc_scaling_frac_fops); + debugfs_create_file_unsafe("tsc-scaling-ratio", 0444, + vcpu->debugfs_dentry, vcpu, + &vcpu_tsc_scaling_fops); + debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444, + vcpu->debugfs_dentry, vcpu, + &vcpu_tsc_scaling_frac_fops); } }
Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for debugfs files. Semantic patch information: Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() imposes some significant overhead as compared to DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). Signed-off-by: Chen Wandun <chenwandun@huawei.com> --- arch/x86/kvm/debugfs.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)