linux-kernel - Re: [PATCH v2 08/23] x86/resctrl: Create mba_sc configuration in the rdt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 19:30:18 +0100
From:   James Morse <james.morse@....com>
To:     Reinette Chatre <reinette.chatre@...el.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Babu Moger <Babu.Moger@....com>,
        shameerali.kolothum.thodi@...wei.com,
        Jamie Iles <jamie@...iainc.com>,
        D Scott Phillips OS <scott@...amperecomputing.com>,
        lcherian@...vell.com, bobo.shaobowang@...wei.com,
        tan.shaopeng@...itsu.com
Subject: Re: [PATCH v2 08/23] x86/resctrl: Create mba_sc configuration in the
 rdt_domain

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