Re: [PATCH v4 08/21] x86/resctrl: Switch over to the resctrl mbps_val list

[Reinette Chatre <reinette.chatre@intel.com>]

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