Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid()

Moger, Babu
2024-04-15T20:40:56Z

Hi James/Dave,

On 3/21/24 11:50, James Morse wrote:
> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> it uses to update the local CPUs default pqr values. This is a problem
> once the resctrl parts move out to /fs/, as the arch code cannot
> poke around inside struct rdtgroup.
>
> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> to be used as the target of an IPI, and pass the effective CLOSID
> and RMID in a new struct.
>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
> include/linux/resctrl.h | 11 +++++++++++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5d2c1ce5b6b1..18f097fce51e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> * from update_closid_rmid() is protected against __switch_to() because
> * preemption is disabled.
> */
> -static void update_cpu_closid_rmid(void *info)
> +void resctrl_arch_sync_cpu_defaults(void *info)

How about keeping the name similar to the old name?

resctrl_arch_update_cpu_defaults

> {
> - struct rdtgroup *r = info;
> + struct resctrl_cpu_sync *r = info;
>
> if (r) {
> this_cpu_write(pqr_state.default_closid, r->closid);
> - this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> + this_cpu_write(pqr_state.default_rmid, r->rmid);
> }
>
> /*
> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
> * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
> *
> * Per task closids/rmids must have been set up before calling this function.
> + * @r may be NULL.
> */
> static void
> update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
> {
> - on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
> + struct resctrl_cpu_sync defaults;
> + struct resctrl_cpu_sync *defaults_p = NULL;
> +
> + if (r) {
> + defaults.closid = r->closid;
> + defaults.rmid = r->mon.rmid;
> + defaults_p = &defaults;
> + }
> +
> + on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_defaults, defaults_p,
> + 1);
> }
>
> static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 6e87bc95f5ea..2b79e4159507 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -220,6 +220,17 @@ struct resctrl_schema {
> u32 num_closid;
> };
>
> +struct resctrl_cpu_sync {

How about changing it to resctrl_cpu_defaults?

> + u32 closid;
> + u32 rmid;
> +};
> +
> +/*
> + * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to
> + * struct resctrl_cpu_sync, or NULL.
> + */
> +void resctrl_arch_sync_cpu_defaults(void *info);
> +
> /* The number of closid supported by this resource regardless of CDP */
> u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
> int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);

--
Thanks
Babu Moger