Received-SPF: pass (google.com: domain of linux-kernel+bounces-145676-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <6469d1c9-f535-45e8-91b7-992175cf4697@intel.com>
Date: Mon, 15 Apr 2024 10:45:07 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default
 properties
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-5-james.morse@arm.com>
 <c8d1f4ec-eb38-4846-a6ae-722e39ab75f9@intel.com>
 <Zhldk3XV/g/ZZfO5@e133380.arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <Zhldk3XV/g/ZZfO5@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?clducEQ3eVhaSnJkOU5WR1JxTjR3ald1dTc5MWV2U0RhckZCUzF3ajB4clBZ?=
 =?utf-8?B?QVlkM0NFM25tMVdIVnpjem1iVGU3SlVHNVg2aWZHbkRNOE9XZHlCbjU0cXlh?=
 =?utf-8?B?OFU5T2RSNHhuOStOVzJzZngzRW9RQUtoUWNodVhaTnUxbzJCQzNHSU5FOVM1?=
 =?utf-8?B?QW1CZWhwQmdaTGttZGZqVFBCUmo4TG9NeERwNDArbExLSWhqTEVvTGlZTlUv?=
 =?utf-8?B?TTJFTDRxVVh4WHdtYXo4V3F3SVZxNWhHbnpjYU1MUHBqYS85bitXL2dScDNq?=
 =?utf-8?B?SFZWWWloN1pGdkEveVEwUDdGTUh5UHl4SjZYSlB1NUVoRGMwMyt2UWExVGlE?=
 =?utf-8?B?Mk5NK3JNS0xVVDFCNUZBRFNuQkcyb1BjSWhWK0ZjejUzU2IrbEJxQ2s3YlFN?=
 =?utf-8?B?dkdKcFlkaWlpRlJOY3p2K1UxbEdta0l1NU5yWEkvWWJWQjdOKzBUcnEyK2NB?=
 =?utf-8?B?dmZ2ZjhTeVRVMktWbjUwNklkMzVGMnVLeDVuOVJBcXEyd1FLU1pueG5vQnFr?=
 =?utf-8?B?U3JxRHc1TkptelhHekpuRzFCQmxpTzZrY0RWdkVqc29YV1pkR1ZrMXJRdEVB?=
 =?utf-8?B?N3VHTE5IVkhHQ3ZmencwbzByZmNFb3VJRlp0cmZxS1NKTzA3V0ZPR1pEbC9o?=
 =?utf-8?B?aHhFeWhSVnd4SkxTYmZzeDgwbTlkWVRpZ3MxWjQ1bE5HU0xtRXJMVnk1NW5U?=
 =?utf-8?B?YkVLTXpnVEx2Rm96QThYL2NwNm5XNnNhZjhUYWMwanJ6Uzg1VkRDejIrSEtF?=
 =?utf-8?B?cldMN1F0d3dGcHgva2FmdFBmMVpUMnBCNVEvcjZvVGVCd0grSFdaQmcvaEZ4?=
 =?utf-8?B?KzU0aWZpV1c2S1ZSMnEyY0pEZmFnajlnZTh6SzZONW1lc2xyYlYzazgxd0p4?=
 =?utf-8?B?UG1HeHMyZzVqbDl3aFVkMGp4MjlURjlvQk8zL0dpR0tWRzJNU0U5Q0pQM1dZ?=
 =?utf-8?B?clExUjJlVDJ2d2tJU2F5WlhGakQwdk9ZeTJFSTlacytaWVRvY0hMeVlxa1p5?=
 =?utf-8?B?Slp2a0ZlTzZMR0ZlTmx6Z0xQeGUzMExLUDVWSW5md1Ura1c5TzVmaDlmcE04?=
 =?utf-8?B?MFZZWDhnTXhaaDJYUjlkM3RSbGdTS1Y0Y0VDTTRTVnhkWVNTbFQrUUVlTUhj?=
 =?utf-8?B?eTlWdkJVZC8zV3hlOFRraTlBMG5LWi9INEpVU1pMSlJUK3JzeTQ5ZGV6Z2F2?=
 =?utf-8?B?L1R1WUtEZ3k3bVpJYjFTRnhwNWJVSEhXQWtMQk9PS3BNQU5KdVNxa3hzUkpi?=
 =?utf-8?B?RnJFbU43Y0tXai9odU9zUzNDcDdwRHRGWCt6eEczakZXNXJuV3BKVGYrMTdj?=
 =?utf-8?B?VERDTnVyY3VuUmtzSHk0TWphUWZDbzJhV3RWYkRFMStmUnM0SkFUOXRYZXFM?=
 =?utf-8?B?SG5ORkZ4ek1OSm1nc1VSQzNOeS91ZTQzdVJrRWNKMkw0RnIyNXo2Wkc3Nk53?=
 =?utf-8?B?MEtOdFllanNvYnptU0pHVHMvVWordjhwZTBzVEV2NFV2TnNmVHFNNGhtaW5E?=
 =?utf-8?B?T015Y2FIWjlDeVhab2JxSExkdXVBQkZaK3c4QXhIbmY4RkwwUW9uR1MwdmRK?=
 =?utf-8?B?aGN3U2Eyd3ZnM0o4TG9OM1NSSmxTTXR0clNFQ0Y5Z1dTNi82dFd4VU9JQS9O?=
 =?utf-8?B?YmZHdEdsOEpkUWI5S3Q1MWNQTGtWUlNydjYxRzRDWWM0Q0NsTTl3T0dKejR6?=
 =?utf-8?B?ZzdFbEhKWlR6L3pvR095eW9lZmZtZjRYOFV2N25OYm9yQlhQL3V4SlNKNno2?=
 =?utf-8?B?M29BSDVFSXQ2SWpWRW1ueEpWYjBJbWZQanFEaU9TQmpCcXh3N0VKNmkxZnAv?=
 =?utf-8?B?ZzBtYk1OclJpZHFZMjArSmNDaTdqZEVxYmRkODN6djVoMWpVWlU1UmNXb2Rl?=
 =?utf-8?B?aWtuSitSM29Ba3VkLzB2NExkd3JseWtDRmlLcWV4ZjIxbk1nekphaVFNUEZE?=
 =?utf-8?B?R0F6ZFpvZXd0WXhaaEszKzliY2M3UUVkZnVBcUViL3IwMm5PYWZkcERPMjFw?=
 =?utf-8?B?M280QjhBeEdaUmtyTjlTZTJTOVBxbXRscVo0Z1U2Wnk0VFl2SWJRV3dwZktr?=
 =?utf-8?B?VC85djU5Uk9zQXNXS2ZjTW9ndEVVdUp1bmtycVdETW01alBCRXV0ZEZqVFFy?=
 =?utf-8?B?YkovR2ZzQ2I3L1JhZ05wdXFjcUpYdVh5TlBZczNySExjdW5MWXJEYTNVVC9j?=
 =?utf-8?B?Zmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b187c68e-b6e3-49d9-77c9-08dc5d73cb74
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2024 17:45:09.6950
 (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: XBQyEJhKA6EOnIcysrM1ya1WQwWcrrcG1XhEeTcD2wa5Mg/95hGOXIHpXN7hxBpIO7GNPCml9jAOVx7vdE9QyJFnrvF/EdY17xV6gvdobcw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6320
X-OriginatorOrg: intel.com

Hi Dave,

On 4/12/2024 9:13 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:15:03PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>>>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>>>  {
>>>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
>>> +	u32 closid, rmid;
>>>  	int cpu;
>>>  
>>>  	/* Give any tasks back to the parent group */
>>>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>>>  
>>>  	/* Update per cpu rmid of the moved CPUs first */
>>> +	closid = rdtgrp->closid;
>>> +	rmid = prdtgrp->mon.rmid;
>>>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
>>> -		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
>>> +		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
>>> +
>>
>> While I understand that the CLOSIDs are the same, I do think it looks unexpected
>> for the CLOSID to be set to the CLOSID of the group being removed. Could this
>> be set to CLOSID of parent group instead?
>>
>> Reinette
> 
> That seems reasonable.  How about something like this?
> 
> -	closid = rdtgrp->closid;
> +	closid = prdtgrp->closid; /* no change, but the arch code needs it */

Looks good. If the comment stays, please do replace the tail comment with a
freestanding comment (for reference you can search for "No tail comments"
in Documentation/process/maintainer-tip.rst). 

Thank you

Reinette