Re: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service management

[Vikas Shivappa <vikas.shivappa@intel.com>]

On Fri, 15 May 2015, Thomas Gleixner wrote:

> 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?

Documentation/cgroups/rdt.txt

>
>> +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.
>

Will fix

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

Will fix

>
>> +	/* 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?
>

Similar to __get_rmid.

>> +{
>> +	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?

Well, we still have some calls some may be frequent depending on the usage.. 
should the assert decided based on number of times its called ?

>
>> +	ccm->clos_refcnt += 1;
>
> What's wrong with:
>
>       ccmap[closid].clos_refcnt++;
>
> Hmm?

Yes , this can be fixed.

>
>> +}
>> +
>> +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.

Ok , will change to printing the WARN_ON debug message and just returning.

>
>> +
>> +	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.

Will change this.

>
>> +		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?
>

Well , this is for cgroup_init called with parent=null which is when its 
initializing the cgroup subsystem. I cant return error in this case but I can 
otherwise.

static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
{

 	/* Create the root cgroup state for this subsystem */
 	ss->root = &cgrp_dfl_root;
 	css = ss->css_alloc(cgroup_css(&cgrp_dfl_root.cgrp, ss));
 	/* We don't handle early failures gracefully */
 	BUG_ON(IS_ERR(css));

>> +	 */
>> +	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?

The cache bitmask is really a bitmask where every bit represents a cache way , 
so its way based mapping . Although currently the max is 32 bits we still need 
to treat it as a bitmask.

In the first patch version you are the one who suggested to use all the bitmap 
functions to check the presence of contiguous bits (although I was already using 
the bitmaps, I had not used for some cases). So i made the change and other 
similar code was changed to using bitmap/bit manipulation APIs. There are 
scenarios where in we need to check for subset of 
bitmasks and other cases but agree they can all be done without the bitmask APIs 
as well. But now your comment contradicts the old comment ?

>
>> +	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.

This was due to PeterZ's feedback that the return paradigm needs to not have 
too many exit points or return a lot of times from the middle of code..
Both contradict now ?

>
>> +	}
>> +
>> +	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.

This is just printed once! how is that sprinke all over?   - we have a dmsg 
print for Cache monitoring as well when cqm is enabled.

>
> +out_err:
>
> -       return 0;
> +       return err;
>
>
> Thanks,
>
> 	tglx
>