From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754854AbbERSlw (ORCPT ); Mon, 18 May 2015 14:41:52 -0400 Received: from www.linutronix.de ([62.245.132.108]:50461 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754407AbbERSlt (ORCPT ); Mon, 18 May 2015 14:41:49 -0400 Date: Mon, 18 May 2015 20:41:53 +0200 (CEST) From: Thomas Gleixner To: Vikas Shivappa cc: 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.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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