Received-SPF: pass (google.com: domain of linux-kernel+bounces-149513-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <069c0e2e-81e1-4343-8a9f-21f1cb74bde6@intel.com>
Date: Wed, 17 Apr 2024 22:12:35 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from
 update_cpu_closid_rmid()
To: Dave Martin <Dave.Martin@arm.com>
CC: James Morse <james.morse@arm.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "Borislav
 Petkov" <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, <shameerali.kolothum.thodi@huawei.com>, "D Scott
 Phillips OS" <scott@os.amperecomputing.com>, <carl@os.amperecomputing.com>,
	<lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
	<tan.shaopeng@fujitsu.com>, <baolin.wang@linux.alibaba.com>, Jamie Iles
	<quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	<peternewman@google.com>, <dfustini@baylibre.com>, <amitsinght@marvell.com>,
	David Hildenbrand <david@redhat.com>, Rex Nie <rex.nie@jaguarmicro.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-6-james.morse@arm.com>
 <6bbff669-cbe5-4284-b64a-4825a541b35f@intel.com>
 <ZhldWJVck7VmU3G3@e133380.arm.com>
 <0698418a-aede-40d6-90ce-dbf6e8796916@intel.com>
 <Zh6kchkH9Dy+ywXc@e133380.arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <Zh6kchkH9Dy+ywXc@e133380.arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QlZVWXFvdzN5NldrbnNmMGtLaVNEdmN3QkgyQVd4bDZlZ1pqSFRrS1JPakFY?=
 =?utf-8?B?d29qbUVUaUVkWkJXSjFQN2oybktXZTJ0REFFT0FSdVhmZ255b2xkY3NBeTA4?=
 =?utf-8?B?Vk10Rkk5eHd2eFpLY1FYbkFQbkk2WGIybVgvRnlGYWhMdWN4SUdqWGpHVVgv?=
 =?utf-8?B?blFKZEVCa0gweDV6clc0NDRjb2FCR3J3QVNzQkI2WUZJWHQxT0xuQk11WnFw?=
 =?utf-8?B?WjNDWXpXdklVeGxISEZjWFpGU3RqUWJyTkZMZ29TY3V6TjNsdVZobFlnd2tT?=
 =?utf-8?B?N3YzRllST2NIUVFUREIwbzNLMTNCc3J4cmpyOEZhcUxUV2RiZDZrb0xSOW5U?=
 =?utf-8?B?Rmo3SXBvTWJkZmtpejdublJUcFFaYno4dWtTS1pObDVHem1DdFhBTXMrSkdV?=
 =?utf-8?B?dGpEaWhzMzZ1UWVwVjNicms3M2JRLzZVb1lYeTFFRC9GWlg5S1RicGFnTkhH?=
 =?utf-8?B?UGdYdzI4UE9VMDhCaGlqeHgwV3hKY3I5MmNJUUU1ZFB4VmowMzFhWHhKYW1O?=
 =?utf-8?B?UUg1MTZyNno1VTd2dmFsQjZlT1NoakFlK2V0T1lnR0hZRDgwWVhQeVhaRWZm?=
 =?utf-8?B?VVVBdUNYY3gyS0tTelU1cmE0ekptVFdlWXpFNGpvR29aZEF4V2t0cHZyQ1Rn?=
 =?utf-8?B?S1FnUnFZTHFNWnB0NncrUHQ2RHUwSzFlbXRBQ2tBWFJxT0ZVUjByekhLeUxp?=
 =?utf-8?B?Rk1qczRHczVEZGpJaEdveTNCWlo1Q21jenh1Wkg4MVdEdU81ZlBnUFZzZS9M?=
 =?utf-8?B?WkR0azB4MkdNNDJzR0gzdjNlNWJ1UjhPdWV4UERDWjlTL2doWm9yQlB2cTJT?=
 =?utf-8?B?anowU2gxMmROYmRMUWhVcStwRGh0U2tac2JKUDJxRXZ6Vk0rVVhVd2xUTVRR?=
 =?utf-8?B?NGp5UnNUM1M2NjJBSDBFNjRtMmY0ajJKSHhINVlERCt0Q2FPTjVEekVHdkUr?=
 =?utf-8?B?bkFid3NYQU1rdEF3MFA3YmpnQWs2bzlydWVqVUtTd0hqd3F6NlQyZ2ZuUjc3?=
 =?utf-8?B?U1N1ZWsvdFQrYVR5czFQeHhjbzRGTUxkRk50QWRnaStCbnhWWXFEY1FzWGlY?=
 =?utf-8?B?d2RzbEZvOEYvMFVsQ0hwcktCZ1ZFakhyUGxoZTlqbUJINEZhMnArSkdEcThT?=
 =?utf-8?B?NHFMOGNSYVhHak9kSjRlWU1BczAxL3dERk16TWFzSEpCZ2o0UktDaDFMaWo3?=
 =?utf-8?B?R2FFM1N2YkZ2V1JrR1hSOEVCNWlrQzdxN2x4U2cvSU84V2JDZHU5OEtwNlZB?=
 =?utf-8?B?SHloVFNLRXhkNklwcGEvYXh0U29tNFI5bGhIeVBLK3hIS1prRHB1eHNmVjRo?=
 =?utf-8?B?ZzhMVGUya3ROSG5GZE5yMnlyeHVpUW51aVlhclpxaW9LRHNsWTBqa3VmYkta?=
 =?utf-8?B?c1NPT2ZUdnZKazkyN2g3c3pDSEhyYm1SU1pyOUN3YUxyWjQrSElqWmJjT3NZ?=
 =?utf-8?B?NzV1L0VZdE1Iai9qUFJnK0M0QTE4clFTc0V3MUQ2N1NaOU91VEZ3ZkdHVnIv?=
 =?utf-8?B?amwwaDNVb25FVFkzMXU1WW9IbVZsNkNmOTh3WlNOWXZvM2tlUlQxRit3MnNI?=
 =?utf-8?B?Y1lvbEpmVTZ4UEI5Skhvc1VkejUzRUgxRzd3cDlqVzkvQkpHakwxMDZJZ091?=
 =?utf-8?B?TEhrOVRSaWg0T0FWK1ZJREU5cmNMckVLOXM1MWdUL3R3cHFXcW5oaVV4Y3BQ?=
 =?utf-8?B?MFhyZ0x0OGdNOW9aTHNkMnp6cVh0aER5OS9YME1ZQXNsK290TGE4RkR5TG5Z?=
 =?utf-8?B?T3lyamMwV2pidERXYWpiWGRHZWZYcmpNMzZrZllWdnplSmw5U2Z3SnBjYm9w?=
 =?utf-8?B?S2lEL0JDL0xrOExhREVIMW5SQ1BaOElOWVdEZTlVQ21LeGFYWk5IR1FDMjhr?=
 =?utf-8?B?M3JOb05NOVRZZUFDK3VzZnVvUHVpY29ZaHhSbTlyTSsvbEd4STljRGRoL2Fm?=
 =?utf-8?B?NkJRZHlja3k4ZFdPM1FkMGtoU2d5c0hTclc2VHFIVWFLcHU4OTlVME1EeXRX?=
 =?utf-8?B?OGFTNkdBSmxKUDZLYm5xYUdRZXFlcGRpSzZ6cXZGYmQ3MHRhNUkyZmw2VjdI?=
 =?utf-8?B?enNiOFhBb1J6djdTbU1tS21PK1M4YmsrckRyMERqQ3NWR0pIa3RVWlZUWkRE?=
 =?utf-8?B?S3lmWmlQOHJUUDBXRXBraFlpSlRHVUFmUEtXc1hIUFZ6N3gyWGJjV0tXR1BY?=
 =?utf-8?B?bHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a795e9a5-c436-4542-31a9-08dc5f662ace
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 05:12:39.0569
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lngrRq/RS7sv+TmR12b1XH1/qR+S2Vnstnr6M/w1ReTD/WxvyDeRLY/lT+EKSTbaNIUBIoXTw7qD0v5q354vZReDPF5OKra82r7ceVTbQyg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5212
X-OriginatorOrg: intel.com

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