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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
