lkml.org 
[lkml]   [2018]   [May]   [22]   [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,

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

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