Date: Fri, 15 May 2015 21:18:34 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Vikas Shivappa <vikas.shivappa@linux.intel.com>
cc: vikas.shivappa@intel.com, x86@kernel.org, linux-kernel@vger.kernel.org,
        hpa@zytor.com, mingo@kernel.org, tj@kernel.org, peterz@infradead.org,
        matt.fleming@intel.com, will.auld@intel.com, peter.zijlstra@intel.com,
        h.peter.anvin@intel.com, kanaka.d.juvva@intel.com, mtosatti@redhat.com
Subject: Re: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service
 management
In-Reply-To: <1431370976-31115-3-git-send-email-vikas.shivappa@linux.intel.com>
Message-ID: <alpine.DEB.2.11.1505152030400.4225@nanos>
References: <1431370976-31115-1-git-send-email-vikas.shivappa@linux.intel.com> <1431370976-31115-3-git-send-email-vikas.shivappa@linux.intel.com>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4743
Lines: 192

On Mon, 11 May 2015, Vikas Shivappa wrote:
>  arch/x86/include/asm/intel_rdt.h |  38 +++++++++++++
>  arch/x86/kernel/cpu/intel_rdt.c  | 112 +++++++++++++++++++++++++++++++++++++--
>  include/linux/cgroup_subsys.h    |   4 ++

Where is the Documentation for the new cgroup subsystem?

> +struct rdt_subsys_info {
> +	/* Clos Bitmap to keep track of available CLOSids.*/

If you want to document your struct members, please use KernelDoc
formatting, so tools can pick it up as well.

> +	unsigned long *closmap;
> +};
> +
> +struct intel_rdt {
> +	struct cgroup_subsys_state css;

Ditto

> +	/* Class of service for the cgroup.*/
> +	unsigned int clos;
> +};
> +
> +struct clos_cbm_map {
> +	unsigned long cache_mask;
> +	unsigned int clos_refcnt;
> +};

> +/*
> + * ccmap maintains 1:1 mapping between CLOSid and cache bitmask.
> + */
> +static struct clos_cbm_map *ccmap;
> +static struct rdt_subsys_info rdtss_info;
> +static DEFINE_MUTEX(rdt_group_mutex);
> +struct intel_rdt rdt_root_group;
> +
> +static void intel_rdt_free_closid(unsigned int clos)
> +{
> +	lockdep_assert_held(&rdt_group_mutex);
> +
> +	clear_bit(clos, rdtss_info.closmap);
> +}
> +
> +static void __clos_get(unsigned int closid)

What's the purpose of these double underscores?

> +{
> +	struct clos_cbm_map *ccm = &ccmap[closid];
> +
> +	lockdep_assert_held(&rdt_group_mutex);

Do we really need all these lockdep asserts for a handfull of call
sites?

> +	ccm->clos_refcnt += 1;

What's wrong with:

       ccmap[closid].clos_refcnt++;

Hmm?

> +}
> +
> +static void __clos_put(unsigned int closid)
> +{
> +	struct clos_cbm_map *ccm = &ccmap[closid];
> +
> +	lockdep_assert_held(&rdt_group_mutex);
> +	WARN_ON(!ccm->clos_refcnt);

So we have a warning but we do not act on it.

> +
> +	ccm->clos_refcnt -= 1;
> +	if (!ccm->clos_refcnt)

You can do that in a single line w/o the pointer magic. We want easy
to read and small code rather than pointlessly blown up helper
functions which just eat screen space.

> +		intel_rdt_free_closid(closid);
> +}
> +
> +static struct cgroup_subsys_state *
> +intel_rdt_css_alloc(struct cgroup_subsys_state *parent_css)
> +{
> +	struct intel_rdt *parent = css_rdt(parent_css);
> +	struct intel_rdt *ir;
> +
> +	/*
> +	 * Cannot return failure on systems with no Cache Allocation
> +	 * as the cgroup_init does not handle failures gracefully.

This comment is blatantly wrong. 5 lines further down you return
-ENOMEM. Now what?

> +	 */
> +	if (!parent)
> +		return &rdt_root_group.css;
> +
> +	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
> +	if (!ir)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_lock(&rdt_group_mutex);
> +	ir->clos = parent->clos;
> +	__clos_get(ir->clos);
> +	mutex_unlock(&rdt_group_mutex);
> +
> +	return &ir->css;
> +}
> +
> +static void intel_rdt_css_free(struct cgroup_subsys_state *css)
> +{
> +	struct intel_rdt *ir = css_rdt(css);
> +
> +	mutex_lock(&rdt_group_mutex);
> +	__clos_put(ir->clos);
> +	kfree(ir);
> +	mutex_unlock(&rdt_group_mutex);
> +}
>  
>  static int __init intel_rdt_late_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
> -	int maxid, max_cbm_len;
> +	static struct clos_cbm_map *ccm;
> +	int maxid, max_cbm_len, err = 0;
> +	size_t sizeb;
>  
> -	if (!cpu_has(c, X86_FEATURE_CAT_L3))
> +	if (!cpu_has(c, X86_FEATURE_CAT_L3)) {
> +		rdt_root_group.css.ss->disabled = 1;
>  		return -ENODEV;
> -
> +	}
>  	maxid = c->x86_rdt_max_closid;
>  	max_cbm_len = c->x86_rdt_max_cbm_len;
>  
> +	sizeb = BITS_TO_LONGS(maxid) * sizeof(long);
> +	rdtss_info.closmap = kzalloc(sizeb, GFP_KERNEL);

What's the point of this bitmap? In later patches you have a
restriction to 64bits and the SDM says that CBM_LEN is limited to 32.

So where is the point for allocating a bitmap?

> +	if (!rdtss_info.closmap) {
> +		err = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	sizeb = maxid * sizeof(struct clos_cbm_map);
> +	ccmap = kzalloc(sizeb, GFP_KERNEL);
> +	if (!ccmap) {
> +		kfree(rdtss_info.closmap);
> +		err = -ENOMEM;
> +		goto out_err;

What's wrong with return -ENOMEM? We only use goto if we have to
cleanup stuff, certainly not for just returning err.

> +	}
> +
> +	set_bit(0, rdtss_info.closmap);
> +	rdt_root_group.clos = 0;
> +	ccm = &ccmap[0];
> +	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
> +	ccm->clos_refcnt = 1;
> +
>  	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len, maxid);

We surely do not want to sprinkle these all over dmesg.

+out_err:
 
-       return 0;
+       return err;


Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/