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

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

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