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@xxxxxxxxxx>
>> ---
>> 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
>
> .
>
