Messages in this thread | | | Date | Fri, 1 Apr 2022 15:54:51 -0700 | Subject | Re: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain | From | Reinette Chatre <> |
| |
Hi James,
On 3/30/2022 9:43 AM, James Morse wrote: > 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). > >
I think this points to more than a need to reset the correct array on mount/unmount ... or perhaps I am not understanding this correctly?
As the analysis above shows the mbps_val array only exists for rdt_domains associated with RDT_RESOURCE_L3 but yet mbps_val will contain the MB value provided by user space associated with RDT_RESOURCE_MBA.
For example, following what happens when the user writes to the schemata, would this series not attempt to set the user provided MB value in the rdt_domain->mbps_val that belongs to RDT_RESOURCE_MBA ... but that array would not exist for the domain since the resource is not monitor capable, no?
Reinette
| |