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.l...@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:
