lkml.org 
[lkml]   [2021]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 08/23] x86/resctrl: Create mba_sc configuration in the rdt_domain
From
Date
Hi Reinette,

On 15/10/2021 23:26, Reinette Chatre wrote:
> On 10/1/2021 9:02 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 code.
>
> This complicates the interface between the architecture code and ... ?

and the filesystem parts ... fixed.


>> 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.

> This changes significantly more than just where the mbps_val array is hosted. It also
> changes how the life cycle of this array is managed. Previously it followed the domain,
> whether mba_sc was enabled or not. Now that it depends on mba_sc it is managed quite
> differently.

> Could the changelog be upfront about this change and its motivation? Stating this would
> make this much easier to review and also the later patches where the original mbps_val
> initialization code is removed without replacement.

Yes, I'd not considered those as different things. I'll split this into two patches, and
move the change that only allocates the memory if its going to be used to the end of the
series.


>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 38670bb810cb..9d402bc8bdff 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -3291,6 +3355,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct
>> rdt_domain *d)
>>           }
>>       }
>>   +    if (is_mba_sc(r))
>> +        return mba_sc_domain_allocate(r, d);
>> +
>>       return 0;
>>   }
>>  

> Could this be done symmetrically? That is, allocate in resctrl_online_domain() and free in
> resctrl_offline_domain().

Yes, that would be better!


>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 5d283bdd6162..355660d46612 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -15,6 +15,9 @@ int proc_resctrl_show(struct seq_file *m,
>>     #endif
>>   +/* max value for struct resctrl_mba_sc's mbps_val */
>> +#define MBA_MAX_MBPS   U32_MAX
>
> struct resctrl_mba_sc?

Was squashed out of the previous version as it only had one member.
This should be struct rdt_domain.


>>   /**
>>    * enum resctrl_conf_type - The type of configuration.
>>    * @CDP_NONE:    No prioritisation, both code and data are controlled or monitored.
>> @@ -53,6 +56,8 @@ struct resctrl_staged_config {
>>    * @cqm_work_cpu:    worker CPU for CQM h/w counters
>>    * @plr:        pseudo-locked region (if any) associated with domain
>>    * @staged_config:    parsed configuration to be applied
>> + * @mbps_val:        Array of user specified control values for mba_sc,
>> + *            indexed by closid

> Could this inherit some of the useful kerneldoc associated with the mbps_val being
> replaced? That is, it exists when mba_sc is enabled and contains bandwidth values in MBps.

Yup, done.


Thanks,

James

\
 
 \ /
  Last update: 2021-10-22 20:31    [W:0.039 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site