Messages in this thread | | | Date | Fri, 31 Mar 2023 16:28:49 -0700 | Subject | Re: [PATCH v3 12/19] x86/resctrl: Make resctrl_mounted checks explicit | From | Reinette Chatre <> |
| |
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
| |