linux-kernel - Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file system

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2016 16:45:50 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Stephane Eranian <eranian@...gle.com>,
        Borislav Petkov <bp@...e.de>,
        Dave Hansen <dave.hansen@...el.com>,
        Nilay Vaish <nilayvaish@...il.com>, Shaohua Li <shli@...com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file
 system

On Sat, 22 Oct 2016, Fenghua Yu wrote:
> +static int rdtgroup_add_files(struct kernfs_node *kn, struct rftype *rfts,
> +			      int len)
> +{
> +	struct rftype *rft;
> +	int ret;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	for (rft = rfts; rft < rfts + len; rft++) {
> +		ret = rdtgroup_add_file(kn, rft);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	return 0;
> +error:
> +	pr_warn("%s: failed to add %s, err=%d\n", __func__, rft->name, ret);

Please stop this silly __func__ nonsense. You already have a prefix and if
you write out a proper sentence describing the problem then it's way more
helpful than __func__

	pr_warn("Failed to add file %s, err %d\n" ....

Tells exactly what happened and for a sysadmin this is information enough
as he does not care at all in which function that happens. For the
developer who is poked by that admin it's easy enough to find the function
via the string.

> +/* rdtgroup information files for one cache resource. */
> +static struct rftype res_info_files[] = {
> +	{
> +		.name		= "num_closid",

num_closids please

> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdt_num_closid_show,
> +	},
> +	{
> +		.name		= "cbm_val",

cbm_val? Is that a value? No, it's the valid bitmask which you can set. So
cmb_mask or something else which is unambiguous is what you want here.

This is a user space interface and we really must make it as intuitive as
possible.

> +/*
> + * Forcibly remove all of subdirectories under root.
> + */
> +static void rmdir_all_sub(void)
> +{
> +	kernfs_remove(kn_info);

What clears kn_info?

Thanks,

	tglx