linux-kernel - Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid

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: <069c0e2e-81e1-4343-8a9f-21f1cb74bde6@intel.com>
Date: Wed, 17 Apr 2024 22:12:35 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Dave Martin <Dave.Martin@....com>
CC: James Morse <james.morse@....com>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>, 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>
Subject: Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from
 update_cpu_closid_rmid()

Hi Dave,

On 4/16/2024 9:16 AM, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
>> On 4/12/2024 9:12 AM, Dave Martin wrote:
>>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
>>>> On 3/21/2024 9:50 AM, James Morse wrote:
..

>> Do you imply that this would maintain the order in this patch? It does
>> not look to me that it would but I may be looking wrong.
> 
> I'm not sure without looking again, but since this discussion is not a
> good use of your time I'll just go ahead and implement the change at
> [*] above, while restoring referse FIR order, if that is good for you.
> 
>>
>> sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
>> one line.
> 
> I guess that might have been split to stick to the 80-char limit.
> 
> Due the the small size of this function, shall I just rename defaults_p to p?
> Alternatively, there are already a few non-printk lines over 80 chars, so
> maybe we can tolerate one more here?

80 chars are not enforced so strictly that it impacts readability. You
may refer to how update_task_closid_rmid() looks for more confidence in/
motivation for placing this on one line.

> 
>>
>> ..
>>
>>>>> + * struct resctrl_cpu_sync, or NULL.
>>>>> + */
>>>>
>>>> Updating the CPU's defaults is not the primary goal of this function and because
>>>> of that I do not think this should be the focus with the main goal (updating
>>>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
>>>> the defaults if *info is set but it _always_ ensures CPU is running with
>>>> appropriate CLOSID/RMID (which may or may not be from a CPU default).
>>>>
>>>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
>>>> and the comment needs to elaborate what the function does.
>>>>
>>>>> +void resctrl_arch_sync_cpu_defaults(void *info);
>>>
>>> That seems reasonable, and follows the original naming and what the
>>> code does:
>>>
>>> What about:
>>>
>>> /**
>>>  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
>>>  *				      Call via IPI.
>>
>> Did you intend to change function name?
> 
> Er, yes, I meant to use your suggestion here, so:
> resctrl_arch_sync_cpu_closid_rmid().
> 

I'm a bit confused here when comparing with your response in [1] mentioning
a change to another name. 

[1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@e133380.arm.com/

> Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync
> to resctrl_cpu_defaults seems good, since that accurately describes what
> is specified in the struct (and what is *not* specified if NULL is
> passed).

Sounds good, yes.

Reinette