Messages in this thread | | | Date | Mon, 18 May 2015 12:44:33 -0700 (PDT) | From | Vikas Shivappa <> | Subject | Re: [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 > >
| |