[PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Shaohua Li]

rdtgroup_rmdir deletes a group and frees the closid, but doesn't
reiniaialize domain's cbm to default for the closid. Next time the
closid is allocated again, 'schemata' will show old cbm. We can do the
reinitialization at free/alloc closid, but sounds doing it at alloc is
more nartual.

Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 8af04af..7e81527 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -78,12 +78,21 @@ static void closid_init(void)
 int closid_alloc(void)
 {
 	int closid = ffs(closid_free_map);
+	struct rdt_resource *r;
+	struct rdt_domain *dom;
 
 	if (closid == 0)
 		return -ENOSPC;
 	closid--;
 	closid_free_map &= ~(1 << closid);
 
+	for_each_enabled_rdt_resource(r) {
+		if (closid >= r->num_closid)
+			continue;
+		list_for_each_entry(dom, &r->domains, list)
+			dom->cbm[closid] = r->max_cbm;
+	}
+
 	return closid;
 }
 
-- 
2.9.3

Re: [PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Fenghua Yu]

On Fri, Jan 06, 2017 at 04:05:19PM -0800, Shaohua Li wrote:
> rdtgroup_rmdir deletes a group and frees the closid, but doesn't
> reiniaialize domain's cbm to default for the closid. Next time the
> closid is allocated again, 'schemata' will show old cbm. We can do the
> reinitialization at free/alloc closid, but sounds doing it at alloc is
> more nartual.
> 
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index 8af04af..7e81527 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -78,12 +78,21 @@ static void closid_init(void)
>  int closid_alloc(void)
>  {
>  	int closid = ffs(closid_free_map);
> +	struct rdt_resource *r;
> +	struct rdt_domain *dom;
>  
>  	if (closid == 0)
>  		return -ENOSPC;
>  	closid--;
>  	closid_free_map &= ~(1 << closid);
>  
> +	for_each_enabled_rdt_resource(r) {
> +		if (closid >= r->num_closid)
> +			continue;
> +		list_for_each_entry(dom, &r->domains, list)
> +			dom->cbm[closid] = r->max_cbm;
> +	}
> +

This only changes the date stored in kernel software. But The cbm MSR
registers are not updated by the max_cbm. You need to call rdt_cbm_update
to update registers.

>  	return closid;
>  }
>  
> -- 
> 2.9.3
> 

I actually had a patch to fix this before but didn't send it out. I
thought this won't impact any functionality other than user may see some
unexpected schemata values, but user will always change schemata anyway
following mkdir.

But since you come here now,  I would think reseting the CBM in
closid_free() is better. The reason is user can see "right" max_cbm
even through rdmsr after rmdir, ie no gap for cbm values between rmdir and
the next mkdir.

Thanks.

-Fenghua

Re: [PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Thomas Gleixner]

On Fri, 6 Jan 2017, Shaohua Li wrote:

> rdtgroup_rmdir deletes a group and frees the closid, but doesn't
> reiniaialize domain's cbm to default for the closid. Next time the
> closid is allocated again, 'schemata' will show old cbm.

Rightfully so. Because that's the value which is in the configuration
MSR. That's perfectly fine because nothing can use that closid and therefor
the configuration MSR is completely irrelevant until the closid is reused.

> We can do the reinitialization at free/alloc closid, but sounds doing it
> at alloc is more nartual.

What you are doing is not natural, it's just wrong.

Assume the admin wants to keep the 'max_cbm' value for a domain and
therefor does not update the schemata file, but the MSR still contains the
old value. Not what you really want, right?

If you really want to have the CBM values reset to max_cbm then this needs
to be done when the directory is removed and this needs to be written to
the hardware as well on all affected packages. Not sure if that's worth the
effort.

Thanks,

	tglx

Re: [PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Thomas Gleixner]

On Mon, 9 Jan 2017, Fenghua Yu wrote:
> On Fri, Jan 06, 2017 at 04:05:19PM -0800, Shaohua Li wrote:
> But since you come here now,  I would think reseting the CBM in
> closid_free() is better.

No. closid_free() is the wrong place. closid_alloc/free() merily deal with
the bitmap and nothing else.

If you really want to do that, then this needs a seperate function called
from rmdir.

> The reason is user can see "right" max_cbm even through rdmsr after
> rmdir, ie no gap for cbm values between rmdir and the next mkdir.

And the value pf this is?

The closid is not usable after the rmdir, so it's really completely
uninteresting when the user can read the old value from that configuration
MSR.

When the closid is reused then it hardly matters either what's in those cbm
values (the old or max_cbm).

Thanks,

	tglx

Re: [PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Fenghua Yu]

On Mon, Jan 09, 2017 at 11:03:59PM +0100, Thomas Gleixner wrote:
> On Mon, 9 Jan 2017, Fenghua Yu wrote:
> > On Fri, Jan 06, 2017 at 04:05:19PM -0800, Shaohua Li wrote:
> > But since you come here now,  I would think reseting the CBM in
> > closid_free() is better.
> 
> No. closid_free() is the wrong place. closid_alloc/free() merily deal with
> the bitmap and nothing else.
> 
> If you really want to do that, then this needs a seperate function called
> from rmdir.
> 
> > The reason is user can see "right" max_cbm even through rdmsr after
> > rmdir, ie no gap for cbm values between rmdir and the next mkdir.
> 
> And the value pf this is?

Not very useful value, but user may get a bit confused by a non max_cbm if
a closid is not used after rdmsr. A couple of users actually asked this.
Reseting to initial max_cbm may reduce questions from users.

> 
> The closid is not usable after the rmdir, so it's really completely
> uninteresting when the user can read the old value from that configuration
> MSR.

Agree with you. The cbm value is uninteresting after rmdir.

> 
> When the closid is reused then it hardly matters either what's in those cbm
> values (the old or max_cbm).
> 
> Thanks,
> 
> 	tglx

Re: [PATCH] x86/intel_rdt: reinitialize cbm for new group allocation

[Shaohua Li]

On Mon, Jan 09, 2017 at 11:03:59PM +0100, Thomas Gleixner wrote:
> On Mon, 9 Jan 2017, Fenghua Yu wrote:
> > On Fri, Jan 06, 2017 at 04:05:19PM -0800, Shaohua Li wrote:
> > But since you come here now,  I would think reseting the CBM in
> > closid_free() is better.
> 
> No. closid_free() is the wrong place. closid_alloc/free() merily deal with
> the bitmap and nothing else.
> 
> If you really want to do that, then this needs a seperate function called
> from rmdir.
> 
> > The reason is user can see "right" max_cbm even through rdmsr after
> > rmdir, ie no gap for cbm values between rmdir and the next mkdir.
> 
> And the value pf this is?
> 
> The closid is not usable after the rmdir, so it's really completely
> uninteresting when the user can read the old value from that configuration
> MSR.
> 
> When the closid is reused then it hardly matters either what's in those cbm
> values (the old or max_cbm).

It's just a bit confusing. Ok, I'm fine with current behavior. Probably
document it somewhere in case others found the same issue.

Thanks,
Shaohua