Mailing List Archive

[PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit
The rdt_enable_key is switched when resctrl is mounted, and used to
prevent a second mount of the filesystem. It also enables the
architecture's context switch code.

This requires another architecture to have the same set of static-keys,
as resctrl depends on them too.

Make the resctrl_mounted checks explicit: resctrl can keep track of
whether it has been mounted once. This doesn't need to be combined with
whether the arch code is context switching the CLOSID.
Tests against the rdt_mon_enable_key become a test that resctrl is
mounted and that monitoring is enabled.

This will allow the static-key changing to be moved behind resctrl_arch_
calls.

Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/monitor.c | 5 +++--
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++------
3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 7262b355e128..7d5188e8bec3 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -142,6 +142,7 @@ extern bool rdt_alloc_capable;
extern bool rdt_mon_capable;
extern unsigned int rdt_mon_features;
extern struct list_head resctrl_schema_all;
+extern bool resctrl_mounted;

enum rdt_group_type {
RDTCTRL_GROUP = 0,
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index f38cd2f12285..6279f5c98b39 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -834,7 +834,7 @@ void mbm_handle_overflow(struct work_struct *work)

mutex_lock(&rdtgroup_mutex);

- if (!static_branch_likely(&rdt_mon_enable_key))
+ if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
goto out_unlock;

r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
@@ -867,8 +867,9 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms)
unsigned long delay = msecs_to_jiffies(delay_ms);
int cpu;

- if (!static_branch_likely(&rdt_mon_enable_key))
+ if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
return;
+
cpu = cpumask_any_housekeeping(&dom->cpu_mask);
dom->mbm_work_cpu = cpu;
schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2306fbc9a9bb..5176a85f281c 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -42,6 +42,9 @@ LIST_HEAD(rdt_all_groups);
/* list of entries for the schemata file */
LIST_HEAD(resctrl_schema_all);

+/* the filesystem can only be mounted once */
+bool resctrl_mounted;
+
/* Kernel fs node for "info" directory under root */
static struct kernfs_node *kn_info;

@@ -796,7 +799,7 @@ int proc_resctrl_show(struct seq_file *s, struct pid_namespace *ns,
mutex_lock(&rdtgroup_mutex);

/* Return empty if resctrl has not been mounted. */
- if (!static_branch_unlikely(&rdt_enable_key)) {
+ if (!resctrl_mounted) {
seq_puts(s, "res:\nmon:\n");
goto unlock;
}
@@ -2463,7 +2466,7 @@ static int rdt_get_tree(struct fs_context *fc)
/*
* resctrl file system can only be mounted once.
*/
- if (static_branch_unlikely(&rdt_enable_key)) {
+ if (resctrl_mounted) {
ret = -EBUSY;
goto out;
}
@@ -2511,8 +2514,10 @@ static int rdt_get_tree(struct fs_context *fc)
if (rdt_mon_capable)
static_branch_enable_cpuslocked(&rdt_mon_enable_key);

- if (rdt_alloc_capable || rdt_mon_capable)
+ if (rdt_alloc_capable || rdt_mon_capable) {
static_branch_enable_cpuslocked(&rdt_enable_key);
+ resctrl_mounted = true;
+ }

if (is_mbm_enabled()) {
r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
@@ -2783,6 +2788,7 @@ static void rdt_kill_sb(struct super_block *sb)
static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
static_branch_disable_cpuslocked(&rdt_mon_enable_key);
static_branch_disable_cpuslocked(&rdt_enable_key);
+ resctrl_mounted = false;
kernfs_kill_sb(sb);
mutex_unlock(&rdtgroup_mutex);
cpus_read_unlock();
@@ -3610,7 +3616,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
* If resctrl is mounted, remove all the
* per domain monitor data directories.
*/
- if (static_branch_unlikely(&rdt_mon_enable_key))
+ if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
rmdir_mondata_subdir_allrdtgrp(r, d->id);

if (is_mbm_enabled())
@@ -3687,8 +3693,7 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
if (is_llc_occupancy_enabled())
INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);

- /* If resctrl is mounted, add per domain monitor data directories. */
- if (static_branch_unlikely(&rdt_mon_enable_key))
+ if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
mkdir_mondata_subdir_allrdtgrp(r, d);

return 0;
--
2.39.2
Re: [PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit [ In reply to ]
Hi James,

On 3/20/2023 10:26 AM, James Morse wrote:
> The rdt_enable_key is switched when resctrl is mounted, and used to
> prevent a second mount of the filesystem. It also enables the
> architecture's context switch code.
>
> This requires another architecture to have the same set of static-keys,
> as resctrl depends on them too.
>
> Make the resctrl_mounted checks explicit: resctrl can keep track of
> whether it has been mounted once. This doesn't need to be combined with
> whether the arch code is context switching the CLOSID.
> Tests against the rdt_mon_enable_key become a test that resctrl is
> mounted and that monitoring is enabled.

The last sentence above makes the code change hard to follow ...
(see below)

>
> This will allow the static-key changing to be moved behind resctrl_arch_
> calls.
>
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/monitor.c | 5 +++--
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++------
> 3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 7262b355e128..7d5188e8bec3 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -142,6 +142,7 @@ extern bool rdt_alloc_capable;
> extern bool rdt_mon_capable;
> extern unsigned int rdt_mon_features;
> extern struct list_head resctrl_schema_all;
> +extern bool resctrl_mounted;
>
> enum rdt_group_type {
> RDTCTRL_GROUP = 0,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f38cd2f12285..6279f5c98b39 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -834,7 +834,7 @@ void mbm_handle_overflow(struct work_struct *work)
>
> mutex_lock(&rdtgroup_mutex);
>
> - if (!static_branch_likely(&rdt_mon_enable_key))
> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))

... considering the text in the changelog the "resctrl_mounted" check seems
unnecessary. Looking ahead I wonder if this check would not be more
appropriate in patch 15?

> goto out_unlock;
>
> r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> @@ -867,8 +867,9 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms)
> unsigned long delay = msecs_to_jiffies(delay_ms);
> int cpu;
>
> - if (!static_branch_likely(&rdt_mon_enable_key))
> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
> return;

same here

> +

This seems unnecessary.

> cpu = cpumask_any_housekeeping(&dom->cpu_mask);
> dom->mbm_work_cpu = cpu;
> schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 2306fbc9a9bb..5176a85f281c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -42,6 +42,9 @@ LIST_HEAD(rdt_all_groups);
> /* list of entries for the schemata file */
> LIST_HEAD(resctrl_schema_all);
>
> +/* the filesystem can only be mounted once */

Please start sentences with capital letters and end with period.

> +bool resctrl_mounted;
> +
> /* Kernel fs node for "info" directory under root */
> static struct kernfs_node *kn_info;
>
> @@ -796,7 +799,7 @@ int proc_resctrl_show(struct seq_file *s, struct pid_namespace *ns,
> mutex_lock(&rdtgroup_mutex);
>
> /* Return empty if resctrl has not been mounted. */
> - if (!static_branch_unlikely(&rdt_enable_key)) {
> + if (!resctrl_mounted) {
> seq_puts(s, "res:\nmon:\n");
> goto unlock;
> }
> @@ -2463,7 +2466,7 @@ static int rdt_get_tree(struct fs_context *fc)
> /*
> * resctrl file system can only be mounted once.
> */
> - if (static_branch_unlikely(&rdt_enable_key)) {
> + if (resctrl_mounted) {
> ret = -EBUSY;
> goto out;
> }
> @@ -2511,8 +2514,10 @@ static int rdt_get_tree(struct fs_context *fc)
> if (rdt_mon_capable)
> static_branch_enable_cpuslocked(&rdt_mon_enable_key);
>
> - if (rdt_alloc_capable || rdt_mon_capable)
> + if (rdt_alloc_capable || rdt_mon_capable) {
> static_branch_enable_cpuslocked(&rdt_enable_key);
> + resctrl_mounted = true;
> + }
>
> if (is_mbm_enabled()) {
> r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> @@ -2783,6 +2788,7 @@ static void rdt_kill_sb(struct super_block *sb)
> static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
> static_branch_disable_cpuslocked(&rdt_mon_enable_key);
> static_branch_disable_cpuslocked(&rdt_enable_key);
> + resctrl_mounted = false;
> kernfs_kill_sb(sb);
> mutex_unlock(&rdtgroup_mutex);
> cpus_read_unlock();
> @@ -3610,7 +3616,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
> * If resctrl is mounted, remove all the
> * per domain monitor data directories.
> */
> - if (static_branch_unlikely(&rdt_mon_enable_key))
> + if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
> rmdir_mondata_subdir_allrdtgrp(r, d->id);
>
> if (is_mbm_enabled())
> @@ -3687,8 +3693,7 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
> if (is_llc_occupancy_enabled())
> INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
>
> - /* If resctrl is mounted, add per domain monitor data directories. */
> - if (static_branch_unlikely(&rdt_mon_enable_key))
> + if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
> mkdir_mondata_subdir_allrdtgrp(r, d);
>
> return 0;

Above also, the resctrl_mounted check does not seem to be needed.

Reinette
Re: [PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit [ In reply to ]
Hi Reinette,

On 01/04/2023 00:28, Reinette Chatre wrote:
> On 3/20/2023 10:26 AM, James Morse wrote:
>> The rdt_enable_key is switched when resctrl is mounted, and used to
>> prevent a second mount of the filesystem. It also enables the
>> architecture's context switch code.
>>
>> This requires another architecture to have the same set of static-keys,
>> as resctrl depends on them too.
>>
>> Make the resctrl_mounted checks explicit: resctrl can keep track of
>> whether it has been mounted once. This doesn't need to be combined with
>> whether the arch code is context switching the CLOSID.
>> Tests against the rdt_mon_enable_key become a test that resctrl is
>> mounted and that monitoring is enabled.
>
> The last sentence above makes the code change hard to follow ...
> (see below)
>
>>
>> This will allow the static-key changing to be moved behind resctrl_arch_
>> calls.

>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f38cd2f12285..6279f5c98b39 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -834,7 +834,7 @@ void mbm_handle_overflow(struct work_struct *work)
>>
>> mutex_lock(&rdtgroup_mutex);
>>
>> - if (!static_branch_likely(&rdt_mon_enable_key))
>> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
>
> ... considering the text in the changelog the "resctrl_mounted" check seems
> unnecessary. Looking ahead I wonder if this check would not be more
> appropriate in patch 15?

How so?

This is secretly relying on rdt_mon_enable_key being cleared in rdt_kill_sb() when the
filesystem is unmounted, otherwise the overflow thread keeps running once the filesystem
is unmounted.

I thought it simpler to add all these checks explicitly in one go.
That makes it simpler to thin out the static keys as their 'and its mounted' behaviour is
no longer relied on.

I'll add comments for these cases covering why the filesystem-mounted check is needed.


>> goto out_unlock;
>>
>> r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> @@ -867,8 +867,9 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms)
>> unsigned long delay = msecs_to_jiffies(delay_ms);
>> int cpu;
>>
>> - if (!static_branch_likely(&rdt_mon_enable_key))
>> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
>> return;
>
> same here

If a domain comes online mbm_setup_overflow_handler() is called, if the filesystem is not
mounted, there is nothing for it to do. Today this relies on the architecture having a
static key that resctrl can toggle when it gets unmounted.


>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 2306fbc9a9bb..5176a85f281c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c

>> @@ -3687,8 +3693,7 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>> if (is_llc_occupancy_enabled())
>> INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
>>
>> - /* If resctrl is mounted, add per domain monitor data directories. */
>> - if (static_branch_unlikely(&rdt_mon_enable_key))
>> + if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
>> mkdir_mondata_subdir_allrdtgrp(r, d);
>>
>> return 0;
>
> Above also, the resctrl_mounted check does not seem to be needed.

Today its implicit in the rdt_mon_enable_key.

If the filesystem isn't mounted, there is no need to create the directories as no-one can
see them. (it does look like it would be harmless as kernfs_create_root() is called once
at init time).
Instead this work gets done at mount time by rdt_get_tree() calling mkdir_mondata_all().


Thanks,

James
Re: [PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit [ In reply to ]
Hi James,

On 4/27/2023 7:19 AM, James Morse wrote:
> Hi Reinette,
>
> On 01/04/2023 00:28, Reinette Chatre wrote:
>> On 3/20/2023 10:26 AM, James Morse wrote:
>>> The rdt_enable_key is switched when resctrl is mounted, and used to
>>> prevent a second mount of the filesystem. It also enables the
>>> architecture's context switch code.
>>>
>>> This requires another architecture to have the same set of static-keys,
>>> as resctrl depends on them too.
>>>
>>> Make the resctrl_mounted checks explicit: resctrl can keep track of
>>> whether it has been mounted once. This doesn't need to be combined with
>>> whether the arch code is context switching the CLOSID.
>>> Tests against the rdt_mon_enable_key become a test that resctrl is
>>> mounted and that monitoring is enabled.
>>
>> The last sentence above makes the code change hard to follow ...
>> (see below)
>>
>>> This will allow the static-key changing to be moved behind resctrl_arch_
>>> calls.
>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index f38cd2f12285..6279f5c98b39 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -834,7 +834,7 @@ void mbm_handle_overflow(struct work_struct *work)
>>>
>>> mutex_lock(&rdtgroup_mutex);
>>>
>>> - if (!static_branch_likely(&rdt_mon_enable_key))
>>> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
>>
>> ... considering the text in the changelog the "resctrl_mounted" check seems
>> unnecessary. Looking ahead I wonder if this check would not be more
>> appropriate in patch 15?
>
> How so?
>
> This is secretly relying on rdt_mon_enable_key being cleared in rdt_kill_sb() when the
> filesystem is unmounted, otherwise the overflow thread keeps running once the filesystem
> is unmounted.

hmmm ... I do not think my feedback was clear. I understand that this is done
as a prep patch but that was only clear when I read patch 15 because as the
work is presented here it seems unnecessary.

>
> I thought it simpler to add all these checks explicitly in one go.
> That makes it simpler to thin out the static keys as their 'and its mounted' behaviour is
> no longer relied on.

Understood. If you want to keep this as a prep patch, could you please update the
changelog to reflect this? The following sentence in the changelog makes this patch
hard to follow since it essentially claims that what this patch does is unnecessary:
"Tests against the rdt_mon_enable_key become a test that resctrl is mounted
and that monitoring is enabled."

I also do still wonder why these resctrl_mounted checks cannot move to patch
15 when they are needed. Adding them there makes it obvious that rdt_mon_enable_key
had a dual purpose that patch 15 splits into two separate checks.

Reinette
Re: [PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit [ In reply to ]
Hi Reinette,

On 28/04/2023 00:37, Reinette Chatre wrote:
> On 4/27/2023 7:19 AM, James Morse wrote:
>> On 01/04/2023 00:28, Reinette Chatre wrote:
>>> On 3/20/2023 10:26 AM, James Morse wrote:
>>>> The rdt_enable_key is switched when resctrl is mounted, and used to
>>>> prevent a second mount of the filesystem. It also enables the
>>>> architecture's context switch code.
>>>>
>>>> This requires another architecture to have the same set of static-keys,
>>>> as resctrl depends on them too.
>>>>
>>>> Make the resctrl_mounted checks explicit: resctrl can keep track of
>>>> whether it has been mounted once. This doesn't need to be combined with
>>>> whether the arch code is context switching the CLOSID.
>>>> Tests against the rdt_mon_enable_key become a test that resctrl is
>>>> mounted and that monitoring is enabled.
>>>
>>> The last sentence above makes the code change hard to follow ...
>>> (see below)
>>>
>>>> This will allow the static-key changing to be moved behind resctrl_arch_
>>>> calls.
>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> index f38cd2f12285..6279f5c98b39 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> @@ -834,7 +834,7 @@ void mbm_handle_overflow(struct work_struct *work)
>>>>
>>>> mutex_lock(&rdtgroup_mutex);
>>>>
>>>> - if (!static_branch_likely(&rdt_mon_enable_key))
>>>> + if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
>>>
>>> ... considering the text in the changelog the "resctrl_mounted" check seems
>>> unnecessary. Looking ahead I wonder if this check would not be more
>>> appropriate in patch 15?
>>
>> How so?
>>
>> This is secretly relying on rdt_mon_enable_key being cleared in rdt_kill_sb() when the
>> filesystem is unmounted, otherwise the overflow thread keeps running once the filesystem
>> is unmounted.
>
> hmmm ... I do not think my feedback was clear. I understand that this is done
> as a prep patch but that was only clear when I read patch 15 because as the
> work is presented here it seems unnecessary.
>
>>
>> I thought it simpler to add all these checks explicitly in one go.
>> That makes it simpler to thin out the static keys as their 'and its mounted' behaviour is
>> no longer relied on.
>
> Understood. If you want to keep this as a prep patch, could you please update the
> changelog to reflect this? The following sentence in the changelog makes this patch
> hard to follow since it essentially claims that what this patch does is unnecessary:
> "Tests against the rdt_mon_enable_key become a test that resctrl is mounted
> and that monitoring is enabled."

"Because of the implicit mount test" ... the text immediately before this.

We're probably going to keep talking past each other on this - I'll rephrase that
paragraph as:
| rdt_mon_enable_key is never used just to test that resctrl is mounted,
| but does also have this implication. Add a resctrl_mounted to all uses
| of rdt_mon_enable_key. This will allow rdt_mon_enable_key to be swapped
| with a helper in a subsequent patch.


> I also do still wonder why these resctrl_mounted checks cannot move to patch
> 15 when they are needed. Adding them there makes it obvious that rdt_mon_enable_key
> had a dual purpose that patch 15 splits into two separate checks.

That is happening in this patch too, rdt_mon_enable_key becomes
(resctrl_mounted && rdt_mon_enable_key), the implicit property is now explicit, so a later
patch can modify rdt_mon_enable_key without breaking this behaviour.

I think its easier to review if patch 15 is making a set of 1:1 mappings instead of
splitting some static-keys but not others. Let me know what you think of the new version.


Thanks,

James