Re: [PATCH 11/24] x86/resctrl: Group staged configuration into a separate struct
Hi Reinette,
On 17/11/2020 23:28, Reinette Chatre wrote:
> On 10/30/2020 9:11 AM, James Morse wrote:
>> Arm's MPAM may have surprisingly large bitmaps for its cache
>> portions as the architecture allows up to 4K portions. The size
>> exposed via resctrl may not be the same, some scaling may
>> occur.
>>
>> The values written to hardware may be unlike the values received
>> from resctrl, e.g. MBA percentages may be backed by a bitmap,
>> or a maximum value that isn't a percentage.
>>
>> Today resctrl's ctrlval arrays are written to directly by the
> If using a cryptic word like "ctrlval" it would be easier to understand what
> is meant if
> it matches the variable in the code, "ctrl_val".
I thought the non-underscore version was the preferred version, e.g:
setup_default_ctrlval(), domain_setup_ctrlval() and parse_ctrlval. I'll switch
to the
underscore version for things other than functions if you think its clearer.
>> resctrl filesystem code. e.g. apply_config(). This is a problem
>
> This sentence starts with "Today" implying what code does before this change
> but the
> example function, apply_config() is introduced in this patch.
I don't follow the problem here, 'today' refers to what the code does before
the patch is
applied. "Before this patch" would make me unpopular, I'll try 'previously'.
>> if scaling or conversion is needed by the architecture.
>>
>> The arch code should own the ctrlval array (to allow scaling and
>> conversion), and should only need a single copy of the array for the
>> values currently applied in hardware.
> ok, but that is the case, no?
Its true for x86. But whether its true for MPAM depends on which side of the
divide this
thing lands as the value from user-space may be different to what gets written
to hardware.
If the filesystem code owned the list of values, there would need to be two
copies to
allow MPAM to restore the values in hardware when CPUs come online.
(in particular, the MBA percentage control can be emulated with MPAMs bitmap or
fractional
min/max, the driver has to choose at startup).
I'll try and bundle this as a clearer explanation into the commit message.
>> Move the new_ctrl bitmap value and flag into a struct for staged
>> configuration changes. This is created as an array to allow one per type
>
> This is a bit cryptic as the reader may not know while reading this commit
> message what
> "new_ctrl" is or where it is currently hosted.
Sure, I'll add more explanation of the current code.
>> of configuration. Today there is only one element in the array, but
>> eventually resctrl will use the array slots for CODE/DATA/BOTH to detect
>> a duplicate schema being written.
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index 28d69c78c29e..0c95ed83eb05 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>
> ...
>
>> @@ -240,15 +244,30 @@ static int parse_line(char *line, struct
>> resctrl_schema *s,
>> return -EINVAL;
>> }
>> +static void apply_config(struct rdt_hw_domain *hw_dom,
>> + struct resctrl_staged_config *cfg, int closid,
>> + cpumask_var_t cpu_mask, bool mba_sc)
>> +{
>> + struct rdt_domain *dom = _dom->resctrl;
>> + u32 *dc = mba_sc ? hw_dom->mbps_val : hw_dom->ctrl_val;
>> +
>> + if (cfg->new_ctrl != dc[closid]) {
>> + cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
>> + dc[closid] = cfg->new_ctrl;
>> + }
>> +
>> + cfg->have_new_ctrl = false;
>
> Why is this necessary?
(hmm, its ended up in the wrong patch, but:) This was to ensure that once the
resources
are merged, all the work for applying configuration changes is done by the
first IPI,
ensuring if update_domains() is called for a second schema with the same
resource, it
finds no new work to do.
But without this, the empty_bitmap check would still catch it. I'll remove it.
>> +}
>> +
>> int update_domains(struct rdt_resource *r, int closid)
>> {
>> + struct resctrl_staged_config *cfg;
>> struct rdt_hw_domain *hw_dom;
>> struct msr_param msr_param;
>> cpumask_var_t cpu_mask;
>> struct rdt_domain *d;
>> bool mba_sc;
>> - u32 *dc;
>> - int cpu;
>> + int cpu, i;
>> if (!zalloc_cpumask_var(_mask, GFP_KERNEL))
>> return -ENOMEM;
>> @@ -260,10 +279,12 @@ int update_domains(struct rdt_resource *r, int closid)
>> mba_sc = is_mba_sc(r);
>> list_for_each_entry(d, >domains, list) {
>> hw_dom = resctrl_to_arch_dom(d);
>> - dc = !mba_sc ? hw_dom->ctrl_val : hw_dom->mbps_val;
>> - if (d->have_new_ctrl && d->new_ctrl != dc[closid]) {
>> - cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
>> - dc[closid] = d->new_ctrl;
>> + for (i = 0; i < ARRAY_SIZE(d->staged_config); i++) {
> I understand it may make later patches easier but it seems too early to
> introduce
Re: [PATCH 11/24] x86/resctrl: Group staged configuration into a separate struct
Hi James,
On 10/30/2020 9:11 AM, James Morse wrote:
Arm's MPAM may have surprisingly large bitmaps for its cache
portions as the architecture allows up to 4K portions. The size
exposed via resctrl may not be the same, some scaling may
occur.
The values written to hardware may be unlike the values received
from resctrl, e.g. MBA percentages may be backed by a bitmap,
or a maximum value that isn't a percentage.
Today resctrl's ctrlval arrays are written to directly by the
If using a cryptic word like "ctrlval" it would be easier to understand
what is meant if it matches the variable in the code, "ctrl_val".
resctrl filesystem code. e.g. apply_config(). This is a problem
This sentence starts with "Today" implying what code does before this
change but the example function, apply_config() is introduced in this patch.
if scaling or conversion is needed by the architecture.
The arch code should own the ctrlval array (to allow scaling and
conversion), and should only need a single copy of the array for the
values currently applied in hardware.
ok, but that is the case, no?
Move the new_ctrl bitmap value and flag into a struct for staged
configuration changes. This is created as an array to allow one per type
This is a bit cryptic as the reader may not know while reading this
commit message what "new_ctrl" is or where it is currently hosted.
of configuration. Today there is only one element in the array, but
eventually resctrl will use the array slots for CODE/DATA/BOTH to detect
a duplicate schema being written.
Signed-off-by: James Morse
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 49 ---
arch/x86/kernel/cpu/resctrl/rdtgroup.c| 22 +-
include/linux/resctrl.h | 17 +---
3 files changed, 60 insertions(+), 28 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 28d69c78c29e..0c95ed83eb05 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
...
@@ -240,15 +244,30 @@ static int parse_line(char *line, struct resctrl_schema
*s,
return -EINVAL;
}
+static void apply_config(struct rdt_hw_domain *hw_dom,
+struct resctrl_staged_config *cfg, int closid,
+cpumask_var_t cpu_mask, bool mba_sc)
+{
+ struct rdt_domain *dom = _dom->resctrl;
+ u32 *dc = mba_sc ? hw_dom->mbps_val : hw_dom->ctrl_val;
+
+ if (cfg->new_ctrl != dc[closid]) {
+ cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
+ dc[closid] = cfg->new_ctrl;
+ }
+
+ cfg->have_new_ctrl = false;
Why is this necessary?
+}
+
int update_domains(struct rdt_resource *r, int closid)
{
+ struct resctrl_staged_config *cfg;
struct rdt_hw_domain *hw_dom;
struct msr_param msr_param;
cpumask_var_t cpu_mask;
struct rdt_domain *d;
bool mba_sc;
- u32 *dc;
- int cpu;
+ int cpu, i;
if (!zalloc_cpumask_var(_mask, GFP_KERNEL))
return -ENOMEM;
@@ -260,10 +279,12 @@ int update_domains(struct rdt_resource *r, int closid)
mba_sc = is_mba_sc(r);
list_for_each_entry(d, >domains, list) {
hw_dom = resctrl_to_arch_dom(d);
- dc = !mba_sc ? hw_dom->ctrl_val : hw_dom->mbps_val;
- if (d->have_new_ctrl && d->new_ctrl != dc[closid]) {
- cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
- dc[closid] = d->new_ctrl;
+ for (i = 0; i < ARRAY_SIZE(d->staged_config); i++) {
I understand it may make later patches easier but it seems too early to
introduce this loop because apply_config() does not seem to be ready for
it yet (it would just keep overwriting a closid's config).
+ cfg = _dom->resctrl.staged_config[i];
+ if (!cfg->have_new_ctrl)
+ continue;
+
+ apply_config(hw_dom, cfg, closid, cpu_mask, mba_sc);
}
}
@@ -338,7 +359,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
list_for_each_entry(s, _all_schema, list) {
list_for_each_entry(dom, >res->domains, list)
- dom->have_new_ctrl = false;
+ memset(dom->staged_config, 0,
sizeof(dom->staged_config));
}
while ((tok = strsep(, "\n")) != NULL) {
...
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 9f71f0238239..f1164bbb66c5 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -26,13 +26,21 @@ enum resctrl_conf_type {
CDP_DATA,
};
+/**
+ * struct resctrl_staged_config - parsed configuration to be applied
+ * @new_ctrl: new ctrl value to be loaded
+ * @have_new_ctrl: did user provide new_ctrl for this domain
The "for this domain" in
[PATCH 11/24] x86/resctrl: Group staged configuration into a separate struct
Arm's MPAM may have surprisingly large bitmaps for its cache
portions as the architecture allows up to 4K portions. The size
exposed via resctrl may not be the same, some scaling may
occur.
The values written to hardware may be unlike the values received
from resctrl, e.g. MBA percentages may be backed by a bitmap,
or a maximum value that isn't a percentage.
Today resctrl's ctrlval arrays are written to directly by the
resctrl filesystem code. e.g. apply_config(). This is a problem
if scaling or conversion is needed by the architecture.
The arch code should own the ctrlval array (to allow scaling and
conversion), and should only need a single copy of the array for the
values currently applied in hardware.
Move the new_ctrl bitmap value and flag into a struct for staged
configuration changes. This is created as an array to allow one per type
of configuration. Today there is only one element in the array, but
eventually resctrl will use the array slots for CODE/DATA/BOTH to detect
a duplicate schema being written.
Signed-off-by: James Morse
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 49 ---
arch/x86/kernel/cpu/resctrl/rdtgroup.c| 22 +-
include/linux/resctrl.h | 17 +---
3 files changed, 60 insertions(+), 28 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 28d69c78c29e..0c95ed83eb05 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -60,18 +60,19 @@ static bool bw_validate(char *buf, unsigned long *data,
struct rdt_resource *r)
int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
struct rdt_domain *d)
{
+ struct resctrl_staged_config *cfg = >staged_config[0];
struct rdt_resource *r = s->res;
unsigned long bw_val;
- if (d->have_new_ctrl) {
+ if (cfg->have_new_ctrl) {
rdt_last_cmd_printf("Duplicate domain %d\n", d->id);
return -EINVAL;
}
if (!bw_validate(data->buf, _val, r))
return -EINVAL;
- d->new_ctrl = bw_val;
- d->have_new_ctrl = true;
+ cfg->new_ctrl = bw_val;
+ cfg->have_new_ctrl = true;
return 0;
}
@@ -129,11 +130,12 @@ static bool cbm_validate(char *buf, u32 *data, struct
rdt_resource *r)
int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
struct rdt_domain *d)
{
+ struct resctrl_staged_config *cfg = >staged_config[0];
struct rdtgroup *rdtgrp = data->rdtgrp;
struct rdt_resource *r = s->res;
u32 cbm_val;
- if (d->have_new_ctrl) {
+ if (cfg->have_new_ctrl) {
rdt_last_cmd_printf("Duplicate domain %d\n", d->id);
return -EINVAL;
}
@@ -175,8 +177,8 @@ int parse_cbm(struct rdt_parse_data *data, struct
resctrl_schema *s,
}
}
- d->new_ctrl = cbm_val;
- d->have_new_ctrl = true;
+ cfg->new_ctrl = cbm_val;
+ cfg->have_new_ctrl = true;
return 0;
}
@@ -190,6 +192,7 @@ int parse_cbm(struct rdt_parse_data *data, struct
resctrl_schema *s,
static int parse_line(char *line, struct resctrl_schema *s,
struct rdtgroup *rdtgrp)
{
+ struct resctrl_staged_config *cfg;
struct rdt_resource *r = s->res;
struct rdt_parse_data data;
char *dom = NULL, *id;
@@ -220,6 +223,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
if (r->parse_ctrlval(, s, d))
return -EINVAL;
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
+ cfg = >staged_config[0];
/*
* In pseudo-locking setup mode and just
* parsed a valid CBM that should be
@@ -230,7 +234,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
*/
rdtgrp->plr->s = s;
rdtgrp->plr->d = d;
- rdtgrp->plr->cbm = d->new_ctrl;
+ rdtgrp->plr->cbm = cfg->new_ctrl;
d->plr = rdtgrp->plr;
return 0;
}
@@ -240,15 +244,30 @@ static int parse_line(char *line, struct resctrl_schema
*s,
return -EINVAL;
}
+static void apply_config(struct rdt_hw_domain *hw_dom,
+struct resctrl_staged_config *cfg, int closid,
+cpumask_var_t cpu_mask, bool mba_sc)
+{
+ struct rdt_domain *dom = _dom->resctrl;
+ u32 *dc = mba_sc ? hw_dom->mbps_val : hw_dom->ctrl_val;
+
+ if (cfg->new_ctrl != dc[closid]) {
+ cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
+ dc[closid] =
