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

[James Morse <james.morse@arm.com>]

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;
 }
-----------------%<-----------------