Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752583AbcJJXr6 (ORCPT ); Mon, 10 Oct 2016 19:47:58 -0400 Received: from mga14.intel.com ([192.55.52.115]:37845 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752419AbcJJXr5 (ORCPT ); Mon, 10 Oct 2016 19:47:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,327,1473145200"; d="scan'208";a="18278549" Date: Mon, 10 Oct 2016 16:44:53 -0700 From: "Luck, Tony" To: Nilay Vaish Cc: Fenghua Yu , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , Peter Zijlstra , Stephane Eranian , Borislav Petkov , Dave Hansen , Shaohua Li , David Carrillo-Cisneros , Ravi V Shankar , Sai Prakhya , Vikas Shivappa , linux-kernel , x86 Subject: Re: [PATCH v3 11/18] x86/intel_rdt: Add basic resctrl filesystem support Message-ID: <20161010234453.GA11107@intel.com> References: <1475894763-64683-1-git-send-email-fenghua.yu@intel.com> <1475894763-64683-12-git-send-email-fenghua.yu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6798 Lines: 212 On Sun, Oct 09, 2016 at 05:31:25PM -0500, Nilay Vaish wrote: > > +static struct dentry *rdt_mount(struct file_system_type *fs_type, > > + int flags, const char *unused_dev_name, > > + void *data) > > +{ > > + struct dentry *dentry; > > + int ret; > > + bool new_sb; > > + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3]; > > + > > + mutex_lock(&rdtgroup_mutex); > > + /* > > + * resctrl file system can only be mounted once. > > + */ > > + if (static_branch_unlikely(&rdt_enable_key)) { > > + dentry = ERR_PTR(-EBUSY); > > + goto out; > > + } > > + > > + r->cdp_enabled = false; > > + ret = parse_rdtgroupfs_options(data, r); > > + if (ret) { > > + dentry = ERR_PTR(ret); > > + goto out; > > + } > > + if (r->cdp_enabled) > > + r->num_closid = r->max_closid / 2; > > + else > > + r->num_closid = r->max_closid; > > + > > + /* Recompute rdt_max_closid because CDP may have changed things. */ > > + rdt_max_closid = 0; > > + for_each_rdt_resource(r) > > + rdt_max_closid = max(rdt_max_closid, r->num_closid); > > + if (rdt_max_closid > 32) > > + rdt_max_closid = 32; > > + > > + dentry = kernfs_mount(fs_type, flags, rdt_root, > > + RDTGROUP_SUPER_MAGIC, &new_sb); > > + if (IS_ERR(dentry)) > > + goto out; > > + if (!new_sb) { > > + dentry = ERR_PTR(-EINVAL); > > + goto out; > > + } > > + r = &rdt_resources_all[RDT_RESOURCE_L3]; > > + if (r->cdp_capable) > > + set_l3_qos_cfg(r); > > + static_branch_enable(&rdt_enable_key); > > + > > +out: > > + mutex_unlock(&rdtgroup_mutex); > > + > > + return dentry; > > +} > > I am not too happy with the function above. I would have expected > that the function above executes the common code and also makes calls > to per resource functions. The way it has been written as now, it > seems to be mixing up things. It is a bit muddled. Below patch is on top of the series, but can be threaded back into appropriate parts. 1) Moves the r->cdp_enabled = false into parse_rdtgroupfs_options() 2) Move the rdt_max_closid re-computation out of mount() and into closid_init() [not in above as it is introduced later]. 3) Bonus ... gets rid of global "rdt_max_closid" as really nothing outside of closid_init() needs to know what it is. Better? -Tony commit 15a8ef34e2f849cdcb3e7faa72226cf2ecd82780 Author: Tony Luck Date: Mon Oct 10 16:18:31 2016 -0700 Respond to Nilay's comment that the "mount" code is mixed up. diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h index aefa3a655408..1104b214b972 100644 --- a/arch/x86/include/asm/intel_rdt.h +++ b/arch/x86/include/asm/intel_rdt.h @@ -136,9 +136,6 @@ enum { RDT_RESOURCE_L3, }; -/* Maximum CLOSID allowed across all enabled resoources */ -extern int rdt_max_closid; - /* CPUID.(EAX=10H, ECX=ResID=1).EAX */ union cpuid_0x10_1_eax { struct { diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c index e3c397306f1a..5c504abc9239 100644 --- a/arch/x86/kernel/cpu/intel_rdt.c +++ b/arch/x86/kernel/cpu/intel_rdt.c @@ -35,8 +35,6 @@ /* Mutex to protect rdtgroup access. */ DEFINE_MUTEX(rdtgroup_mutex); -int rdt_max_closid; - DEFINE_PER_CPU_READ_MOSTLY(int, cpu_closid); #define domain_init(name) LIST_HEAD_INIT(rdt_resources_all[name].domains) @@ -267,15 +265,6 @@ static int __init intel_rdt_late_init(void) if (ret < 0) return ret; - for_each_rdt_resource(r) - rdt_max_closid = max(rdt_max_closid, r->max_closid); - - /* limitation of our allocator, but h/w is more limited */ - if (rdt_max_closid > 32) { - pr_warn("Only using 32/%d CLOSIDs\n", rdt_max_closid); - rdt_max_closid = 32; - } - rdtgroup_init(); for_each_rdt_resource(r) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index 82f53ad935ca..5978a3742e5e 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -47,6 +47,23 @@ static int closid_free_map; static void closid_init(void) { + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3]; + int rdt_max_closid; + + /* Enabling L3 CDP halves the number of CLOSIDs */ + if (r->cdp_enabled) + r->num_closid = r->max_closid / 2; + else + r->num_closid = r->max_closid; + + /* Compute rdt_max_closid across all resources */ + rdt_max_closid = 0; + for_each_rdt_resource(r) + rdt_max_closid = max(rdt_max_closid, r->num_closid); + if (rdt_max_closid > 32) { + pr_warn_once("Only using 32/%d CLOSIDs\n", rdt_max_closid); + rdt_max_closid = 32; + } closid_free_map = BIT_MASK(rdt_max_closid) - 1; /* CLOSID 0 is always reserved for the default group */ @@ -546,10 +563,12 @@ static void set_l3_qos_cfg(struct rdt_resource *r) smp_call_function_many(&cpu_mask, l3_qos_cfg_update, r, 1); } -static int parse_rdtgroupfs_options(char *data, struct rdt_resource *r) +static int parse_rdtgroupfs_options(char *data) { char *token, *o = data; + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3]; + r->cdp_enabled = false; while ((token = strsep(&o, ",")) != NULL) { if (!*token) return -EINVAL; @@ -617,7 +636,6 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type, struct dentry *dentry; int ret; bool new_sb; - struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3]; mutex_lock(&rdtgroup_mutex); /* @@ -628,23 +646,11 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type, goto out; } - r->cdp_enabled = false; - ret = parse_rdtgroupfs_options(data, r); + ret = parse_rdtgroupfs_options(data); if (ret) { dentry = ERR_PTR(ret); goto out; } - if (r->cdp_enabled) - r->num_closid = r->max_closid / 2; - else - r->num_closid = r->max_closid; - - /* Recompute rdt_max_closid because CDP may have changed things. */ - rdt_max_closid = 0; - for_each_rdt_resource(r) - rdt_max_closid = max(rdt_max_closid, r->num_closid); - if (rdt_max_closid > 32) - rdt_max_closid = 32; closid_init(); dentry = kernfs_mount(fs_type, flags, rdt_root, @@ -655,9 +661,8 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type, dentry = ERR_PTR(-EINVAL); goto out; } - r = &rdt_resources_all[RDT_RESOURCE_L3]; - if (r->cdp_capable) - set_l3_qos_cfg(r); + if (rdt_resources_all[RDT_RESOURCE_L3].cdp_capable) + set_l3_qos_cfg(&rdt_resources_all[RDT_RESOURCE_L3]); static_branch_enable(&rdt_enable_key); out: