| Message ID | 1548384137-171488-1-git-send-email-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [-next] MIPS: fix debugfs_simple_attr.cocci warnings | expand |
On Fri, Jan 25, 2019 at 02:42:17AM +0000, YueHaibing 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(). What kind of overhead is this adding, and how are you measuring it? > > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > arch/mips/kernel/spinlock_test.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/mips/kernel/spinlock_test.c b/arch/mips/kernel/spinlock_test.c > index ab4e3e1..509de1e 100644 > --- a/arch/mips/kernel/spinlock_test.c > +++ b/arch/mips/kernel/spinlock_test.c > @@ -35,7 +35,7 @@ static int ss_get(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); > > > > @@ -114,14 +114,14 @@ static int multi_get(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); > > static int __init spinlock_test(void) > { > - debugfs_create_file("spin_single", S_IRUGO, mips_debugfs_dir, NULL, > - &fops_ss); > - debugfs_create_file("spin_multi", S_IRUGO, mips_debugfs_dir, NULL, > - &fops_multi); > + debugfs_create_file_unsafe("spin_single", 0444, mips_debugfs_dir, > + NULL, &fops_ss); > + debugfs_create_file_unsafe("spin_multi", 0444, mips_debugfs_dir, > + NULL, &fops_multi); > return 0; > } > device_initcall(spinlock_test); This is just testing code, right? Why is using the unsafe methods needed? thanks, greg k-h
On 2019/1/25 15:11, Greg Kroah-Hartman wrote: > On Fri, Jan 25, 2019 at 02:42:17AM +0000, YueHaibing 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(). > > What kind of overhead is this adding, and how are you measuring it? The log message on the commit introducing the semantic patch says the following: commit 5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") In order to protect against file removal races, debugfs files created via debugfs_create_file() now get wrapped by a struct file_operations at their opening. If the original struct file_operations are known to be safe against removal races by themselves already, the proxy creation may be bypassed by creating the files through debugfs_create_file_unsafe(). In order to help debugfs users who use the common DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file() idiom to transition to removal safe struct file_operations, the helper macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced. Thus, the preferred strategy is to use DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe() now. > >> >> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci >> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >> --- >> arch/mips/kernel/spinlock_test.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/mips/kernel/spinlock_test.c b/arch/mips/kernel/spinlock_test.c >> index ab4e3e1..509de1e 100644 >> --- a/arch/mips/kernel/spinlock_test.c >> +++ b/arch/mips/kernel/spinlock_test.c >> @@ -35,7 +35,7 @@ static int ss_get(void *data, u64 *val) >> return 0; >> } >> >> -DEFINE_SIMPLE_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); >> +DEFINE_DEBUGFS_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); >> >> >> >> @@ -114,14 +114,14 @@ static int multi_get(void *data, u64 *val) >> return 0; >> } >> >> -DEFINE_SIMPLE_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); >> +DEFINE_DEBUGFS_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); >> >> static int __init spinlock_test(void) >> { >> - debugfs_create_file("spin_single", S_IRUGO, mips_debugfs_dir, NULL, >> - &fops_ss); >> - debugfs_create_file("spin_multi", S_IRUGO, mips_debugfs_dir, NULL, >> - &fops_multi); >> + debugfs_create_file_unsafe("spin_single", 0444, mips_debugfs_dir, >> + NULL, &fops_ss); >> + debugfs_create_file_unsafe("spin_multi", 0444, mips_debugfs_dir, >> + NULL, &fops_multi); >> return 0; >> } >> device_initcall(spinlock_test); > > This is just testing code, right? Why is using the unsafe methods > needed? > > thanks, > > greg k-h > > . >
On Fri, Jan 25, 2019 at 03:43:21PM +0800, YueHaibing wrote: > On 2019/1/25 15:11, Greg Kroah-Hartman wrote: > > On Fri, Jan 25, 2019 at 02:42:17AM +0000, YueHaibing 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(). > > > > What kind of overhead is this adding, and how are you measuring it? > > The log message on the commit introducing the semantic patch says the > following: > > commit 5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") > > In order to protect against file removal races, debugfs files created via > debugfs_create_file() now get wrapped by a struct file_operations at their > opening. > > If the original struct file_operations are known to be safe against removal > races by themselves already, the proxy creation may be bypassed by creating > the files through debugfs_create_file_unsafe(). > > In order to help debugfs users who use the common > DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file() > idiom to transition to removal safe struct file_operations, the helper > macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced. > > Thus, the preferred strategy is to use > DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe() > now. That is true. So, are you saying that you "know" when you remove these files everything is safe? Are you seeing some sort of problem with these files as-is? If not, why change them to the "unsafe" method? thanks, greg k-h
diff --git a/arch/mips/kernel/spinlock_test.c b/arch/mips/kernel/spinlock_test.c index ab4e3e1..509de1e 100644 --- a/arch/mips/kernel/spinlock_test.c +++ b/arch/mips/kernel/spinlock_test.c @@ -35,7 +35,7 @@ static int ss_get(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_ss, ss_get, NULL, "%llu\n"); @@ -114,14 +114,14 @@ static int multi_get(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_multi, multi_get, NULL, "%llu\n"); static int __init spinlock_test(void) { - debugfs_create_file("spin_single", S_IRUGO, mips_debugfs_dir, NULL, - &fops_ss); - debugfs_create_file("spin_multi", S_IRUGO, mips_debugfs_dir, NULL, - &fops_multi); + debugfs_create_file_unsafe("spin_single", 0444, mips_debugfs_dir, + NULL, &fops_ss); + debugfs_create_file_unsafe("spin_multi", 0444, mips_debugfs_dir, + NULL, &fops_multi); return 0; } device_initcall(spinlock_test);
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(). Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- arch/mips/kernel/spinlock_test.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)