Hi James,
On 2/7/25 12:17, 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.
>
> Co-developed-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
> Changes since v1:
> * To clarify the meanings of the new helper and struct:
>
> Rename resctrl_arch_sync_cpu_default() to
> resctrl_arch_sync_cpu_closid_rmid();
>
> Rename struct resctrl_cpu_sync to struct resctrl_cpu_defaults;
>
> Flesh out the comment block in <linux/resctrl.h>.
>
> No functional change.
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++----
> include/linux/resctrl.h | 22 ++++++++++++++++++++++
> 2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index f706e5a288b1..62d9a50c7bba 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -355,13 +355,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_closid_rmid(void *info)
> {
> - struct rdtgroup *r = info;
> + struct resctrl_cpu_defaults *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);
> }
>
> /*
> @@ -376,11 +376,20 @@ 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_defaults defaults, *p = NULL;
> +
> + if (r) {
> + defaults.closid = r->closid;
> + defaults.rmid = r->mon.rmid;
> + p = &defaults;
> + }
> +
> + on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_closid_rmid, 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 a939c0cec7fe..da3b344d06d3 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -266,6 +266,28 @@ struct resctrl_schema {
> u32 num_closid;
> };
>
> +struct resctrl_cpu_defaults {
> + u32 closid;
> + u32 rmid;
> +};
> +
Can you please explain why this is part of resctrl.h?
Isn't this part of architecture specific definition?
> +/**
> + * resctrl_arch_sync_cpu_closid_rmid() - Refresh this CPU's CLOSID and RMID.
> + * Call via IPI.
> + * @info: If non-NULL, a pointer to a struct resctrl_cpu_defaults
> + * specifying the new CLOSID and RMID for tasks in the default
> + * resctrl ctrl and mon group when running on this CPU. If NULL,
> + * this CPU is not re-assigned to a different default group.
> + *
> + * Propagates reassignment of CPUs and/or tasks to different resctrl groups
> + * when requested by the resctrl core code.
> + *
> + * This function records the per-cpu defaults specified by @info (if any),
> + * and then reconfigures the CPU's hardware CLOSID and RMID for subsequent
> + * execution based on @current, in the same way as during a task switch.
> + */
> +void resctrl_arch_sync_cpu_closid_rmid(void *info);
> +
> /**
> * resctrl_get_default_ctrl() - Return the default control value for this
> * resource.
--
Thanks
Babu Moger