Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Shivappa Vikas]

On Mon, 10 Apr 2017, Thomas Gleixner wrote:


On Wed, 5 Apr 2017, Luck, Tony wrote:

On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:

That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.


I don't see that as any more helpful. When you make a new
control group it is because none of the existing groups
provides the QoS that you want.  So the first thing the
user will do is write the schemata file with the values
they do want.

So "all access", or "same as default group" are both the
same to the user ... not what they want.

We do need to make sure that the schemata matches what is
in the registers. We need to make sure that changes to the
schemata file result in the MSRs being written where needed.


That's true today. The MSRs and the schemata file of a newly created group
always match. So there's nothing to fix, right?


Yes. Upstream patch has the MSRs matching with what schemata shows. we can drop 
this patch. I sent a new MBA version without this just on the tip which has the 
other two patches.


Thanks,
Vikas



Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Shivappa Vikas]

On Mon, 10 Apr 2017, Thomas Gleixner wrote:


On Wed, 5 Apr 2017, Luck, Tony wrote:

On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:

That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.


I don't see that as any more helpful. When you make a new
control group it is because none of the existing groups
provides the QoS that you want.  So the first thing the
user will do is write the schemata file with the values
they do want.

So "all access", or "same as default group" are both the
same to the user ... not what they want.

We do need to make sure that the schemata matches what is
in the registers. We need to make sure that changes to the
schemata file result in the MSRs being written where needed.


That's true today. The MSRs and the schemata file of a newly created group
always match. So there's nothing to fix, right?


Yes. Upstream patch has the MSRs matching with what schemata shows. we can drop 
this patch. I sent a new MBA version without this just on the tip which has the 
other two patches.


Thanks,
Vikas



Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Thomas Gleixner]

On Wed, 5 Apr 2017, Luck, Tony wrote:
> On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> > That's just wrong.
> > 
> > The proper behaviour for a new control group is, that at the time when it
> > is created it copies the CBM values of the default group and not claiming
> > access to ALL of the cache by default.
> 
> I don't see that as any more helpful. When you make a new
> control group it is because none of the existing groups
> provides the QoS that you want.  So the first thing the
> user will do is write the schemata file with the values
> they do want.
> 
> So "all access", or "same as default group" are both the
> same to the user ... not what they want.
> 
> We do need to make sure that the schemata matches what is
> in the registers. We need to make sure that changes to the
> schemata file result in the MSRs being written where needed.

That's true today. The MSRs and the schemata file of a newly created group
always match. So there's nothing to fix, right?

Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Thomas Gleixner]

On Wed, 5 Apr 2017, Luck, Tony wrote:
> On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> > That's just wrong.
> > 
> > The proper behaviour for a new control group is, that at the time when it
> > is created it copies the CBM values of the default group and not claiming
> > access to ALL of the cache by default.
> 
> I don't see that as any more helpful. When you make a new
> control group it is because none of the existing groups
> provides the QoS that you want.  So the first thing the
> user will do is write the schemata file with the values
> they do want.
> 
> So "all access", or "same as default group" are both the
> same to the user ... not what they want.
> 
> We do need to make sure that the schemata matches what is
> in the registers. We need to make sure that changes to the
> schemata file result in the MSRs being written where needed.

That's true today. The MSRs and the schemata file of a newly created group
always match. So there's nothing to fix, right?

Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Fenghua Yu]

On Wed, Apr 05, 2017 at 11:07:37AM -0700, Luck, Tony wrote:
> On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> > That's just wrong.
> > 
> > The proper behaviour for a new control group is, that at the time when it
> > is created it copies the CBM values of the default group and not claiming
> > access to ALL of the cache by default.
> 
> I don't see that as any more helpful. When you make a new
> control group it is because none of the existing groups
> provides the QoS that you want.  So the first thing the
> user will do is write the schemata file with the values
> they do want.
> 
> So "all access", or "same as default group" are both the
> same to the user ... not what they want.
> 
> We do need to make sure that the schemata matches what is
> in the registers. We need to make sure that changes to the
> schemata file result in the MSRs being written where needed.

FYI. This behavior and a fix patch were discussed before:
http://lkml.org/lkml/2017/1/9/747

Thanks.

-Fenghua

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Fenghua Yu]

On Wed, Apr 05, 2017 at 11:07:37AM -0700, Luck, Tony wrote:
> On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> > That's just wrong.
> > 
> > The proper behaviour for a new control group is, that at the time when it
> > is created it copies the CBM values of the default group and not claiming
> > access to ALL of the cache by default.
> 
> I don't see that as any more helpful. When you make a new
> control group it is because none of the existing groups
> provides the QoS that you want.  So the first thing the
> user will do is write the schemata file with the values
> they do want.
> 
> So "all access", or "same as default group" are both the
> same to the user ... not what they want.
> 
> We do need to make sure that the schemata matches what is
> in the registers. We need to make sure that changes to the
> schemata file result in the MSRs being written where needed.

FYI. This behavior and a fix patch were discussed before:
http://lkml.org/lkml/2017/1/9/747

Thanks.

-Fenghua

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Luck, Tony]

On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> That's just wrong.
> 
> The proper behaviour for a new control group is, that at the time when it
> is created it copies the CBM values of the default group and not claiming
> access to ALL of the cache by default.

I don't see that as any more helpful. When you make a new
control group it is because none of the existing groups
provides the QoS that you want.  So the first thing the
user will do is write the schemata file with the values
they do want.

So "all access", or "same as default group" are both the
same to the user ... not what they want.

We do need to make sure that the schemata matches what is
in the registers. We need to make sure that changes to the
schemata file result in the MSRs being written where needed.

-Tony

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Luck, Tony]

On Wed, Apr 05, 2017 at 05:20:24PM +0200, Thomas Gleixner wrote:
> That's just wrong.
> 
> The proper behaviour for a new control group is, that at the time when it
> is created it copies the CBM values of the default group and not claiming
> access to ALL of the cache by default.

I don't see that as any more helpful. When you make a new
control group it is because none of the existing groups
provides the QoS that you want.  So the first thing the
user will do is write the schemata file with the values
they do want.

So "all access", or "same as default group" are both the
same to the user ... not what they want.

We do need to make sure that the schemata matches what is
in the registers. We need to make sure that changes to the
schemata file result in the MSRs being written where needed.

-Tony

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Shivappa Vikas]

On Wed, 5 Apr 2017, Thomas Gleixner wrote:


On Mon, 3 Apr 2017, Vikas Shivappa wrote:


Subject: x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid


This subject line is useless again. It want's to be descriptive.

"Fix issue" Which issue?


Each resctrl directory has one CLOSid allocated which is mapped to a
control register/QOS_MSR. During an rmdir this CLOSid is freed and can
be reused later when a new directory is created.  Currently we do not
reset the QOS_MSR to a default when the CLOSid is freed. So when the
next mkdir uses a freed CLOSid, the new directory is associated with a
stale QOS_MSR.


That's not an issue. That's maybe something people are not expecting.


Fix this issue by writing a default value to the QOS_MSR when the
associated CLOSid is freed. The default value is all 1s for CBM which
means no control is enforced when a mkdir reuses this CLOSid.


That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.


The behaviour of the default/root group is having access to all cache. Because 
we set the value of all CBMs to all 1s and then root group takes CLOS 0. This is 
trying to set the same values for all newly created groups.


Thanks,
Vikas



Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Shivappa Vikas]

On Wed, 5 Apr 2017, Thomas Gleixner wrote:


On Mon, 3 Apr 2017, Vikas Shivappa wrote:


Subject: x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid


This subject line is useless again. It want's to be descriptive.

"Fix issue" Which issue?


Each resctrl directory has one CLOSid allocated which is mapped to a
control register/QOS_MSR. During an rmdir this CLOSid is freed and can
be reused later when a new directory is created.  Currently we do not
reset the QOS_MSR to a default when the CLOSid is freed. So when the
next mkdir uses a freed CLOSid, the new directory is associated with a
stale QOS_MSR.


That's not an issue. That's maybe something people are not expecting.


Fix this issue by writing a default value to the QOS_MSR when the
associated CLOSid is freed. The default value is all 1s for CBM which
means no control is enforced when a mkdir reuses this CLOSid.


That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.


The behaviour of the default/root group is having access to all cache. Because 
we set the value of all CBMs to all 1s and then root group takes CLOS 0. This is 
trying to set the same values for all newly created groups.


Thanks,
Vikas



Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Thomas Gleixner]

On Mon, 3 Apr 2017, Vikas Shivappa wrote:

> Subject: x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

This subject line is useless again. It want's to be descriptive.

"Fix issue" Which issue?

> Each resctrl directory has one CLOSid allocated which is mapped to a
> control register/QOS_MSR. During an rmdir this CLOSid is freed and can
> be reused later when a new directory is created.  Currently we do not
> reset the QOS_MSR to a default when the CLOSid is freed. So when the
> next mkdir uses a freed CLOSid, the new directory is associated with a
> stale QOS_MSR.

That's not an issue. That's maybe something people are not expecting.

> Fix this issue by writing a default value to the QOS_MSR when the
> associated CLOSid is freed. The default value is all 1s for CBM which
> means no control is enforced when a mkdir reuses this CLOSid.

That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.

Thanks,

tglx

Re: [PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Thomas Gleixner]

On Mon, 3 Apr 2017, Vikas Shivappa wrote:

> Subject: x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

This subject line is useless again. It want's to be descriptive.

"Fix issue" Which issue?

> Each resctrl directory has one CLOSid allocated which is mapped to a
> control register/QOS_MSR. During an rmdir this CLOSid is freed and can
> be reused later when a new directory is created.  Currently we do not
> reset the QOS_MSR to a default when the CLOSid is freed. So when the
> next mkdir uses a freed CLOSid, the new directory is associated with a
> stale QOS_MSR.

That's not an issue. That's maybe something people are not expecting.

> Fix this issue by writing a default value to the QOS_MSR when the
> associated CLOSid is freed. The default value is all 1s for CBM which
> means no control is enforced when a mkdir reuses this CLOSid.

That's just wrong.

The proper behaviour for a new control group is, that at the time when it
is created it copies the CBM values of the default group and not claiming
access to ALL of the cache by default.

Thanks,

tglx

[PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Vikas Shivappa]

Each resctrl directory has one CLOSid allocated which is mapped to a
control register/QOS_MSR. During an rmdir this CLOSid is freed and can
be reused later when a new directory is created.  Currently we do not
reset the QOS_MSR to a default when the CLOSid is freed. So when the
next mkdir uses a freed CLOSid, the new directory is associated with a
stale QOS_MSR.

Fix this issue by writing a default value to the QOS_MSR when the
associated CLOSid is freed. The default value is all 1s for CBM which
means no control is enforced when a mkdir reuses this CLOSid.

Signed-off-by: Vikas Shivappa 
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c 
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 9ac2a5c..77e88c0 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -780,7 +780,7 @@ static struct dentry *rdt_mount(struct file_system_type 
*fs_type,
return dentry;
 }
 
-static int reset_all_cbms(struct rdt_resource *r)
+static int reset_all_ctrls(struct rdt_resource *r, u32 closid, u32 count)
 {
struct msr_param msr_param;
cpumask_var_t cpu_mask;
@@ -791,8 +791,8 @@ static int reset_all_cbms(struct rdt_resource *r)
return -ENOMEM;
 
msr_param.res = r;
-   msr_param.low = 0;
-   msr_param.high = r->num_closid;
+   msr_param.low = closid;
+   msr_param.high = closid + count;
 
/*
 * Disable resource control for this resource by setting all
@@ -802,7 +802,7 @@ static int reset_all_cbms(struct rdt_resource *r)
list_for_each_entry(d, >domains, list) {
cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
 
-   for (i = 0; i < r->num_closid; i++)
+   for (i = closid; i < closid + count; i++)
d->cbm[i] = r->max_cbm;
}
cpu = get_cpu();
@@ -896,7 +896,7 @@ static void rdt_kill_sb(struct super_block *sb)
 
/*Put everything back to default values. */
for_each_enabled_rdt_resource(r)
-   reset_all_cbms(r);
+   reset_all_ctrls(r, 0, r->num_closid);
cdp_disable();
rmdir_all_sub();
static_branch_disable(_enable_key);
@@ -991,6 +991,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 {
int ret, cpu, closid = rdtgroup_default.closid;
struct rdtgroup *rdtgrp;
+   struct rdt_resource *r;
cpumask_var_t tmpmask;
 
if (!zalloc_cpumask_var(, GFP_KERNEL))
@@ -1019,6 +1020,13 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
cpumask_or(tmpmask, tmpmask, >cpu_mask);
rdt_update_closid(tmpmask, NULL);
 
+   /*
+* Put domain control values back to default for the
+* rdtgrp thats being removed.
+*/
+   for_each_enabled_rdt_resource(r)
+   reset_all_ctrls(r, rdtgrp->closid, 1);
+
rdtgrp->flags = RDT_DELETED;
closid_free(rdtgrp->closid);
list_del(>rdtgroup_list);
-- 
1.9.1

[PATCH 1/3] x86/intel_rdt: Fix issue when mkdir uses a freed CLOSid

[Vikas Shivappa]

Each resctrl directory has one CLOSid allocated which is mapped to a
control register/QOS_MSR. During an rmdir this CLOSid is freed and can
be reused later when a new directory is created.  Currently we do not
reset the QOS_MSR to a default when the CLOSid is freed. So when the
next mkdir uses a freed CLOSid, the new directory is associated with a
stale QOS_MSR.

Fix this issue by writing a default value to the QOS_MSR when the
associated CLOSid is freed. The default value is all 1s for CBM which
means no control is enforced when a mkdir reuses this CLOSid.

Signed-off-by: Vikas Shivappa 
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c 
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 9ac2a5c..77e88c0 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -780,7 +780,7 @@ static struct dentry *rdt_mount(struct file_system_type 
*fs_type,
return dentry;
 }
 
-static int reset_all_cbms(struct rdt_resource *r)
+static int reset_all_ctrls(struct rdt_resource *r, u32 closid, u32 count)
 {
struct msr_param msr_param;
cpumask_var_t cpu_mask;
@@ -791,8 +791,8 @@ static int reset_all_cbms(struct rdt_resource *r)
return -ENOMEM;
 
msr_param.res = r;
-   msr_param.low = 0;
-   msr_param.high = r->num_closid;
+   msr_param.low = closid;
+   msr_param.high = closid + count;
 
/*
 * Disable resource control for this resource by setting all
@@ -802,7 +802,7 @@ static int reset_all_cbms(struct rdt_resource *r)
list_for_each_entry(d, >domains, list) {
cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
 
-   for (i = 0; i < r->num_closid; i++)
+   for (i = closid; i < closid + count; i++)
d->cbm[i] = r->max_cbm;
}
cpu = get_cpu();
@@ -896,7 +896,7 @@ static void rdt_kill_sb(struct super_block *sb)
 
/*Put everything back to default values. */
for_each_enabled_rdt_resource(r)
-   reset_all_cbms(r);
+   reset_all_ctrls(r, 0, r->num_closid);
cdp_disable();
rmdir_all_sub();
static_branch_disable(_enable_key);
@@ -991,6 +991,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 {
int ret, cpu, closid = rdtgroup_default.closid;
struct rdtgroup *rdtgrp;
+   struct rdt_resource *r;
cpumask_var_t tmpmask;
 
if (!zalloc_cpumask_var(, GFP_KERNEL))
@@ -1019,6 +1020,13 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
cpumask_or(tmpmask, tmpmask, >cpu_mask);
rdt_update_closid(tmpmask, NULL);
 
+   /*
+* Put domain control values back to default for the
+* rdtgrp thats being removed.
+*/
+   for_each_enabled_rdt_resource(r)
+   reset_all_ctrls(r, rdtgrp->closid, 1);
+
rdtgrp->flags = RDT_DELETED;
closid_free(rdtgrp->closid);
list_del(>rdtgroup_list);
-- 
1.9.1