Re: [PATCH v2 02/24] x86/resctrl: Split struct rdt_domain
Hi James, On 4/8/2021 10:20 AM, James Morse wrote: On 31/03/2021 22:36, Reinette Chatre wrote: On 3/12/2021 9:58 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 the features provided by Intel RDT and AMD PQoS, and moved to /fs/. 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. Similar to previous patch it would help to explain how this split was chosen. For example, why are the CPUs to which a resource is associated not considered a hardware property? Similarly, because the meaning of those CPUs doesn't differ or change between architectures. Got it. This seems to be a repeat of the discussion about patch 1. Please note that the description of rdt_hw_domain in this patch reads "hw attributes of a group of CPUs that share a resource". This can be understood to mean that the struct contains attributes discovered from hardware. I've expanded the middle paragraph in the commit message to explain why the arch specific things are arch specific: | Continue by splitting struct rdt_domain, into an architecture private | 'hw' struct, which contains the common resctrl structure that would be | used by any architecture. | | The hardware values in ctrl_val and mbps_val need to be accessed via | helpers to allow another architecture to convert these into a different | format if necessary. | | After this split, filesystem code code paths touching a 'hw' struct | indicates where an abstraction is needed. and similarly changed the kernel doc comment. Thank you. I assume this includes changing the "hw attributes of a group of CPUs that share a resource" I mention above. Let me know if you prefer some other struct name. I am ok with current naming. Other folks may have better ideas. Thank you Reinette
Re: [PATCH v2 02/24] x86/resctrl: Split struct rdt_domain
Hi Reinette, On 31/03/2021 22:36, Reinette Chatre wrote: > On 3/12/2021 9:58 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 >> the features provided by Intel RDT and AMD PQoS, and moved to /fs/. >> >> 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. > > Similar to previous patch it would help to explain how this split was chosen. > For example, > why are the CPUs to which a resource is associated not considered a hardware > property? Similarly, because the meaning of those CPUs doesn't differ or change between architectures. I've expanded the middle paragraph in the commit message to explain why the arch specific things are arch specific: | Continue by splitting struct rdt_domain, into an architecture private | 'hw' struct, which contains the common resctrl structure that would be | used by any architecture. | | The hardware values in ctrl_val and mbps_val need to be accessed via | helpers to allow another architecture to convert these into a different | format if necessary. | | After this split, filesystem code code paths touching a 'hw' struct | indicates where an abstraction is needed. and similarly changed the kernel doc comment. Let me know if you prefer some other struct name. Thanks, James
Re: [PATCH v2 02/24] x86/resctrl: Split struct rdt_domain
Hi James, On 3/12/2021 9:58 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 the features provided by Intel RDT and AMD PQoS, and moved to /fs/. 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. Similar to previous patch it would help to explain how this split was chosen. For example, why are the CPUs to which a resource is associated not considered a hardware property? Thank you Reinette
[PATCH v2 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
the features provided by Intel RDT and AMD PQoS, and moved 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.
Splitting this structure only moves types around, and should not lead
to any change in behaviour.
Reviewed-by: Jamie Iles
Signed-off-by: James Morse
---
Changes since v1:
* Tabs space and other whitespace
* cpu becomes CPU in all comments touched
---
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 | 32 +-
6 files changed, 91 insertions(+), 60 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c
b/arch/x86/kernel/cpu/resctrl/core.c
index 6cd84d54086f..ca43a7491fda 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)
{
unsigned int i;
+ struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
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)
{
unsigned int i;
+ struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
/* 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)
{
unsigned int i;
+ struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
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, &add_pos);
@@ -601,10 +606,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 = &hw_dom->resctrl;
d->id = id;
cpumask_set_cpu(cpu, &d->cpu_mask);
@@ -633,6 +639,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);
@@ -640,6 +647,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource
*r)
pr_warn("Couldn't find cac
