lkml.org 
[lkml]   [2018]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V4 34/38] x86/intel_rdt: Create debugfs files for pseudo-locking testing
On Wed, May 23, 2018 at 10:19:41AM -0700, Reinette Chatre wrote:
> 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.

Ah, that's where that pattern came from, thanks for finding it. It was
a conversion of the "old" api in the IB code that was using likely(),
which in a way, did make sense to use (due to the way processors assume
0 is "true")

I'll work on cleaning all of these up on my next long plane ride, should
give me something to do :)

thanks,

greg k-h

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