| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 4 Apr 2024 16:16:59 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Peter Newman <peternewman@...gle.com>, Fenghua Yu <fenghua.yu@...el.com>,
James Morse <james.morse@....com>
CC: Stephane Eranian <eranian@...gle.com>, Thomas Gleixner
<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
<bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
"H. Peter Anvin" <hpa@...or.com>, Peter Zijlstra <peterz@...radead.org>, Juri
Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, "Mel Gorman"
<mgorman@...e.de>, Daniel Bristot de Oliveira <bristot@...hat.com>, Valentin
Schneider <vschneid@...hat.com>, Uros Bizjak <ubizjak@...il.com>, Mike
Rapoport <rppt@...nel.org>, "Kirill A. Shutemov"
<kirill.shutemov@...ux.intel.com>, Rick Edgecombe
<rick.p.edgecombe@...el.com>, Xin Li <xin3.li@...el.com>, Babu Moger
<babu.moger@....com>, Shaopeng Tan <tan.shaopeng@...itsu.com>, "Maciej
Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Jens Axboe
<axboe@...nel.dk>, Christian Brauner <brauner@...nel.org>, Oleg Nesterov
<oleg@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, Tycho Andersen
<tandersen@...flix.com>, Nicholas Piggin <npiggin@...il.com>, Beau Belgrave
<beaub@...ux.microsoft.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 5/6] x86/resctrl: Abstract PQR_ASSOC from generic code
Hi Peter,
On 3/25/2024 10:27 AM, Peter Newman wrote:
> While CLOSID and RMID originated in RDT, the concept applies to other
> architectures, as it's standard to write allocation and monitoring IDs
> into per-CPU registers.
>
> - Rename resctrl_pqr_state and pqr_state to be more
> architecturally-neutral.
I think it will be helpful to also introduce the PQR register, otherwise
it is not clear how this rename is motivated by the first paragraph.
>
> - Introduce resctrl_arch_update_cpu() to replace the explicit write to
> MSR_IA32_PQR_ASSOC in __resctrl_sched_in(). In the case of MPAM,
> PARTID[_I,D] and PMG are a simple function of closid, rmid, and an
> internal global.
It is not obvious where this is going ... it sounds as though MPAM will
need three parameters for this function?
>
> - Update terminology containing explicit references to the PQR_ASSOC
> register.
>
> Signed-off-by: Peter Newman <peternewman@...gle.com>
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 11 ++++++++---
> arch/x86/kernel/cpu/resctrl/internal.h | 6 +++---
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 4 ++--
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 +++++++++---------
> include/linux/resctrl.h | 11 +++++++++++
> 5 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index ae5878d748fc..4cc584754f8b 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -37,12 +37,12 @@
> static DEFINE_MUTEX(domain_list_lock);
>
> /*
> - * The cached resctrl_pqr_state is strictly per CPU and can never be
> + * The cached resctrl_cpu_state is strictly per CPU and can never be
> * updated from a remote CPU. Functions which modify the state
> * are called with interrupts disabled and no preemption, which
> * is sufficient for the protection.
> */
> -DEFINE_PER_CPU(struct resctrl_pqr_state, pqr_state);
> +DEFINE_PER_CPU(struct resctrl_cpu_state, resctrl_state);
>
> /*
> * Used to store the max resource name width and max resource data width
> @@ -309,6 +309,11 @@ static void rdt_get_cdp_l2_config(void)
> rdt_get_cdp_config(RDT_RESOURCE_L2);
> }
>
> +void resctrl_arch_update_cpu(u32 ctrl_id, u32 mon_id)
We already started using the names ctrl_hw_id and mon_hw_id. Could this be consistent
with that?
> +{
> + wrmsr(MSR_IA32_PQR_ASSOC, mon_id, ctrl_id);
> +}
> +
> static void
> mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
> {
> @@ -598,7 +603,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>
> static void clear_closid_rmid(int cpu)
> {
> - struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
> + struct resctrl_cpu_state *state = this_cpu_ptr(&resctrl_state);
>
> state->default_group = &rdtgroup_default;
> state->cur_closid = RESCTRL_RESERVED_CLOSID;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0ba0d2428780..e30f42744ac7 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -332,7 +332,7 @@ struct rftype {
> };
>
> /**
> - * struct resctrl_pqr_state - State cache for the PQR MSR
> + * struct resctrl_cpu_state - State cache for allocation/monitoring group IDs
> * @cur_rmid: The cached Resource Monitoring ID
> * @cur_closid: The cached Class Of Service ID
> * @default_group: The user assigned rdtgroup
> @@ -340,13 +340,13 @@ struct rftype {
> * The cache also helps to avoid pointless updates if the value does
> * not change.
> */
> -struct resctrl_pqr_state {
> +struct resctrl_cpu_state {
> u32 cur_rmid;
> u32 cur_closid;
> struct rdtgroup *default_group;
> };
>
> -DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
> +DECLARE_PER_CPU(struct resctrl_cpu_state, resctrl_state);
>
> /**
> * struct mbm_state - status for each MBM counter in each domain
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 884b88e25141..ca1805a566cb 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -480,8 +480,8 @@ static int pseudo_lock_fn(void *_rdtgrp)
> */
> saved_msr = __rdmsr(MSR_MISC_FEATURE_CONTROL);
> __wrmsr(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits, 0x0);
> - closid_p = this_cpu_read(pqr_state.cur_closid);
> - rmid_p = this_cpu_read(pqr_state.cur_rmid);
> + closid_p = this_cpu_read(resctrl_state.cur_closid);
> + rmid_p = this_cpu_read(resctrl_state.cur_rmid);
> mem_r = plr->kmem;
> size = plr->size;
> line_size = plr->line_size;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index badf181c8cbb..bd067f7ed5b6 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -112,7 +112,7 @@ void rdt_staged_configs_clear(void)
> * + We can simply set current's closid to assign a task to a resource
> * group.
> * + Context switch code can avoid extra memory references deciding which
> - * CLOSID to load into the PQR_ASSOC MSR
> + * CLOSID to load into the CPU
> * - We give up some options in configuring resource groups across multi-socket
> * systems.
> * - Our choices on how to configure each resource become progressively more
> @@ -347,7 +347,7 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> */
> void __resctrl_sched_in(struct task_struct *tsk)
> {
> - struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
> + struct resctrl_cpu_state *state = this_cpu_ptr(&resctrl_state);
> u32 closid = state->cur_closid;
> u32 rmid = state->cur_rmid;
> struct rdtgroup *rgrp;
> @@ -401,7 +401,7 @@ void __resctrl_sched_in(struct task_struct *tsk)
> if (closid != state->cur_closid || rmid != state->cur_rmid) {
> state->cur_closid = closid;
> state->cur_rmid = rmid;
> - wrmsr(MSR_IA32_PQR_ASSOC, rmid, closid);
> + resctrl_arch_update_cpu(closid, rmid);
> }
> }
>
> @@ -416,7 +416,7 @@ static void update_cpu_closid_rmid(void *info)
> struct rdtgroup *r = info;
>
> if (r)
> - this_cpu_write(pqr_state.default_group, r);
> + this_cpu_write(resctrl_state.default_group, r);
>
> /*
> * We cannot unconditionally write the MSR because the current
> @@ -635,8 +635,8 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp)
> static void _update_task_closid_rmid(void *task)
> {
> /*
> - * If the task is still current on this CPU, update PQR_ASSOC MSR.
> - * Otherwise, the MSR is updated when the task is scheduled in.
> + * If the task is still current on this CPU, update the current ctrl
> + * group. Otherwise, the CPU is updated when the task is scheduled in.
I think control and/or monitor group could be updated, not just control group?
> */
> if (task == current)
> resctrl_sched_in(task);
> @@ -3005,7 +3005,7 @@ static void rmdir_all_sub(void)
> else
> rdtgroup_remove(rdtgrp);
> }
> - /* Notify online CPUs to update per cpu storage and PQR_ASSOC MSR */
> + /* Update online CPUs to propagate group membership changes. */
> update_closid_rmid(cpu_online_mask, &rdtgroup_default);
>
> kernfs_remove(kn_info);
> @@ -3688,7 +3688,7 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>
> /* Update per cpu rmid of the moved CPUs first */
> for_each_cpu(cpu, &rdtgrp->cpu_mask)
> - per_cpu(pqr_state.default_group, cpu) = prdtgrp;
> + per_cpu(resctrl_state.default_group, cpu) = prdtgrp;
> /*
> * Update the MSR on moved CPUs and CPUs which have moved
> * task running on them.
> @@ -3732,7 +3732,7 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>
> /* Update per cpu closid and rmid of the moved CPUs first */
> for_each_cpu(cpu, &rdtgrp->cpu_mask)
> - per_cpu(pqr_state.default_group, cpu) = &rdtgroup_default;
> + per_cpu(resctrl_state.default_group, cpu) = &rdtgroup_default;
>
> /*
> * Update the MSR on moved CPUs and CPUs which have moved
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index b2af1fbc7aa1..a6b1b13cc769 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -306,6 +306,17 @@ extern unsigned int resctrl_rmid_realloc_limit;
>
> DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
>
> +/*
For valid kerneldoc this needs to be /**
> + * resctrl_arch_update_cpu() - Make control and monitoring group IDs effective
> + * on the current CPU
> + *
> + * @ctrl_id: An identifier for the control group which is to be used on the
> + * current CPU.
Same comment here about naming.
> + * @mon_id: An identifier for the monitoring group which is to be used on
> + * the current CPU.
> + */
> +void resctrl_arch_update_cpu(u32 ctrl_id, u32 mon_id);
> +
> void __resctrl_sched_in(struct task_struct *tsk);
>
> /*
Reinette
Powered by blists - more mailing lists