From: "Luck, Tony" <tony.luck@intel.com>
To: Nilay Vaish <nilayvaish@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <h.peter.anvin@intel.com>,
Ingo Molnar <mingo@elte.hu>,
Peter Zijlstra <peterz@infradead.org>,
Stephane Eranian <eranian@google.com>,
Borislav Petkov <bp@suse.de>, Dave Hansen <dave.hansen@intel.com>,
Shaohua Li <shli@fb.com>,
David Carrillo-Cisneros <davidcc@google.com>,
Ravi V Shankar <ravi.v.shankar@intel.com>,
Sai Prakhya <sai.praneeth.prakhya@intel.com>,
Vikas Shivappa <vikas.shivappa@linux.intel.com>,
linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>
Subject: Re: [PATCH v3 11/18] x86/intel_rdt: Add basic resctrl filesystem support
Date: Mon, 10 Oct 2016 16:44:53 -0700 [thread overview]
Message-ID: <20161010234453.GA11107@intel.com> (raw)
In-Reply-To: <CACbG30-O0-HmCX3kuuNwoPEc_6GzDuzePL0fFy-4s8QxuQ9PmA@mail.gmail.com>
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 <tony.luck@intel.com>
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:
next prev parent reply other threads:[~2016-10-10 23:47 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-08 2:45 [PATCH v3 00/18] Intel Cache Allocation Technology Fenghua Yu
2016-10-07 23:54 ` [RFC PATCH 19/18] x86/intel_rdt: Add support for L2 cache allocation Luck, Tony
2016-10-08 2:45 ` [PATCH v3 01/18] Documentation, ABI: Add a document entry for cache id Fenghua Yu
2016-10-08 17:11 ` Nilay Vaish
2016-10-10 16:45 ` Luck, Tony
2016-10-11 16:48 ` Nilay Vaish
2016-10-08 2:45 ` [PATCH v3 02/18] cacheinfo: Introduce " Fenghua Yu
2016-10-08 17:10 ` Nilay Vaish
2016-10-08 2:45 ` [PATCH v3 03/18] x86, intel_cacheinfo: Enable cache id in x86 Fenghua Yu
2016-10-08 2:45 ` [PATCH v3 04/18] x86/intel_rdt: Feature discovery Fenghua Yu
2016-10-08 17:11 ` Nilay Vaish
2016-10-08 20:54 ` Fenghua Yu
2016-10-08 19:52 ` Borislav Petkov
2016-10-11 16:57 ` Nilay Vaish
2016-10-11 17:03 ` Borislav Petkov
2016-10-10 16:01 ` Dave Hansen
2016-10-10 16:18 ` Borislav Petkov
2016-10-08 2:45 ` [PATCH v3 05/18] Documentation, x86: Documentation for Intel resource allocation user interface Fenghua Yu
2016-10-08 17:12 ` Nilay Vaish
2016-10-08 20:33 ` Fenghua Yu
2016-10-10 17:19 ` Luck, Tony
2016-10-11 17:07 ` Nilay Vaish
2016-10-11 18:04 ` Luck, Tony
2016-10-08 2:45 ` [PATCH v3 06/18] x86/intel_rdt: Add CONFIG, Makefile, and basic initialization Fenghua Yu
2016-10-08 20:57 ` Borislav Petkov
2016-10-08 2:45 ` [PATCH v3 07/18] x86/intel_rdt: Add Haswell feature discovery Fenghua Yu
2016-10-09 11:41 ` Borislav Petkov
2016-10-09 17:09 ` Fenghua Yu
2016-10-09 16:28 ` Borislav Petkov
2016-10-10 18:55 ` Luck, Tony
2016-10-11 11:12 ` Borislav Petkov
2016-10-11 14:51 ` Luck, Tony
2016-10-08 2:45 ` [PATCH v3 08/18] x86/intel_rdt: Pick up L3 RDT parameters from CPUID Fenghua Yu
2016-10-08 2:45 ` [PATCH v3 09/18] x86/cqm: Move PQR_ASSOC management code into generic code used by both CQM and CAT Fenghua Yu
2016-10-08 2:45 ` [PATCH v3 10/18] x86/intel_rdt: Build structures for each resource based on cache topology Fenghua Yu
2016-10-09 21:57 ` Nilay Vaish
2016-10-08 2:45 ` [PATCH v3 11/18] x86/intel_rdt: Add basic resctrl filesystem support Fenghua Yu
2016-10-09 22:31 ` Nilay Vaish
2016-10-10 23:44 ` Luck, Tony [this message]
2016-10-08 2:45 ` [PATCH v3 12/18] x86/intel_rdt: Add "info" files to resctrl file system Fenghua Yu
2016-10-08 2:45 ` [PATCH v3 13/18] x86/intel_rdt: Add mkdir " Fenghua Yu
2016-10-10 17:51 ` Nilay Vaish
2016-10-08 2:45 ` [PATCH v3 14/18] x86/intel_rdt: Add cpus file Fenghua Yu
2016-10-08 2:46 ` [PATCH v3 15/18] x86/intel_rdt: Add tasks files Fenghua Yu
2016-10-08 2:46 ` [PATCH v3 16/18] x86/intel_rdt: Add schemata file Fenghua Yu
2016-10-08 2:46 ` [PATCH v3 17/18] x86/intel_rdt: Add scheduler hook Fenghua Yu
2016-10-08 2:46 ` [PATCH v3 18/18] MAINTAINERS: Add maintainer for Intel RDT resource allocation Fenghua Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161010234453.GA11107@intel.com \
--to=tony.luck@intel.com \
--cc=bp@suse.de \
--cc=dave.hansen@intel.com \
--cc=davidcc@google.com \
--cc=eranian@google.com \
--cc=fenghua.yu@intel.com \
--cc=h.peter.anvin@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=nilayvaish@gmail.com \
--cc=peterz@infradead.org \
--cc=ravi.v.shankar@intel.com \
--cc=sai.praneeth.prakhya@intel.com \
--cc=shli@fb.com \
--cc=tglx@linutronix.de \
--cc=vikas.shivappa@linux.intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).