Re: [PATCH 02/24] x86/resctrl: Split struct rdt_domain
Hi James, On 10/30/2020 9:10 AM, James Morse wrote: resctrl is the defacto Linux ABI for SoC resource partitioning features. To support it on another architecture, it needs to be abstracted from Intel RDT, and moved it to /fs/. Same comments as previous patch regarding "Intel RDT" and "moved it to" Split struct rdt_domain up too. Move everything that that is particular s/that that/that/ to resctrl into a new header file. resctrl code paths touching a 'hw' struct indicates where an abstraction is needed. No change in behaviour, this patch just moves types around. Please remove the "this patch" term. Signed-off-by: James Morse --- arch/x86/kernel/cpu/resctrl/core.c| 32 +++--- arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 -- arch/x86/kernel/cpu/resctrl/internal.h| 40 +-- arch/x86/kernel/cpu/resctrl/monitor.c | 8 +++-- arch/x86/kernel/cpu/resctrl/rdtgroup.c| 29 ++-- include/linux/resctrl.h | 35 +++- 6 files changed, 94 insertions(+), 60 deletions(-) ... diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h index bcae86138cad..f7aab9245259 100644 --- a/arch/x86/kernel/cpu/resctrl/internal.h +++ b/arch/x86/kernel/cpu/resctrl/internal.h @@ -299,44 +299,22 @@ struct mbm_state { }; /** - * struct rdt_domain - group of cpus sharing an RDT resource - * @list: all instances of this resource - * @id:unique id for this instance - * @cpu_mask: which cpus share this resource - * @rmid_busy_llc: - * bitmap of which limbo RMIDs are above threshold - * @mbm_total: saved state for MBM total bandwidth - * @mbm_local: saved state for MBM local bandwidth - * @mbm_over: worker to periodically read MBM h/w counters - * @cqm_limbo: worker to periodically read CQM h/w counters - * @mbm_work_cpu: - * worker cpu for MBM h/w counters - * @cqm_work_cpu: - * worker cpu for CQM h/w counters + * struct rdt_hw_domain - group of cpus sharing an RDT resource s/RDT/resctrl/? Even so, looking ahead, struct rdt_hw_domain and struct rdt_domain are receiving duplicate descriptions that needs to be adjusted. Also, CPU is preferred instead of cpu. I understand that in some cases you copy existing usage and I am actually not sure if this would be insisted upon when going to the next level of review. Since these are comments changing it does seem like a good time to use preferred term. + * @resctrl:Properties exposed to the resctrl file system Please use tabs and spaces consistently. * @ctrl_val: array of cache or mem ctrl values (indexed by CLOSID) * @mbps_val: When mba_sc is enabled, this holds the bandwidth in MBps - * @new_ctrl: new ctrl value to be loaded - * @have_new_ctrl: did user provide new_ctrl for this domain - * @plr: pseudo-locked region (if any) associated with domain */ -struct rdt_domain { - struct list_headlist; - int id; - struct cpumask cpu_mask; - unsigned long *rmid_busy_llc; - struct mbm_state*mbm_total; - struct mbm_state*mbm_local; - struct delayed_work mbm_over; - struct delayed_work cqm_limbo; - int mbm_work_cpu; - int cqm_work_cpu; +struct rdt_hw_domain { + struct rdt_domain resctrl; u32 *ctrl_val; u32 *mbps_val; - u32 new_ctrl; - boolhave_new_ctrl; - struct pseudo_lock_region *plr; }; +static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r) +{ + return container_of(r, struct rdt_hw_domain, resctrl); +} + /** * struct msr_param - set a range of MSRs from a domain * @res: The resource to use ... diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h index b2c2b7386d28..f5af59b8f2a9 100644 --- a/include/linux/resctrl.h +++ b/include/linux/resctrl.h @@ -15,7 +15,40 @@ int proc_resctrl_show(struct seq_file *m, #endif -struct rdt_domain; +/** + * struct rdt_domain - group of cpus sharing an RDT resource Duplicate description here (same as rdt_hw_domain). Please use CPUs instead and resctrl instead of RDT. + * @list: all instances of this resource + * @id:unique id for this instance + * @cpu_mask: which cpus share this resource + * @new_ctrl: new ctrl value to be loaded + * @have_new_ctrl: did user provide new_ctrl for this domain + * @rmid_busy_llc: bitmap of which limbo RMIDs are above threshold + * @mbm_total: saved state for MBM total bandwidth + * @mbm_local: saved state for MBM
[PATCH 02/24] x86/resctrl: Split struct rdt_domain
resctrl is the defacto Linux ABI for SoC resource partitioning features. To support it on another architecture, it needs to be abstracted from Intel RDT, and moved it to /fs/. Split struct rdt_domain up too. Move everything that that is particular to resctrl into a new header file. resctrl code paths touching a 'hw' struct indicates where an abstraction is needed. No change in behaviour, this patch just moves types around. Signed-off-by: James Morse --- arch/x86/kernel/cpu/resctrl/core.c| 32 +++--- arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 -- arch/x86/kernel/cpu/resctrl/internal.h| 40 +-- arch/x86/kernel/cpu/resctrl/monitor.c | 8 +++-- arch/x86/kernel/cpu/resctrl/rdtgroup.c| 29 ++-- include/linux/resctrl.h | 35 +++- 6 files changed, 94 insertions(+), 60 deletions(-) diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c index 470661f2eb68..97040a54cc9a 100644 --- a/arch/x86/kernel/cpu/resctrl/core.c +++ b/arch/x86/kernel/cpu/resctrl/core.c @@ -385,10 +385,11 @@ static void mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r) { struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); + struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); unsigned int i; for (i = m->low; i < m->high; i++) - wrmsrl(hw_res->msr_base + i, d->ctrl_val[i]); + wrmsrl(hw_res->msr_base + i, hw_dom->ctrl_val[i]); } /* @@ -410,21 +411,23 @@ mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r) { struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); + struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); unsigned int i; /* Write the delay values for mba. */ for (i = m->low; i < m->high; i++) - wrmsrl(hw_res->msr_base + i, delay_bw_map(d->ctrl_val[i], r)); + wrmsrl(hw_res->msr_base + i, delay_bw_map(hw_dom->ctrl_val[i], r)); } static void cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r) { struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); + struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); unsigned int i; for (i = m->low; i < m->high; i++) - wrmsrl(hw_res->msr_base + cbm_idx(r, i), d->ctrl_val[i]); + wrmsrl(hw_res->msr_base + cbm_idx(r, i), hw_dom->ctrl_val[i]); } struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r) @@ -510,21 +513,22 @@ void setup_default_ctrlval(struct rdt_resource *r, u32 *dc, u32 *dm) static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d) { struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); + struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); struct msr_param m; u32 *dc, *dm; - dc = kmalloc_array(hw_res->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL); + dc = kmalloc_array(hw_res->num_closid, sizeof(*hw_dom->ctrl_val), GFP_KERNEL); if (!dc) return -ENOMEM; - dm = kmalloc_array(hw_res->num_closid, sizeof(*d->mbps_val), GFP_KERNEL); + dm = kmalloc_array(hw_res->num_closid, sizeof(*hw_dom->mbps_val), GFP_KERNEL); if (!dm) { kfree(dc); return -ENOMEM; } - d->ctrl_val = dc; - d->mbps_val = dm; + hw_dom->ctrl_val = dc; + hw_dom->mbps_val = dm; setup_default_ctrlval(r, dc, dm); m.low = 0; @@ -586,6 +590,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r) { int id = get_cpu_cacheinfo_id(cpu, r->cache_level); struct list_head *add_pos = NULL; + struct rdt_hw_domain *hw_dom; struct rdt_domain *d; d = rdt_find_domain(r, id, _pos); @@ -599,10 +604,11 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r) return; } - d = kzalloc_node(sizeof(*d), GFP_KERNEL, cpu_to_node(cpu)); - if (!d) + hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu)); + if (!hw_dom) return; + d = _dom->resctrl; d->id = id; cpumask_set_cpu(cpu, >cpu_mask); @@ -631,6 +637,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r) static void domain_remove_cpu(int cpu, struct rdt_resource *r) { int id = get_cpu_cacheinfo_id(cpu, r->cache_level); + struct rdt_hw_domain *hw_dom; struct rdt_domain *d; d = rdt_find_domain(r, id, NULL); @@ -638,6 +645,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r) pr_warn("Couldn't find cache id for CPU %d\n", cpu); return; } + hw_dom = resctrl_to_arch_dom(d); cpumask_clear_cpu(cpu, >cpu_mask); if (cpumask_empty(>cpu_mask)) { @@ -670,12 +678,12