linux-kernel - Re: [PATCH v5 07/40] x86/resctrl: Add max_bw to struct resctrl

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1b01d18f-c398-4706-9da6-e920512426a1@arm.com>
Date: Fri, 20 Dec 2024 18:10:47 +0000
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,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v5 07/40] x86/resctrl: Add max_bw to struct resctrl_membw

Hi Reinette,

On 23/10/2024 22:14, Reinette Chatre wrote:
> On 10/4/24 11:03 AM, James Morse wrote:
>> __rdt_get_mem_config_amd() and __get_mem_config_intel() both use
>> the default_ctrl property as a maximum value. This is because the
>> MBA schema works differently between these platforms. Doing this

> The schema works differently but they can still use the same property
> as maximum, is that a problem?

I think its a problem for user-space - but its an existing problem.
Today resctrl uses the default as the maximum. This makes that property explicit.


>> complicates determining whether the default_ctrl property belongs
>> to the arch code, or can be derived from the schema format.
> 
> So instead of Intel and AMD both using default_ctrl as a maximum this patch
> introduces a new max_bw with both using that as maximum instead.
> Unclear how this change fixes the unclear complication.

Is the default value something that can be determine from the schema format?

Previously, no - because the default value is where the per-platform maximum bandwidth is
stashed. By making that explicit, we can drop the spoon feeding the architecture code has
to do to tell resctrl that the maximum bitmap is all-ones, and the maximum percentage is
100. Bandwidth is always going to be weird like this, so it makes sense to special case it.

I'll add some form of this text to the commit message.


>> Add a max_bw property for x86 platforms to specify their maximum
>> MBA bandwidth. This isn't needed for other schema formats.
> 
> It is not clear to me how replacing one value with a new value that is
> used in exactly the same way addresses the initial complaint of complication.
> 
>>
>> This will allow the default_ctrl to be generated from the schema
>> properties when it is needed.

>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 4c16e58c4a1b..e79807a8f060 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c

>> @@ -248,6 +249,8 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>  	cpuid_count(0x80000020, subleaf, &eax, &ebx, &ecx, &edx);
>>  	hw_res->num_closid = edx + 1;
>>  	r->default_ctrl = 1 << eax;
>> +	r->schema_fmt = RESCTRL_SCHEMA_RANGE;
> 
> Stray change?

Yes, when these were different values, AMD had to override the value in the table.
Since they got merged back together, its the same.

Thanks!


>> +	r->membw.max_bw = 1 << eax;
>>  
>>  	/* AMD does not use delay */
>>  	r->membw.delay_linear = false;
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index 8d1bdfe89692..56c41bfd07e4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c

>> @@ -108,8 +108,9 @@ static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>>   */
>>  static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  {
>> -	unsigned long first_bit, zero_bit, val;
>> +	u32 supported_bits = BIT_MASK(r->cache.cbm_len + 1) - 1;
> 
> Same issue as V4:
> https://lore.kernel.org/all/ca528ebd-fb76-40cd-a495-88c2de443cd8@intel.com/

Huh. I was sure I'd fixed that when you first pointed it out.
Sorry about that!


>> @@ -118,7 +119,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  		return false;
>>  	}
>>  
>> -	if ((r->cache.min_cbm_bits > 0 && val == 0) || val > r->default_ctrl) {
>> +	if ((r->cache.min_cbm_bits > 0 && val == 0) || val > supported_bits) {
>>  		rdt_last_cmd_puts("Mask out of range\n");
>>  		return false;
>>  	}
> 
> The above two changes have nothing to do with memory bandwidth. They are unrelated
> to the changelog.

Yes, these should be in the next patch.


Thanks!

James