Messages in this thread Patch in this message | | | Date | Tue, 17 May 2022 09:19:21 -0700 | Subject | Re: [PATCH v4 08/21] x86/resctrl: Switch over to the resctrl mbps_val list | From | Reinette Chatre <> |
| |
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
| |