lkml.org 
[lkml]   [2022]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the rdt_domain
From
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

\
 
 \ /
  Last update: 2022-04-02 00:56    [W:0.094 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site