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

[Thomas Gleixner]

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