| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 17 Nov 2020 15:28:57 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: James Morse <james.morse@....com>, x86@...nel.org,
linux-kernel@...r.kernel.org
Cc: Fenghua Yu <fenghua.yu@...el.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
shameerali.kolothum.thodi@...wei.com,
Jamie Iles <jamie@...iainc.com>,
D Scott Phillips OS <scott@...amperecomputing.com>
Subject: 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 <james.morse@....com>
> ---
> 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 = &hw_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(&dom->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(&cpu_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, &r->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(&d->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 = &hw_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, &resctrl_all_schema, list) {
> list_for_each_entry(dom, &s->res->domains, list)
> - dom->have_new_ctrl = false;
> + memset(dom->staged_config, 0, sizeof(dom->staged_config));
> }
>
> while ((tok = strsep(&buf, "\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 this description is no longer appropriate after
the copy.
> + */
> +struct resctrl_staged_config {
> + u32 new_ctrl;
> + bool have_new_ctrl;
> +};
> +
> /**
> * 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
> - * @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 local bandwidth
> @@ -41,15 +49,13 @@ enum resctrl_conf_type {
> * @mbm_work_cpu: worker cpu for MBM h/w counters
> * @cqm_work_cpu: worker cpu for CQM h/w counters
> * @plr: pseudo-locked region (if any) associated with domain
> + * @staged_config: parsed configuration to be applied
> */
> struct rdt_domain {
> struct list_head list;
> int id;
> struct cpumask cpu_mask;
>
> - u32 new_ctrl;
> - bool have_new_ctrl;
> -
> unsigned long *rmid_busy_llc;
> struct mbm_state *mbm_total;
> struct mbm_state *mbm_local;
> @@ -59,6 +65,7 @@ struct rdt_domain {
> int cqm_work_cpu;
>
> struct pseudo_lock_region *plr;
> + struct resctrl_staged_config staged_config[1];
> };
>
> /**
>
Reinette
Powered by blists - more mailing lists