Messages in this thread | | | Subject | Re: [PATCH V4 34/38] x86/intel_rdt: Create debugfs files for pseudo-locking testing | From | Reinette Chatre <> | Date | Tue, 22 May 2018 14:02:37 -0700 |
| |
Hi Greg,
Thank you very much for taking a look.
On 5/22/2018 12:43 PM, Greg KH wrote: > On Tue, May 22, 2018 at 04:29:22AM -0700, Reinette Chatre wrote: >> @@ -149,6 +151,9 @@ struct pseudo_lock_region { >> unsigned int line_size; >> unsigned int size; >> void *kmem; >> +#ifdef CONFIG_INTEL_RDT_DEBUGFS >> + struct dentry *debugfs_dir; >> +#endif > > Who cares, just always have this here, it's not going to save you > anything to #ifdef the .c code everywhere just for this one pointer.
ok
> >> @@ -174,6 +180,9 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr) >> plr->d->plr = NULL; >> plr->d = NULL; >> plr->cbm = 0; >> +#ifdef CONFIG_INTEL_RDT_DEBUGFS >> + plr->debugfs_dir = NULL; >> +#endif > > See? Ick. > >> + 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.
>> +#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.
>> + } >> + >> + 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?
Reinette
| |