Messages in this thread | | | Subject | Re: [PATCH V4 34/38] x86/intel_rdt: Create debugfs files for pseudo-locking testing | From | Reinette Chatre <> | Date | Wed, 23 May 2018 10:19:41 -0700 |
| |
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
| |