Hi, James and Reinette,
On 2/19/25 20:08, Reinette Chatre wrote:
> 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@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 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?
These two functions are moved from x86 core.c to resctrl.h (same as
restrl_find_domain()).
If motivation is to reduce one file (fs/resctrl/core.c), would it be
better to create fs/resctrl/core.c and host the three functions in the
.c file and remove "inline" in the .h file?
>> /*
>> * Update the ctrl_val and apply this config right now.
>> * Must be called on one of the domain's CPUs.
> Reinette
>
Thanks.
-Fenghua