Re: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain

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

Hi James,

I tried out this work and encountered a null pointer de-reference that
seems related to this patch. After digging into that it is not
clear to me how this is expected to work.

I encounter the issue just by attempting to mount with "-o mba_MBps" which is
the way to enable the mba_sc and exactly what this patch aims to address.

More below ...

On 2/17/2022 10:20 AM, James Morse wrote:
> To support resctrl's MBA software controller, the architecture must provide
> a second configuration array to hold the mbps_val[] from user-space.
> 
> This complicates the interface between the architecture specific code and
> the filesystem portions of resctrl that will move to /fs/, to allow
> multiple architectures to support resctrl.
> 
> Make the filesystem parts of resctrl create an array for the mba_sc
> values when is_mba_sc() is set to true. The software controller
> can be changed to use this, allowing the architecture code to only
> consider the values configured in hardware.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---

...

>  
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 794a84ba9097..e4313f907eb6 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1889,6 +1889,30 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
>  		l3_qos_cfg_update(&hw_res->cdp_enabled);
>  }
>  
> +static int mba_sc_domain_allocate(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	u32 num_closid = resctrl_arch_get_num_closid(r);
> +	int cpu = cpumask_any(&d->cpu_mask);
> +	int i;
> +
> +	d->mbps_val = kcalloc_node(num_closid, sizeof(*d->mbps_val),
> +				   GFP_KERNEL, cpu_to_node(cpu));
> +	if (!d->mbps_val)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_closid; i++)
> +		d->mbps_val[i] = MBA_MAX_MBPS;
> +
> +	return 0;
> +}
> +

...

> @@ -1905,6 +1932,11 @@ 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;
>  }
>  

...

> @@ -3309,6 +3344,12 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  	if (err)
>  		return err;
>  
> +	err = mba_sc_domain_allocate(r, d);
> +	if (err) {
> +		domain_destroy_mon_state(d);
> +		return err;
> +	}
> +

Before the above snippet there is a check if the resource is capable of monitoring:

resctrl_online_domain()
{
	...
	if (!r->mon_capable)
		return 0;

	...
	err = mba_sc_domain_allocate(r, d);
	...
}

Thus, the rdt_domain->mbps_val array will only exist in those resources that
support monitoring.
	
Taking a look at where mon_capable is set we see it is done in 
get_rdt_mon_resources() and as you can see it is only done for RDT_RESOURCE_L3.

get_rdt_mon_resources()
{
	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;

	...

	return !rdt_get_mon_l3_config(r); /* mon_capable is set within */
}

Based on the above the rdt_domain->mbps_val array can only exist for those
domains that belong to resource RDT_RESOURCE_L3 (if it is capable of monitoring).

Now, looking at set_mba_sc() changed here, it only interacts with RDT_RESOURCE_MBA:

set_mba_sc() 
{
	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;

	...

	list_for_each_entry(d, &r->domains, list) {
		for (i = 0; i < num_closid; i++)
			d->mbps_val[i] = MBA_MAX_MBPS;
	}
}
	
Considering that no domain belonging to RDT_RESOURCE_MBA will have this array this
always ends up being a null pointer de-reference.

Reinette