Resctrl assumes that all CPUs are online when the filesystem is
mounted, and that CPUs remember their CDP-enabled state over CPU
hotplug.
This goes wrong when resctrl's CDP-enabled state changes while all
the CPUs in a domain are offline.
When a domain comes online, enable (or disable!) CDP to match resctrl's
current setting.
Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
arch/x86/kernel/cpu/resctrl/core.c | 2 ++
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
3 files changed, 16 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 89049b343c7a..d8cc5223b7ce 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -578,6 +578,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
d->id = id;
cpumask_set_cpu(cpu, &d->cpu_mask);
+ rdt_domain_reconfigure_cdp(r);
+
if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
kfree(d);
return;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 181c992f448c..3dd13f3a8b23 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -601,5 +601,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
void __check_limbo(struct rdt_domain *d, bool force_free);
bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
+void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 064e9ef44cd6..1c78908ef395 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1859,6 +1859,19 @@ static int set_cache_qos_cfg(int level, bool enable)
return 0;
}
+/* Restore the qos cfg state when a domain comes online */
+void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
+{
+ if (!r->alloc_capable)
+ return;
+
+ if (r == &rdt_resources_all[RDT_RESOURCE_L2DATA])
+ l2_qos_cfg_update(&r->alloc_enabled);
+
+ if (r == &rdt_resources_all[RDT_RESOURCE_L3DATA])
+ l3_qos_cfg_update(&r->alloc_enabled);
+}
+
/*
* Enable or disable the MBA software controller
* which helps user specify bandwidth in MBps.
--
2.24.1
Hi James,
On 2/21/2020 8:21 AM, James Morse wrote:
> Resctrl assumes that all CPUs are online when the filesystem is
> mounted, and that CPUs remember their CDP-enabled state over CPU
> hotplug.
>
> This goes wrong when resctrl's CDP-enabled state changes while all
> the CPUs in a domain are offline.
>
> When a domain comes online, enable (or disable!) CDP to match resctrl's
> current setting.
>
> Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
> Suggested-by: Reinette Chatre <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 2 ++
> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 89049b343c7a..d8cc5223b7ce 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -578,6 +578,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
> d->id = id;
> cpumask_set_cpu(cpu, &d->cpu_mask);
>
> + rdt_domain_reconfigure_cdp(r);
> +
> if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
> kfree(d);
> return;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 181c992f448c..3dd13f3a8b23 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -601,5 +601,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
> void __check_limbo(struct rdt_domain *d, bool force_free);
> bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
> bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>
> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 064e9ef44cd6..1c78908ef395 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1859,6 +1859,19 @@ static int set_cache_qos_cfg(int level, bool enable)
> return 0;
> }
>
> +/* Restore the qos cfg state when a domain comes online */
> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
> +{
> + if (!r->alloc_capable)
> + return;
> +
> + if (r == &rdt_resources_all[RDT_RESOURCE_L2DATA])
> + l2_qos_cfg_update(&r->alloc_enabled);
> +
> + if (r == &rdt_resources_all[RDT_RESOURCE_L3DATA])
> + l3_qos_cfg_update(&r->alloc_enabled);
> +}
> +
> /*
> * Enable or disable the MBA software controller
> * which helps user specify bandwidth in MBps.
>
As mentioned in my response to v2 the lockdep annotation that formed
part of this fix is welcome. It is not clear to me if you will be
submitting again with the annotation added back. Since it is not
required for this fix I will add my tag here and you could include it if
you do decide to resubmit.
Thank you
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
Hi Thomas and Borislav,
Could you please consider this patch for inclusion as a fix for v5.7?
Thank you
Reinette
On 2/24/2020 10:23 AM, Reinette Chatre wrote:
> Hi James,
>
> On 2/21/2020 8:21 AM, James Morse wrote:
>> Resctrl assumes that all CPUs are online when the filesystem is
>> mounted, and that CPUs remember their CDP-enabled state over CPU
>> hotplug.
>>
>> This goes wrong when resctrl's CDP-enabled state changes while all
>> the CPUs in a domain are offline.
>>
>> When a domain comes online, enable (or disable!) CDP to match resctrl's
>> current setting.
>>
>> Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
>> Suggested-by: Reinette Chatre <[email protected]>
>> Signed-off-by: James Morse <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/core.c | 2 ++
>> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
>> 3 files changed, 16 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 89049b343c7a..d8cc5223b7ce 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -578,6 +578,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>> d->id = id;
>> cpumask_set_cpu(cpu, &d->cpu_mask);
>>
>> + rdt_domain_reconfigure_cdp(r);
>> +
>> if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
>> kfree(d);
>> return;
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 181c992f448c..3dd13f3a8b23 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -601,5 +601,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
>> void __check_limbo(struct rdt_domain *d, bool force_free);
>> bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
>> bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
>> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>
>> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 064e9ef44cd6..1c78908ef395 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1859,6 +1859,19 @@ static int set_cache_qos_cfg(int level, bool enable)
>> return 0;
>> }
>>
>> +/* Restore the qos cfg state when a domain comes online */
>> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
>> +{
>> + if (!r->alloc_capable)
>> + return;
>> +
>> + if (r == &rdt_resources_all[RDT_RESOURCE_L2DATA])
>> + l2_qos_cfg_update(&r->alloc_enabled);
>> +
>> + if (r == &rdt_resources_all[RDT_RESOURCE_L3DATA])
>> + l3_qos_cfg_update(&r->alloc_enabled);
>> +}
>> +
>> /*
>> * Enable or disable the MBA software controller
>> * which helps user specify bandwidth in MBps.
>>
>
> As mentioned in my response to v2 the lockdep annotation that formed
> part of this fix is welcome. It is not clear to me if you will be
> submitting again with the annotation added back. Since it is not
> required for this fix I will add my tag here and you could include it if
> you do decide to resubmit.
>
> Thank you
>
> Reviewed-by: Reinette Chatre <[email protected]>
>
> Reinette
>
On Wed, Apr 15, 2020 at 08:57:34AM -0700, Reinette Chatre wrote:
> Hi Thomas and Borislav,
>
> Could you please consider this patch for inclusion as a fix for v5.7?
Do you mean by that that I should add
Cc: <[email protected]>
so that it goes to stable?
The commit in Fixes: is from 4.10-ish times...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Borislav and James,
On 4/17/2020 7:18 AM, Borislav Petkov wrote:
> On Wed, Apr 15, 2020 at 08:57:34AM -0700, Reinette Chatre wrote:
>> Hi Thomas and Borislav,
>>
>> Could you please consider this patch for inclusion as a fix for v5.7?
>
> Do you mean by that that I should add
>
> Cc: <[email protected]>
>
> so that it goes to stable?
>
> The commit in Fixes: is from 4.10-ish times...
>
Borislav: Sorry about that and thank you very much for catching this
omission. Yes, this patch needs to go to stable.
James: would you be able to do the backports to stable? Please note that
support for L2 and L3 CDP was added across Linux versions and the
resctrl files moved around since then so the backport would need some
changes to address this issue in all versions.
Reinette
Hi guys,
On 17/04/2020 15:57, Reinette Chatre wrote:
> On 4/17/2020 7:18 AM, Borislav Petkov wrote:
>> On Wed, Apr 15, 2020 at 08:57:34AM -0700, Reinette Chatre wrote:
>>> Hi Thomas and Borislav,
>>>
>>> Could you please consider this patch for inclusion as a fix for v5.7?
>>
>> Do you mean by that that I should add
>>
>> Cc: <[email protected]>
>>
>> so that it goes to stable?
Is the CC-stable still required? I've seen a 'Fixes' tag on its own cause patches to get
picked up...
>> The commit in Fixes: is from 4.10-ish times...
>>
>
> Borislav: Sorry about that and thank you very much for catching this
> omission. Yes, this patch needs to go to stable.
> James: would you be able to do the backports to stable? Please note that
> support for L2 and L3 CDP was added across Linux versions and the
> resctrl files moved around since then so the backport would need some
> changes to address this issue in all versions.
Yes, certainly!
Thanks,
James
On Fri, Apr 17, 2020 at 04:33:10PM +0100, James Morse wrote:
> Is the CC-stable still required? I've seen a 'Fixes' tag on its own
> cause patches to get picked up...
AFAIK that is the ultimate plan but it does not happen for every patch
ATM. It will though, eventually. IOW, currently, if you want a patch in
stable, make sure to Cc it. :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 9fe0450785abbc04b0ed5d3cf61fcdb8ab656b4b
Gitweb: https://git.kernel.org/tip/9fe0450785abbc04b0ed5d3cf61fcdb8ab656b4b
Author: James Morse <[email protected]>
AuthorDate: Fri, 21 Feb 2020 16:21:05
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 17 Apr 2020 19:35:01 +02:00
x86/resctrl: Preserve CDP enable over CPU hotplug
Resctrl assumes that all CPUs are online when the filesystem is mounted,
and that CPUs remember their CDP-enabled state over CPU hotplug.
This goes wrong when resctrl's CDP-enabled state changes while all the
CPUs in a domain are offline.
When a domain comes online, enable (or disable!) CDP to match resctrl's
current setting.
Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: James Morse <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/resctrl/core.c | 2 ++
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
3 files changed, 16 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 89049b3..d8cc522 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -578,6 +578,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
d->id = id;
cpumask_set_cpu(cpu, &d->cpu_mask);
+ rdt_domain_reconfigure_cdp(r);
+
if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
kfree(d);
return;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 181c992..3dd13f3 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -601,5 +601,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
void __check_limbo(struct rdt_domain *d, bool force_free);
bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
+void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9d4e73a..5a359d9 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1859,6 +1859,19 @@ static int set_cache_qos_cfg(int level, bool enable)
return 0;
}
+/* Restore the qos cfg state when a domain comes online */
+void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
+{
+ if (!r->alloc_capable)
+ return;
+
+ if (r == &rdt_resources_all[RDT_RESOURCE_L2DATA])
+ l2_qos_cfg_update(&r->alloc_enabled);
+
+ if (r == &rdt_resources_all[RDT_RESOURCE_L3DATA])
+ l3_qos_cfg_update(&r->alloc_enabled);
+}
+
/*
* Enable or disable the MBA software controller
* which helps user specify bandwidth in MBps.