lkml.org 
[lkml]   [2022]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain
From
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

\
 
 \ /
  Last update: 2022-03-16 22:51    [W:0.250 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site