From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755597AbdCJVu5 (ORCPT ); Fri, 10 Mar 2017 16:50:57 -0500 Received: from mga01.intel.com ([192.55.52.88]:32574 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755461AbdCJVut (ORCPT ); Fri, 10 Mar 2017 16:50:49 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,143,1486454400"; d="scan'208";a="58790422" Date: Fri, 10 Mar 2017 13:51:46 -0800 (PST) From: Shivappa Vikas X-X-Sender: vikas@vshiva-Udesk To: Thomas Gleixner cc: Vikas Shivappa , vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org, hpa@zytor.com, mingo@kernel.org, peterz@infradead.org, ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com, andi.kleen@intel.com Subject: Re: [PATCH 4/8] x86/intel_rct/mba: Add MBA structures and initialize MBA In-Reply-To: Message-ID: References: <1487361535-9727-1-git-send-email-vikas.shivappa@linux.intel.com> <1487361535-9727-5-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 Wed, 1 Mar 2017, Thomas Gleixner wrote: > On Fri, 17 Feb 2017, Vikas Shivappa wrote: >> --- a/arch/x86/include/asm/intel_rdt.h >> +++ b/arch/x86/include/asm/intel_rdt.h >> @@ -11,6 +11,9 @@ >> #define IA32_L3_QOS_CFG 0xc81 >> #define IA32_L3_CBM_BASE 0xc90 >> #define IA32_L2_CBM_BASE 0xd10 >> +#define IA32_MBA_THRTL_BASE 0xd50 >> +#define MAX_MBA_THRTL 100u >> +#define MBA_IS_LINEAR 0x4 > > I have a hard time to figure out how the latter two constants are related > to this list of registers. MBA_IS_LINEAR is used to check the CPUID bit and > MAX_MBA_THRTL is obviously a pure software constant because with a > non-linear scale the maximum value is not 100. > > Just slapping defines to random places is equally bad as using hard coded > constants. > >> +/* >> + * rdt_get_mb_table() - get a mapping of b/w percentage values >> + * exposed to user interface and the h/w understandable delay values. >> + * >> + * The non-linear delay values have the granularity of power of two >> + * and also the h/w does not guarantee a curve for configured delay >> + * values vs. actual b/w throttled. >> + * Hence we need a mapping that is pre caliberated for user to express >> + * the b/w in terms of any sensible number. > > ... calibrated so the user can express the bandwidth as a percentage value. > >> +static inline int rdt_get_mb_table(struct rdt_resource *r) >> +{ >> + /* >> + * There are no Intel SKUs as of now to support non-linear delay. >> + */ >> + r->mb_map = NULL; > > What's the point of setting this to NULL? > > Also it would be helpful to emit log info here so people don't have to > start digging around. > > pr_info("Bandwidth map not implemented for ....", ... model); > >> + >> + return -ENODEV; > > Returning -ENODEV to a function which just returns a boolean value is > pointless. > >> static void rdt_get_cache_config(int idx, struct rdt_resource *r) >> { >> union cpuid_0x10_1_eax eax; >> @@ -184,9 +237,8 @@ static inline bool get_rdt_resources(void) >> ret = true; >> } >> >> - if (boot_cpu_has(X86_FEATURE_MBA)) { >> - ret = true; >> - } >> + if (boot_cpu_has(X86_FEATURE_MBA)) >> + ret = rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]); > > Groan. When rdt_get_mem_config() returns false (because the map is not > implemented), then the whole function returns false and CAT is disabled. > >> +static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d) >> +{ >> + int i; >> + >> + d->ctrl_val = kmalloc_array(r->num_closid, >> + sizeof(*d->ctrl_val), GFP_KERNEL); >> + if (!d->ctrl_val) >> + return -ENOMEM; >> + >> + /* >> + * Initialize the Control MSRs to having no control. >> + * For Cache Allocation: Set all bits in cbm >> + * For Memory Allocation: Set b/w requested to 100 >> + */ >> + for (i = 0; i < r->num_closid; i++) { >> + int idx = cbm_idx(r, i); >> + >> + d->ctrl_val[i] = r->default_ctrl; >> + wrmsrl(r->msr_base + idx, d->ctrl_val[i]); >> + } > > So if you use a local pointer for that, this whole mess becomes readable. > > static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d) > { > u32 *p; > int i; > > p = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL); > if (!p) > return -ENOMEM; > > d->ctrl_val = p; > > /* Initialize the Control MSRs to the default value */ > for (i = 0; i < r->num_closid; i++, p++) { > int idx = cbm_idx(r, i); > > *p = r->default_ctrl; > wrmsrl(r->msr_base + idx, *p); > } >> + >> + return 0; >> +} > >> static void domain_add_cpu(int cpu, struct rdt_resource *r) >> { >> - int i, id = get_cache_id(cpu, r->cache_level); >> + int id = get_cache_id(cpu, r->cache_level), ret; > > Bah. If you have the same type in one line, then please move the > uninitialized variables to the front. > > int ret, id = get_cache_id(cpu, r->cache_level); > > But a s/i/ret/ would have been to simple and kept the code readable. > >> @@ -298,19 +374,12 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r) >> >> d->id = id; >> >> - d->ctrl_val = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL); >> - if (!d->ctrl_val) { >> + ret = domain_setup_ctrlval(r, d); >> + if (ret) { >> kfree(d); >> return; >> } > > What's the point of this 'ret' variable if the function is void? > > if (domain_setup_ctrlval(r, d)) { > kfree(d); > return; > } > > would have been to easy to read, right? Will fix all the issues pointed. Thanks for pointing out. > > Thanks, > > tglx >