Date: Wed, 30 Nov 2016 23:25:49 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Shaohua Li <shli@fb.com>
cc: linux-kernel@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>
Subject: Re: [PATCH] intel-rdt: show mount options
In-Reply-To: <9892546694d8a9d5fad52c64fe5f16485198c38a.1480537792.git.shli@fb.com>
Message-ID: <alpine.DEB.2.20.1611302220590.3619@nanos>
References: <9892546694d8a9d5fad52c64fe5f16485198c38a.1480537792.git.shli@fb.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3590
Lines: 115

On Wed, 30 Nov 2016, Shaohua Li wrote:

> Subject : intel-rdt: show mount options

Oh well. Is it so hard to actually run

  git log arch/x86/kernel/cpu/intel_rdt_rdtgroup.c

to figure out what the proper prefix for this file is?

Aside of that the actual subject is pretty useless as it requires to read
the patch and the code to figure out what this is about.

   x86/intel_rdt: Implement show_options() for resctrlfs

That would have the proper prefix and actually clearly explain what the
patch is about. Aside of that the sentence starts with an upper case
letter, but you might have figured that out via git log as well.

> Show mount options when cdp is enabled

This is not what the patch does. It shows mount options unconditionally
whether CDP is enabled or not. In the latter case the options are empty.

So something like this:

  Implement a show_options() callback for the resource control filesystem
  to expose the active mount options in /proc/

would explain what this is about.

The fact that it only shows 'cdp' when the filesystem was actually mounted
with the 'cdp' option can be read from the patch itself and is completely
irrelevant for the changelog.

Changelogs should explain why and concepts not WHAT the patch does because
that's available from the patch itself.

> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index fb8e03e..49438b0 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -1034,9 +1034,19 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  	return ret;
>  }
>  
> +static int rdtgroup_show_options(struct seq_file *seq,
> +	struct kernfs_root *kf_root)

Sigh. You can avoid the silly line break by shortening the second argument:

static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kr)

which perfectly fine as it's not used anyway.

And if you really need a line break, then it should be either:

static int
rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kr)

or

static int rdtgroup_show_options(struct seq_file *seq,
				 struct kernfs_root *kr)

Can you see why both variants are better readable than this random
formatting you chose?

> +{
> +	struct rdt_resource *r_l3data = &rdt_resources_all[RDT_RESOURCE_L3DATA];

New line between variable declaration and code is missing.

> +	if (r_l3data->enabled)
> +		seq_puts(seq, ",cdp");

and this extra pointer is really pointless

	if (rdt_resources_all[RDT_RESOURCE_L3DATA].enabled)
		seq_puts(seq, ",cdp");

would be completely sufficient and actually more readable than the above.

> +	return 0;
> +}
> +
>  static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>  	.mkdir	= rdtgroup_mkdir,
>  	.rmdir	= rdtgroup_rmdir,
> +	.show_options = rdtgroup_show_options,

Sigh. The initializers of structures in this file are nicely aligned in a
tabular fashion, but just slapping stuff in is simpler, right? Is it so
hard to take a few extra seconds and do:

static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
-  	.mkdir	= rdtgroup_mkdir,
-  	.rmdir	= rdtgroup_rmdir,
-  	.mkdir		= rdtgroup_mkdir,
-  	.rmdir		= rdtgroup_rmdir,
+	.show_options	= rdtgroup_show_options,

Pretty sad, that I have to explain all the above to someone who contributes
to the kernel since 10+ years.

Thanks,

	tglx