| 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
| ||
|
Message-ID: <93459560-0740-57f3-ed15-caae370aa80a@intel.com>
Date: Tue, 17 May 2022 09:19:21 -0700
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>,
H Peter Anvin <hpa@...or.com>,
Babu Moger <Babu.Moger@....com>,
<shameerali.kolothum.thodi@...wei.com>,
D Scott Phillips OS <scott@...amperecomputing.com>,
<lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
<tan.shaopeng@...itsu.com>, Jamie Iles <quic_jiles@...cinc.com>,
Cristian Marussi <cristian.marussi@....com>,
"Xin Hao" <xhao@...ux.alibaba.com>, <xingxin.hx@...nanolis.org>,
<baolin.wang@...ux.alibaba.com>
Subject: Re: [PATCH v4 08/21] x86/resctrl: Switch over to the resctrl mbps_val
list
Hi James,
On 4/12/2022 5:44 AM, James Morse wrote:
> Updates to resctrl's software controller follow the same path as
> other configuration updates, but they don't modify the hardware state.
> rdtgroup_schemata_write() uses parse_line() and the resource's
> parse_ctrlval() function to stage the configuration.
> resctrl_arch_update_domains() then updates the mbps_val[] array
> instead, and resctrl_arch_update_domains() skips the rdt_ctrl_update()
> call that would update hardware.
>
> This complicates the interface between resctrl's filesystem parts
> and architecture specific code. It should be possible for mba_sc
> to be completely implemented by the filesystem parts of resctrl. This
> would allow it to work on a second architecture with no additional code.
> resctrl_arch_update_domains() using the mbps_val[] array prevents this.
>
> Change parse_bw() to write the configuration value directly to the
> mbps_val[] array in the domain structure. Change rdtgroup_schemata_write()
> to skip the call to resctrl_arch_update_domains(), meaning all the
> mba_sc specific code in resctrl_arch_update_domains() can be removed.
> On the read-side, show_doms() and update_mba_bw() are changed to read
> the mbps_val[] array from the domain structure. With this,
> resctrl_arch_get_config() no longer needs to consider mba_sc resources.
>
This sounds like a good cleanup and I understand it to not intend
functional change, so a bit more information is needed on the change
in rdtgroup_init_alloc(). More below.
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 497cadf3285d..5cc1e6b229d4 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -447,13 +447,11 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
> hw_dom_mba = resctrl_to_arch_dom(dom_mba);
>
> cur_bw = pmbm_data->prev_bw;
> - user_bw = resctrl_arch_get_config(r_mba, dom_mba, closid, CDP_NONE);
> + user_bw = dom_mba->mbps_val[closid];
> delta_bw = pmbm_data->delta_bw;
> - /*
> - * resctrl_arch_get_config() chooses the mbps/ctrl value to return
> - * based on is_mba_sc(). For now, reach into the hw_dom.
> - */
> - cur_msr_val = hw_dom_mba->ctrl_val[closid];
> +
> + /* MBA monitor resource doesn't support CDP */
MBA resource does not support monitoring. Perhaps instead:
/* MBA resource doesn't support CDP. */
> + cur_msr_val = resctrl_arch_get_config(r_mba, dom_mba, closid, CDP_NONE);
>
> /*
> * For Ctrl groups read data from child monitor groups.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9d5be6a73644..07904308245c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1356,11 +1356,13 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
> struct seq_file *s, void *v)
> {
> struct resctrl_schema *schema;
> + enum resctrl_conf_type type;
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> struct rdt_domain *d;
> unsigned int size;
> int ret = 0;
> + u32 closid;
> bool sep;
> u32 ctrl;
>
> @@ -1386,8 +1388,11 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
> goto out;
> }
>
> + closid = rdtgrp->closid;
> +
> list_for_each_entry(schema, &resctrl_schema_all, list) {
> r = schema->res;
> + type = schema->conf_type;
> sep = false;
> seq_printf(s, "%*s:", max_name_width, schema->name);
> list_for_each_entry(d, &r->domains, list) {
> @@ -1396,9 +1401,12 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> size = 0;
> } else {
> - ctrl = resctrl_arch_get_config(r, d,
> - rdtgrp->closid,
> - schema->conf_type);
> + if (is_mba_sc(r))
> + ctrl = d->mbps_val[closid];
> + else
> + ctrl = resctrl_arch_get_config(r, d,
> + closid,
> + type);
> if (r->rid == RDT_RESOURCE_MBA)
> size = ctrl;
> else
> @@ -1922,9 +1930,6 @@ static void mba_sc_domain_destroy(struct rdt_resource *r,
> static int set_mba_sc(bool mba_sc)
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> - u32 num_closid = resctrl_arch_get_num_closid(r);
> - struct rdt_domain *d;
> - int i;
>
> if (!is_mbm_enabled() || !is_mba_linear() ||
> mba_sc == is_mba_sc(r))
> @@ -1932,11 +1937,6 @@ static int set_mba_sc(bool mba_sc)
>
> r->membw.mba_sc = mba_sc;
>
> - list_for_each_entry(d, &r->domains, list) {
> - for (i = 0; i < num_closid; i++)
> - d->mbps_val[i] = MBA_MAX_MBPS;
> - }
> -
> return 0;
> }
With this removed, where is rdt_domain->mbps_val reset on remount of resctrl?
>
> @@ -2809,15 +2809,18 @@ 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;
>
> list_for_each_entry(d, &r->domains, list) {
> cfg = &d->staged_config[CDP_NONE];
> - cfg->new_ctrl = is_mba_sc(r) ? MBA_MAX_MBPS : r->default_ctrl;
> + cfg->new_ctrl = r->default_ctrl;
> cfg->have_new_ctrl = true;
> +
> + if (is_mba_sc(r))
> + d->mbps_val[closid] = MBA_MAX_MBPS;
> }
> }
>
> @@ -2831,7 +2834,7 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
> list_for_each_entry(s, &resctrl_schema_all, 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)
What follows this hunk and continues to be called is:
ret = resctrl_arch_update_domains(r, rdtgrp->closid);
before this patch resctrl_arch_update_domains() would just have updated
the mbps_val but not made any configuration changed if is_mba_sc() is true.
Before this patch configuration changes done in
resctrl_arch_update_domains() is omitted when is_mba_sc() is true
but after earlier change in this patch it proceeds and will result in
configuration change.
Reinette
Powered by blists - more mailing lists