Message-ID: <alpine.DEB.2.11.1505182020030.4225@nanos>
Date:	Mon, 18 May 2015 20:41:53 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Vikas Shivappa <vikas.shivappa@...el.com>
cc:	Vikas Shivappa <vikas.shivappa@...ux.intel.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, hpa@...or.com, mingo@...nel.org,
	tj@...nel.org, peterz@...radead.org,
	Matt Fleming <matt.fleming@...el.com>,
	"Auld, Will" <will.auld@...el.com>, peter.zijlstra@...el.com,
	h.peter.anvin@...el.com,
	"Juvva, Kanaka D" <kanaka.d.juvva@...el.com>, mtosatti@...hat.com
Subject: Re: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service
 management

On Mon, 18 May 2015, Vikas Shivappa wrote:
> > > +static void __clos_get(unsigned int closid)
> > 
> > What's the purpose of these double underscores?
> > 
> 
> Similar to __get_rmid.

Errm. We use double underscore for such cases:

__bla()
{
	do_stuff();
}
	
bla()
{
	lock();
	__bla();
	unlock();
}

So you have two sorts of callers. Locked and unlocked. But I don't see
something like this. It's just random underscores for no value.

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

We add these things for complex locking scenarios, but for single
callsites where locking is obvious its not really valuable. But that's
the least of my worries.
 
> > > +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.

And then you run into the same BUG_ON ...

> 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));

I know. But still the comment is misleading.

  	/* 
	 * cgroup_init() expects  valid css pointer. Return
	 * the rdt_root_group.css instead of a failure code.
	 */

Can you see the difference?
 
> > > +	 */
> > > +	if (!parent)
> > > +		return &rdt_root_group.css;
> > > +
> > > +	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
> > > +	if (!ir)
> > > +		return ERR_PTR(-ENOMEM);

And why can't you return something useful here instead of letting the
thing run into a BUG?

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

Oh well. You can still use bitmap functions on an u64 where
appropriate.
 
> > 
> > > +	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 ?

There are different opinions about that. But again, that's the least
of my worries.
 
> > 
> > > +	}
> > > +
> > > +	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.

Sorry, mapped that to the wrong function. Though the message itself is
horrible. 

	  "Max bitmask length:32,Max ClosIds: 16"	

With some taste and formatting applied this would read:

     	  "Max. bitmask length: 32, max. closids: 16"

Can you spot the difference?

Thanks,

	tglx

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

Powered by blists - more mailing lists