Re: [PATCH v1 07/20] x86/resctrl: Remove architecture copy of mbps_val

[James Morse]

Hi Reinette,

On 01/09/2021 22:26, Reinette Chatre wrote:
> On 7/29/2021 3:35 PM, James Morse wrote:
>> The resctrl arch code provides a second configuration array mbps_val[]
>> for the mba socftware controller.
> 
> "mba socftware" -> "MBA software"

(fixed)

>> Since resctrl switched over to allocating and freeing its own array
>> when needed, nothing uses the arch code version.
>>
>> Remove it.

>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 56b3541617b5..e864dbc6fe3d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c

>> @@ -406,12 +406,9 @@ void setup_default_ctrlval(struct rdt_resource *r, u32 *dc, u32 *dm)
>>        * Initialize the Control MSRs to having no control.
>>        * For Cache Allocation: Set all bits in cbm
>>        * For Memory Allocation: Set b/w requested to 100%
>> -     * and the bandwidth in MBps to U32_MAX
>>        */
>> -    for (i = 0; i < hw_res->num_closid; i++, dc++, dm++) {
>> +    for (i = 0; i < hw_res->num_closid; i++, dc++)
>>           *dc = r->default_ctrl;
>> -        *dm = MBA_MAX_MBPS;
>> -    }
> 
> Since this function used to reset the array to default I was expecting its callers to now
> reset the new array (more below).

dm/mbps_val? Its initialised when its allocated by mba_sc_domain_allocate() when the
filesystem is mounted, or the domain online callback is made.


>>   }
>>     static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 185f9bb992d1..297c20491549 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1906,7 +1906,7 @@ static int set_mba_sc(bool mba_sc)
>>       r->membw.mba_sc = mba_sc;
>>       list_for_each_entry(d, &r->domains, list) {
>>           hw_dom = resctrl_to_arch_dom(d);
>> -        setup_default_ctrlval(r, hw_dom->ctrl_val, hw_dom->mbps_val);
>> +        setup_default_ctrlval(r, hw_dom->ctrl_val);
>>       }
>>   
> 
> I am wondering why new array is not reset instead of original at this call site?

set_mba_sc() doesn't need to. During mount mba_sc is set to true in rdt_enable_ctx(),
which is before the mbps_val[] array is allocated. Once its allocated, its initialised.
While the filesystem is mounted, and domains come and go, the array is re-allocated and
re-initialised. On unmount, the array is freed when the schemata list is destroyed.

I think moving the allocation/free into set_mba_sc() will make this clearer.

> oh ... I
> see it is removed in following patch BUT it mentions that it is ok because
> reset_all_ctrls() does similar reset ... but it does not seem to do so for mbps_val.

That is about the ctrl_val[] array. The reset_all_ctrls() call should reset the hardware
that the arch code looks after. After these changes the mbps_val[] array is a resctrl
filesystem thing that the arch code doesn't need to know anything about.


Thanks,

James