Messages in this thread | | | Date | Mon, 8 Apr 2024 20:16:08 -0700 | Subject | Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid() | From | Reinette Chatre <> |
| |
Hi James,
On 3/21/2024 9:50 AM, 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) > { > - 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;
Please maintain reverse fir order.
> + > + 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 { > + u32 closid; > + u32 rmid; > +}; > + > +/* > + * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to
"this CPU's defaults"?
> + * struct resctrl_cpu_sync, or NULL. > + */
Updating the CPU's defaults is not the primary goal of this function and because of that I do not think this should be the focus with the main goal (updating RMID and CLOSID on CPU) ignored. Specifically, this function only updates the defaults if *info is set but it _always_ ensures CPU is running with appropriate CLOSID/RMID (which may or may not be from a CPU default).
I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate and the comment needs to elaborate what the function does.
> +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);
Reinette
| |