Re: [PATCH V4 34/38] x86/intel_rdt: Create debugfs files for pseudo-locking testing

[Greg KH]

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@xxxxxxxxx>
> 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@xxxxxxxxx>
> Date:   Tue Oct 31 00:15:51 2017 +0100
> 
>     IB/hfi1: convert to debugfs_file_get() and -put()
> 
> 
> commit 69d29f9e6a53559895e6f785f6cf72daa738f132
> Author: Nicolai Stange <nicstange@xxxxxxxxx>
> 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