Re: [PATCH 12/24] x86/resctrl: Add closid to the staged config
Hi Reinette,
On 17/11/2020 23:46, Reinette Chatre wrote:
> On 10/30/2020 9:11 AM, James Morse wrote:
>> Once the L2/L2CODE/L2DATA resources are merged, there may be two
>> configurations staged for one resource when CDP is enabled. The
>> closid should always be passed with the type of configuration to the
>> arch code.
>>
>> Because update_domains() will eventually apply a set of configurations,
>> it should take the closid from the same place, so they pair up.
>>
>> Move the closid to be a staged parameter.
>
> Move implies that it is taken from one location and added to another. This
> seems like a
> copy instead?
I'll rephrase it.
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index 0c95ed83eb05..b107c0202cfb 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -72,6 +72,7 @@ int parse_bw(struct rdt_parse_data *data, struct
>> resctrl_schema *s,
>> if (!bw_validate(data->buf, _val, r))
>> return -EINVAL;
>> cfg->new_ctrl = bw_val;
>> + cfg->closid = data->rdtgrp->closid;
>> cfg->have_new_ctrl = true;
>> return 0;
>> @@ -178,6 +179,7 @@ int parse_cbm(struct rdt_parse_data *data, struct
>> resctrl_schema *s,
>> }
>> cfg->new_ctrl = cbm_val;
>> + cfg->closid = data->rdtgrp->closid;
>> cfg->have_new_ctrl = true;
> rdtgrp is already available so it could just be:
> cfg->closid = rdtgrp->closid?
Yes, this is just trying to be identical to the earlier version.
I'll change it.
>> return 0;
>> @@ -245,15 +247,15 @@ static int parse_line(char *line, struct
>> resctrl_schema *s,
>> }
>> static void apply_config(struct rdt_hw_domain *hw_dom,
>> - struct resctrl_staged_config *cfg, int closid,
>> + struct resctrl_staged_config *cfg,
>> 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]) {
>> + if (cfg->new_ctrl != dc[cfg->closid]) {
>> cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
>> - dc[closid] = cfg->new_ctrl;
>> + dc[cfg->closid] = cfg->new_ctrl;
>> }
>> cfg->have_new_ctrl = false;
>> @@ -284,7 +286,7 @@ int update_domains(struct rdt_resource *r, int closid)
>> if (!cfg->have_new_ctrl)
>> continue;
>> - apply_config(hw_dom, cfg, closid, cpu_mask, mba_sc);
>> + apply_config(hw_dom, cfg, cpu_mask, mba_sc);
>> }
>> }
> It is not clear to me that storing the closid in the staged config is
> necessary. A closid
> is associated with a resource group so when the user writes to the schemata
> file all
> configurations would be (from the resource group perspective) for the same
> closid. This is
> the value provided here to update domains. Looking ahead in this series this
> closid is
> later used to compute the index (get_config_index()) that would use as input
> cfg->closid,
> but that cfg->closid would be identical for all resources/staged configs and
> then the new
> index would be computed based on the resource type. Having the closid in the
> staged config
> thus does not seem to be necessary, it could remain as this function
> parameter and be used
> for all staged configs?
Yes, when emulating CDP with MPAM, these can be two unrelated numbers, but it
looks like I
lost track of the 'its always going to be the same value' for each invocation
through the
filesystem.
An earlier version tried to push separate code/data closid values further into
the resctrl
code, but I thought it was getting too confusing.
The x86 CDP behaviour is either 'configuration is twice the size' or 'an odd
even
pair of closid' depending on how you look at it. For MPAM the equivalent
feature is an
(arbitary!) pair of closid/partid.
Yes its not necessary for resctrl, I'll see how much of this I can unpick!
Thanks,
James
Re: [PATCH 12/24] x86/resctrl: Add closid to the staged config
Hi James,
On 10/30/2020 9:11 AM, James Morse wrote:
Once the L2/L2CODE/L2DATA resources are merged, there may be two
configurations staged for one resource when CDP is enabled. The
closid should always be passed with the type of configuration to the
arch code.
Because update_domains() will eventually apply a set of configurations,
it should take the closid from the same place, so they pair up.
Move the closid to be a staged parameter.
Move implies that it is taken from one location and added to another.
This seems like a copy instead?
Signed-off-by: James Morse
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c| 6 --
include/linux/resctrl.h | 2 ++
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 0c95ed83eb05..b107c0202cfb 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -72,6 +72,7 @@ int parse_bw(struct rdt_parse_data *data, struct
resctrl_schema *s,
if (!bw_validate(data->buf, _val, r))
return -EINVAL;
cfg->new_ctrl = bw_val;
+ cfg->closid = data->rdtgrp->closid;
cfg->have_new_ctrl = true;
return 0;
@@ -178,6 +179,7 @@ int parse_cbm(struct rdt_parse_data *data, struct
resctrl_schema *s,
}
cfg->new_ctrl = cbm_val;
+ cfg->closid = data->rdtgrp->closid;
cfg->have_new_ctrl = true;
rdtgrp is already available so it could just be:
cfg->closid = rdtgrp->closid?
return 0;
@@ -245,15 +247,15 @@ static int parse_line(char *line, struct resctrl_schema
*s,
}
static void apply_config(struct rdt_hw_domain *hw_dom,
-struct resctrl_staged_config *cfg, int closid,
+struct resctrl_staged_config *cfg,
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]) {
+ if (cfg->new_ctrl != dc[cfg->closid]) {
cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
- dc[closid] = cfg->new_ctrl;
+ dc[cfg->closid] = cfg->new_ctrl;
}
cfg->have_new_ctrl = false;
@@ -284,7 +286,7 @@ int update_domains(struct rdt_resource *r, int closid)
if (!cfg->have_new_ctrl)
continue;
- apply_config(hw_dom, cfg, closid, cpu_mask, mba_sc);
+ apply_config(hw_dom, cfg, cpu_mask, mba_sc);
}
}
It is not clear to me that storing the closid in the staged config is
necessary. A closid is associated with a resource group so when the user
writes to the schemata file all configurations would be (from the
resource group perspective) for the same closid. This is the value
provided here to update domains. Looking ahead in this series this
closid is later used to compute the index (get_config_index()) that
would use as input cfg->closid, but that cfg->closid would be identical
for all resources/staged configs and then the new index would be
computed based on the resource type. Having the closid in the staged
config thus does not seem to be necessary, it could remain as this
function parameter and be used for all staged configs?
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f1164bbb66c5..695247c08ba3 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -28,10 +28,12 @@ enum resctrl_conf_type {
/**
* struct resctrl_staged_config - parsed configuration to be applied
+ * @closid:The closid the new configuration applies to
Please be consistent on how descriptions are started with or without
capital letters.
* @new_ctrl: new ctrl value to be loaded
* @have_new_ctrl:did user provide new_ctrl for this domain
*/
struct resctrl_staged_config {
+ u32 closid;
u32 new_ctrl;
boolhave_new_ctrl;
};
Reinette
[PATCH 12/24] x86/resctrl: Add closid to the staged config
Once the L2/L2CODE/L2DATA resources are merged, there may be two
configurations staged for one resource when CDP is enabled. The
closid should always be passed with the type of configuration to the
arch code.
Because update_domains() will eventually apply a set of configurations,
it should take the closid from the same place, so they pair up.
Move the closid to be a staged parameter.
Signed-off-by: James Morse
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c| 6 --
include/linux/resctrl.h | 2 ++
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 0c95ed83eb05..b107c0202cfb 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -72,6 +72,7 @@ int parse_bw(struct rdt_parse_data *data, struct
resctrl_schema *s,
if (!bw_validate(data->buf, _val, r))
return -EINVAL;
cfg->new_ctrl = bw_val;
+ cfg->closid = data->rdtgrp->closid;
cfg->have_new_ctrl = true;
return 0;
@@ -178,6 +179,7 @@ int parse_cbm(struct rdt_parse_data *data, struct
resctrl_schema *s,
}
cfg->new_ctrl = cbm_val;
+ cfg->closid = data->rdtgrp->closid;
cfg->have_new_ctrl = true;
return 0;
@@ -245,15 +247,15 @@ static int parse_line(char *line, struct resctrl_schema
*s,
}
static void apply_config(struct rdt_hw_domain *hw_dom,
-struct resctrl_staged_config *cfg, int closid,
+struct resctrl_staged_config *cfg,
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]) {
+ if (cfg->new_ctrl != dc[cfg->closid]) {
cpumask_set_cpu(cpumask_any(>cpu_mask), cpu_mask);
- dc[closid] = cfg->new_ctrl;
+ dc[cfg->closid] = cfg->new_ctrl;
}
cfg->have_new_ctrl = false;
@@ -284,7 +286,7 @@ int update_domains(struct rdt_resource *r, int closid)
if (!cfg->have_new_ctrl)
continue;
- apply_config(hw_dom, cfg, closid, cpu_mask, mba_sc);
+ apply_config(hw_dom, cfg, cpu_mask, mba_sc);
}
}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c307170ee45f..1092631ac0b3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2806,6 +2806,7 @@ static int __init_one_rdt_domain(struct rdt_domain *d,
struct resctrl_schema *s,
rdt_last_cmd_printf("No space on %s:%d\n", s->name, d->id);
return -ENOSPC;
}
+ cfg->closid = closid;
cfg->have_new_ctrl = true;
return 0;
@@ -2836,7 +2837,7 @@ static int rdtgroup_init_cat(struct resctrl_schema *s,
u32 closid)
}
/* Initialize MBA resource with default values. */
-static void rdtgroup_init_mba(struct rdt_resource *r)
+static void rdtgroup_init_mba(struct rdt_resource *r, u32 closid)
{
struct resctrl_staged_config *cfg;
struct rdt_domain *d;
@@ -2844,6 +2845,7 @@ static void rdtgroup_init_mba(struct rdt_resource *r)
list_for_each_entry(d, >domains, list) {
cfg = >staged_config[0];
cfg->new_ctrl = is_mba_sc(r) ? MBA_MAX_MBPS : r->default_ctrl;
+ cfg->closid = closid;
cfg->have_new_ctrl = true;
}
}
@@ -2860,7 +2862,7 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
list_for_each_entry(s, _all_schema, list) {
r = s->res;
if (r->rid == RDT_RESOURCE_MBA) {
- rdtgroup_init_mba(r);
+ rdtgroup_init_mba(r, rdtgrp->closid);
} else {
ret = rdtgroup_init_cat(s, rdtgrp->closid);
if (ret < 0)
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f1164bbb66c5..695247c08ba3 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -28,10 +28,12 @@ enum resctrl_conf_type {
/**
* struct resctrl_staged_config - parsed configuration to be applied
+ * @closid:The closid the new configuration applies to
* @new_ctrl: new ctrl value to be loaded
* @have_new_ctrl: did user provide new_ctrl for this domain
*/
struct resctrl_staged_config {
+ u32 closid;
u32 new_ctrl;
boolhave_new_ctrl;
};
--
2.28.0
