lkml.org 
[lkml]   [2015]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service management


On Mon, 18 May 2015, Thomas Gleixner wrote:

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

Ok - I saw this .. will remove underscores then since cpuset also dont use these
anyways.

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

Ok. was just wanting to make sure help debug just in case as its not used by
too many people as yet.

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

May be I misled you - I pasted the code from cgroup.c which is not part of cache
alloc below to show cgroup_init doesnt handle failure.. (cgroup_init_subsys ).
The cache allocation code doesnt return BUG_ON. So I never run into BUG_ON..
(right?)

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

ok can fix the comment. Reminds me of my middle school days when English was my
hardest subject.

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

This is a return for no memory - this is not for the call from
cgroup_init_subsys where failure turns into a BUG.. this is when user is trying
to create new cgroups..

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

I see - I did not notice this bitmap is not for the CBM though.. this is the
closmap which manages the closids. the cbm is just kept as a unsigned long - i
dont do bitmap allocation for that. So we are on same page i guess.

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

Great.

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

Will fix comment.

Thanks,
Vikas

>
> Thanks,
>
> tglx
>
>


\
 
 \ /
  Last update: 2015-05-18 22:21    [W:0.086 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site