lkml.org 
[lkml]   [2018]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V4 34/38] x86/intel_rdt: Create debugfs files for pseudo-locking testing
From
Date
Hi Greg,

On 5/23/2018 1:05 AM, Greg KH wrote:
> On Tue, May 22, 2018 at 02:02:37PM -0700, Reinette Chatre wrote:
>> On 5/22/2018 12:43 PM, Greg KH wrote:
>>> On Tue, May 22, 2018 at 04:29:22AM -0700, Reinette Chatre wrote:
>>>> + ret = strtobool(buf, &bv);
>>>> + if (ret == 0 && bv) {
>>>> + ret = debugfs_file_get(file->f_path.dentry);
>>>> + if (unlikely(ret))
>>>> + return ret;
>>>
>>> Only ever use unlikely/likely if you can measure the performance
>>> difference. Hint, you can't do that here, it's not needed at all.
>>
>> Here my intention was to follow the current best practices and in the
>> kernel source I am working with eight of the ten usages of
>> debugfs_file_get() is followed by an unlikely(). My assumption was thus
>> that this is a best practice. Thanks for catching this - I'll change it.
>
> Really? That's some horrible examples, any pointers to them? I think I
> need to do a massive sweep of the kernel tree and fix up all of this
> crud so that people don't keep cut/paste the same bad code everywhere.

As you know debugfs_file_get() is a recent addition to the kernel,
introduced in:
commit e9117a5a4bf65d8e99f060d356a04d27a60b436d
Author: Nicolai Stange <nicstange@gmail.com>
Date: Tue Oct 31 00:15:48 2017 +0100

debugfs: implement per-file removal protection

Following this introduction, the same author modified the now obsolete
calls of debugfs_use_file_start() to debugfs_file_get() in commits:

commit 7cda7b8f97da9382bb945d541a85cde58d5dac27
Author: Nicolai Stange <nicstange@gmail.com>
Date: Tue Oct 31 00:15:51 2017 +0100

IB/hfi1: convert to debugfs_file_get() and -put()


commit 69d29f9e6a53559895e6f785f6cf72daa738f132
Author: Nicolai Stange <nicstange@gmail.com>
Date: Tue Oct 31 00:15:50 2017 +0100

debugfs: convert to debugfs_file_get() and -put()


In the above two commits the usage of the new debugfs_file_get()
primarily follows the pattern of:
r = debugfs_file_get(d);
if (unlikely(r))

Since the author of the new interface used the pattern above in the
conversions I do not think it is unreasonable to find other developers
following suit believing that it is a best practice.

This pattern remains in the majority when looking at the output of (on
v4.17-rc5):
git grep -A 1 ' = debugfs_file_get'

>>>> +#ifdef CONFIG_INTEL_RDT_DEBUGFS
>>>> + plr->debugfs_dir = debugfs_create_dir(rdtgrp->kn->name,
>>>> + debugfs_resctrl);
>>>> + if (IS_ERR(plr->debugfs_dir)) {
>>>> + ret = PTR_ERR(plr->debugfs_dir);
>>>> + plr->debugfs_dir = NULL;
>>>> + goto out_region;
>>>
>>> Ick no, you never need to care about the return value of a debugfs call.
>>> You code should never do something different if a debugfs call succeeds
>>> or fails. And you are checking it wrong, even if you did want to do
>>> this :)
>>
>> Ah - I see I need to be using IS_ERR_OR_NULL() instead of IS_ERR()? If
>> this is the case then please note that there seems to be quite a few
>> debugfs_create_dir() calls within the kernel that have the same issue.
>
> Again, they are all wrong :)
>
> Just ignore the return value, unless it is a directory, and then just
> save it like you are here. Don't check the value, you can always pass
> it into a future debugfs call with no problems.

Will do. Thank you very much for the advise.

>>>> + }
>>>> +
>>>> + entry = debugfs_create_file("pseudo_lock_measure", 0200,
>>>> + plr->debugfs_dir, rdtgrp,
>>>> + &pseudo_measure_fops);
>>>> + if (IS_ERR(entry)) {
>>>> + ret = PTR_ERR(entry);
>>>> + goto out_debugfs;
>>>> + }
>>>
>>> Again, you don't care, don't do this.
>>>
>>>> +#ifdef CONFIG_INTEL_RDT_DEBUGFS
>>>> + debugfs_remove_recursive(rdtgrp->plr->debugfs_dir);
>>>> +#endif
>>>
>>> Don't put ifdefs in .c files, it's not the Linux way at all. You can
>>> make this a lot simpler/easier to maintain over time if you do not.
>>
>> My mistake - I assumed this would be ok based on my interpretation of
>> how CONFIG_GENERIC_IRQ_DEBUGFS is used.
>>
>> I could rework the debugfs code to be contained in a new debugfs
>> specific .c file that is only compiled if the configuration is set. The
>> ifdefs will then be restricted to a .h file that contains the
>> declarations of these debugfs functions with empty variants when the
>> user did not select the debugfs config option.
>>
>> Would that be acceptable to you?
>
> Yes, that is the correct way to do this.
>
> But why would someone _not_ want this option? Why not always just
> include the functionality, that way you don't have to ask someone to
> rebuild a kernel if you need that debug information. And distros will
> always enable the option anyway, so it's not like you are keeping things
> "smaller", if you disable debugfs, all of that code should just compile
> away to almost nothing anyway.

Will do.

Thank you very much for taking the time to review and provide
constructive feedback.

Reinette

\
 
 \ /
  Last update: 2018-05-23 19:20    [W:0.059 / U:2.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site