Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1509494

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

From Thomas Gleixner <tglx@linutronix.de>
Newsgroups linux.kernel
Subject Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file system
Date 2016-10-26 16:50 +0200
Message-ID <swxGF-51u-9@gated-at.bofh.it> (permalink)
References <sv4x3-3Ya-11@gated-at.bofh.it> <sv4x3-3Ya-13@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw


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

Back to linux.kernel | Previous | NextPrevious in thread | Next in thread | Find similar | Unroll thread


Thread

[PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file system "Fenghua Yu" <fenghua.yu@intel.com> - 2016-10-22 15:30 +0200
  Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file  system Thomas Gleixner <tglx@linutronix.de> - 2016-10-26 16:50 +0200
    Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl  file system "Luck, Tony" <tony.luck@intel.com> - 2016-10-26 17:50 +0200
      Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file  system Thomas Gleixner <tglx@linutronix.de> - 2016-10-26 19:40 +0200

csiph-web