From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932885AbbERTqD (ORCPT ); Mon, 18 May 2015 15:46:03 -0400 Received: from mga02.intel.com ([134.134.136.20]:46550 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932114AbbERTp6 (ORCPT ); Mon, 18 May 2015 15:45:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,454,1427785200"; d="scan'208";a="727939573" Date: Mon, 18 May 2015 12:44:33 -0700 (PDT) From: Vikas Shivappa X-X-Sender: vikas@vshiva-Udesk To: Thomas Gleixner cc: Vikas Shivappa , Vikas Shivappa , x86@kernel.org, linux-kernel@vger.kernel.org, hpa@zytor.com, mingo@kernel.org, tj@kernel.org, peterz@infradead.org, Matt Fleming , "Auld, Will" , peter.zijlstra@intel.com, h.peter.anvin@intel.com, "Juvva, Kanaka D" , mtosatti@redhat.com Subject: Re: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service management In-Reply-To: Message-ID: References: <1431370976-31115-1-git-send-email-vikas.shivappa@linux.intel.com> <1431370976-31115-3-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >