Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain | From | James Morse <> | Date | Wed, 30 Mar 2022 17:43:45 +0100 |
| |
Hi Reinette,
On 16/03/2022 21:50, Reinette Chatre wrote: > 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.
...
>> @@ -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.
Ugh. I'm not sure how I managed to miss that. Thanks for debugging it!
That loop was added to reset the array when the filesystem is mounted, as it may hold stale values from a previous mount of the filesystem. Its currently done by reset_all_ctrls(), but that function should really belong to the architecture code.
Because mbm_handle_overflow() always passes a domain from the L3 to update_mba_bw(), I think the cleanest thing to do is move the reset to a helper that always operates on the L3 array. (and leave some breadcrumbs in the comments).
Thanks!
James
-----------------%<----------------- diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 36edae7dbc6a..3b52f079a5b3 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -1913,6 +1913,23 @@ static void mba_sc_domain_destroy(struct rdt_resource *r, d->mbps_val = NULL; }
+static void mba_sc_reset(void) +{ + /* + * mbm_handle_overflow() only passes domains of the L3 resource to + * update_mba_bw(), so mba_sc only supports monitoring on the L3. + */ + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; + u32 num_closid = resctrl_arch_get_num_closid(r); + struct rdt_domain *d; + int i; + + list_for_each_entry(d, &r->domains, list) { + for (i = 0; i < num_closid; i++) + d->mbps_val[i] = MBA_MAX_MBPS; + } +} + /* * Enable or disable the MBA software controller * which helps user specify bandwidth in MBps. @@ -1922,20 +1939,13 @@ 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)) return -EINVAL;
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; - } + mba_sc_reset();
return 0; } -----------------%<-----------------
| |