Re: [PATCH v6 30/42] x86/resctrl: Claim get_{mon,ctrl}_domain_from_cpu() helpers for resctrl

[Reinette Chatre]

Hi James,

On 2/7/25 10:18 AM, James Morse wrote:
> get_{mon,ctrl}_domain_from_cpu() are handy helpers that both the arch
> code and resctrl need to use. Rename them to have a resctrl_ prefix
> and move them to a header file.
> 
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> Tested-by: Carl Worth <carl@xxxxxxxxxxxxxxxxxxxxxx> # arm64
> Tested-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
> Changes from v5:
>  * Added the word from to a comment.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 30 ---------------------
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 --
>  arch/x86/kernel/cpu/resctrl/monitor.c  |  2 +-
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
>  include/linux/resctrl.h                | 37 ++++++++++++++++++++++++++
>  5 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index e4b676879227..921c351d57ae 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -348,36 +348,6 @@ static void cat_wrmsr(struct msr_param *m)
>  		wrmsrl(hw_res->msr_base + i, hw_dom->ctrl_val[i]);
>  }
>  
> -struct rdt_ctrl_domain *get_ctrl_domain_from_cpu(int cpu, struct rdt_resource *r)
> -{
> -	struct rdt_ctrl_domain *d;
> -
> -	lockdep_assert_cpus_held();
> -
> -	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> -		/* Find the domain that contains this CPU */
> -		if (cpumask_test_cpu(cpu, &d->hdr.cpu_mask))
> -			return d;
> -	}
> -
> -	return NULL;
> -}
> -
> -struct rdt_mon_domain *get_mon_domain_from_cpu(int cpu, struct rdt_resource *r)
> -{
> -	struct rdt_mon_domain *d;
> -
> -	lockdep_assert_cpus_held();
> -
> -	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> -		/* Find the domain that contains this CPU */
> -		if (cpumask_test_cpu(cpu, &d->hdr.cpu_mask))
> -			return d;
> -	}
> -
> -	return NULL;
> -}
> -
>  u32 resctrl_arch_get_num_closid(struct rdt_resource *r)
>  {
>  	return resctrl_to_arch_res(r)->num_closid;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0d13006e920b..c44c5b496355 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -475,8 +475,6 @@ unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r, struct rdt_ctrl_domain
>  				  unsigned long cbm);
>  enum rdtgrp_mode rdtgroup_mode_by_closid(int closid);
>  int rdtgroup_tasks_assigned(struct rdtgroup *r);
> -struct rdt_ctrl_domain *get_ctrl_domain_from_cpu(int cpu, struct rdt_resource *r);
> -struct rdt_mon_domain *get_mon_domain_from_cpu(int cpu, struct rdt_resource *r);
>  int closids_supported(void);
>  void closid_free(int closid);
>  int alloc_rmid(u32 closid);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index d99a05fc1b44..470cf16f506e 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -773,7 +773,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
>  	if (WARN_ON_ONCE(!pmbm_data))
>  		return;
>  
> -	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
> +	dom_mba = resctrl_get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
>  	if (!dom_mba) {
>  		pr_warn_once("Failure to get domain for MBA update\n");
>  		return;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5fc60c9ce28f..08fec23a38bf 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -4274,7 +4274,7 @@ void resctrl_offline_cpu(unsigned int cpu)
>  	if (!l3->mon_capable)
>  		goto out_unlock;
>  
> -	d = get_mon_domain_from_cpu(cpu, l3);
> +	d = resctrl_get_mon_domain_from_cpu(cpu, l3);
>  	if (d) {
>  		if (resctrl_is_mbm_enabled() && cpu == d->mbm_work_cpu) {
>  			cancel_delayed_work(&d->mbm_over);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 29415d023aab..511dab4ffcdc 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -3,6 +3,7 @@
>  #define _RESCTRL_H
>  
>  #include <linux/cacheinfo.h>
> +#include <linux/cpu.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/pid.h>
> @@ -399,6 +400,42 @@ static inline u32 resctrl_get_config_index(u32 closid,
>  	}
>  }
>  
> +/*
> + * Caller must hold the cpuhp read lock to prevent the struct rdt_domain from

struct rdt_domain has since been split into struct rdt_ctrl_domain and struct rdt_mon_domain.
I assume this comment covers both helpers so perhaps this can be "to prevent the domain
from ..."?

> + * being freed.
> + */
> +static inline struct rdt_ctrl_domain *
> +resctrl_get_ctrl_domain_from_cpu(int cpu, struct rdt_resource *r)
> +{
> +	struct rdt_ctrl_domain *d;
> +
> +	lockdep_assert_cpus_held();
> +
> +	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> +		/* Find the domain that contains this CPU */
> +		if (cpumask_test_cpu(cpu, &d->hdr.cpu_mask))
> +			return d;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline struct rdt_mon_domain *
> +resctrl_get_mon_domain_from_cpu(int cpu, struct rdt_resource *r)
> +{
> +	struct rdt_mon_domain *d;
> +
> +	lockdep_assert_cpus_held();
> +
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		/* Find the domain that contains this CPU */
> +		if (cpumask_test_cpu(cpu, &d->hdr.cpu_mask))
> +			return d;
> +	}
> +
> +	return NULL;
> +}
> +

Similar to previous requests, could you please provide a motivation for
the switch to inline?

>  /*
>   * Update the ctrl_val and apply this config right now.
>   * Must be called on one of the domain's CPUs.

Reinette