lkml.org 
[lkml]   [2022]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain
From
Date
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;
}
-----------------%<-----------------
\
 
 \ /
  Last update: 2022-03-30 18:44    [W:1.351 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site