Message ID | 20191218020842.186446-1-chenzhou10@huawei.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | powerpc/setup_64: use DEFINE_DEBUGFS_ATTRIBUTE to define fops_rfi_flush | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (270c0c3e491684893e7250f6c32f4f2eb2e4c3b2) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 13 lines checked |
Chen Zhou <chenzhou10@huawei.com> writes: > 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(). I know you didn't write this text, but these change logs are not great. It doesn't really explain why you're doing it. And it is alarming that you're converting *to* a function with "unsafe" in the name. The commit that added the script: 5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") Has a bit more explanation. Maybe something like this: In order to protect against file removal races, debugfs files created via debugfs_create_file() are wrapped by a struct file_operations at their opening. If the original struct file_operations is known to be safe against removal races already, the proxy creation may be bypassed by creating the files using DEFINE_DEBUGFS_ATTRIBUTE() and debugfs_create_file_unsafe(). The part that's not explained is why this file is "known to be safe against removal races already"? It also seems this conversion will make the file no longer seekable, because DEFINE_SIMPLE_ATTRIBUTE() uses generic_file_llseek() whereas DEFINE_DEBUGFS_ATTRIBUTE() uses no_llseek. That is probably fine, but should be mentioned. cheers > Signed-off-by: Chen Zhou <chenzhou10@huawei.com> > --- > arch/powerpc/kernel/setup_64.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c > index 6104917..4b9fbb2 100644 > --- a/arch/powerpc/kernel/setup_64.c > +++ b/arch/powerpc/kernel/setup_64.c > @@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); > > static __init int rfi_flush_debugfs_init(void) > { > - debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); > + debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); > return 0; > } > device_initcall(rfi_flush_debugfs_init); > -- > 2.7.4
Hi Michael, On 2019/12/18 19:02, Michael Ellerman wrote: > Chen Zhou <chenzhou10@huawei.com> writes: >> 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(). > > I know you didn't write this text, but these change logs are not great. > It doesn't really explain why you're doing it. And it is alarming that > you're converting *to* a function with "unsafe" in the name. > > The commit that added the script: > > 5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") > > Has a bit more explanation. > > Maybe something like this: > > In order to protect against file removal races, debugfs files created via > debugfs_create_file() are wrapped by a struct file_operations at their > opening. > > If the original struct file_operations is known to be safe against removal > races already, the proxy creation may be bypassed by creating the files > using DEFINE_DEBUGFS_ATTRIBUTE() and debugfs_create_file_unsafe(). > > > The part that's not explained is why this file is "known to be safe > against removal races already"? > > It also seems this conversion will make the file no longer seekable, > because DEFINE_SIMPLE_ATTRIBUTE() uses generic_file_llseek() whereas > DEFINE_DEBUGFS_ATTRIBUTE() uses no_llseek. > > That is probably fine, but should be mentioned. Thanks for your explanation. This part indeed should be mentioned. Chen Zhou > > cheers > >> Signed-off-by: Chen Zhou <chenzhou10@huawei.com> >> --- >> arch/powerpc/kernel/setup_64.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c >> index 6104917..4b9fbb2 100644 >> --- a/arch/powerpc/kernel/setup_64.c >> +++ b/arch/powerpc/kernel/setup_64.c >> @@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val) >> return 0; >> } >> >> -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); >> +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); >> >> static __init int rfi_flush_debugfs_init(void) >> { >> - debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); >> + debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); >> return 0; >> } >> device_initcall(rfi_flush_debugfs_init); >> -- >> 2.7.4 > > . >
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c index 6104917..4b9fbb2 100644 --- a/arch/powerpc/kernel/setup_64.c +++ b/arch/powerpc/kernel/setup_64.c @@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n"); static __init int rfi_flush_debugfs_init(void) { - debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); + debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush); return 0; } device_initcall(rfi_flush_debugfs_init);
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 Zhou <chenzhou10@huawei.com> --- arch/powerpc/kernel/setup_64.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)