From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S933099AbcJZOst (ORCPT <rfc822;w@1wt.eu>);
        Wed, 26 Oct 2016 10:48:49 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:40335 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932162AbcJZOsk (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 26 Oct 2016 10:48:40 -0400
Date: Wed, 26 Oct 2016 16:45:50 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Fenghua Yu <fenghua.yu@intel.com>
cc: "H. Peter Anvin" <h.peter.anvin@intel.com>,
        Ingo Molnar <mingo@elte.hu>, Tony Luck <tony.luck@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Stephane Eranian <eranian@google.com>, Borislav Petkov <bp@suse.de>,
        Dave Hansen <dave.hansen@intel.com>,
        Nilay Vaish <nilayvaish@gmail.com>, Shaohua Li <shli@fb.com>,
        David Carrillo-Cisneros <davidcc@google.com>,
        Ravi V Shankar <ravi.v.shankar@intel.com>,
        Sai Prakhya <sai.praneeth.prakhya@intel.com>,
        Vikas Shivappa <vikas.shivappa@linux.intel.com>,
        linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>
Subject: Re: [PATCH v5 12/18] x86/intel_rdt: Add "info" files to resctrl file
 system
In-Reply-To: <1477142405-32078-13-git-send-email-fenghua.yu@intel.com>
Message-ID: <alpine.DEB.2.20.1610261632110.4983@nanos>
References: <1477142405-32078-1-git-send-email-fenghua.yu@intel.com> <1477142405-32078-13-git-send-email-fenghua.yu@intel.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
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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