Received-SPF: pass (google.com: domain of linux-kernel+bounces-132242-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <4a6b1645-c72c-41dd-a455-bdf0ec57d4c5@intel.com>
Date: Thu, 4 Apr 2024 16:11:38 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM
To: Peter Newman <peternewman@google.com>, Fenghua Yu <fenghua.yu@intel.com>,
	James Morse <james.morse@arm.com>
CC: Stephane Eranian <eranian@google.com>, Thomas Gleixner
	<tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov
	<bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Peter Zijlstra <peterz@infradead.org>, Juri
 Lelli <juri.lelli@redhat.com>, Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>, Steven Rostedt
	<rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, "Mel Gorman"
	<mgorman@suse.de>, Daniel Bristot de Oliveira <bristot@redhat.com>, Valentin
 Schneider <vschneid@redhat.com>, Uros Bizjak <ubizjak@gmail.com>, Mike
 Rapoport <rppt@kernel.org>, "Kirill A. Shutemov"
	<kirill.shutemov@linux.intel.com>, Rick Edgecombe
	<rick.p.edgecombe@intel.com>, Xin Li <xin3.li@intel.com>, Babu Moger
	<babu.moger@amd.com>, Shaopeng Tan <tan.shaopeng@fujitsu.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@intel.com>, Jens Axboe
	<axboe@kernel.dk>, Christian Brauner <brauner@kernel.org>, Oleg Nesterov
	<oleg@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, Tycho Andersen
	<tandersen@netflix.com>, Nicholas Piggin <npiggin@gmail.com>, Beau Belgrave
	<beaub@linux.microsoft.com>, "Matthew Wilcox (Oracle)" <willy@infradead.org>,
	<linux-kernel@vger.kernel.org>
References: <20240325172707.73966-1-peternewman@google.com>
 <20240325172707.73966-4-peternewman@google.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240325172707.73966-4-peternewman@google.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?ZERkYnEvOFprOTJtNmYvaUVUdEFwT1o5aUNSclZVRUJYajF0M0l2Z2Mzb2k0?=
 =?utf-8?B?SmRsUkd1dklmNGpLd2RDUFRFWm9MU2RKOTFnUFMvY2FEeEJ3WFVxRDVlUUFX?=
 =?utf-8?B?VHRUT0RZcWZFQzlKSzZTckNReE9ucUtVUUp3d1dMQ0xueFJzeUJFbG1jWUh3?=
 =?utf-8?B?b2U0NnMxWnpxc24zRE9EbFBBcEE4cDN2d3VwcjIvdXhiNjJpc0E3S3BUTU1h?=
 =?utf-8?B?SHIvYkl0VVVHdVhUUzVNM1pzMERsOTZqV2x5R3pNMG90cVRCM1NxSnl5bm9Q?=
 =?utf-8?B?eG0rcEFFYkVadGp1bmcrWng3VStpTVo5VmNnYkFqdnI4OWdRV00vS2FBSUw2?=
 =?utf-8?B?WmdaSlFrTmp4bDBVOGk5bExnRjFoazRLdGVSTlZkaXViWlh0djhmYmRlSzB0?=
 =?utf-8?B?b0R5bzJOOVQyVFNaeXFZN3lrYjE4bGJKVy9OUUVkMHhmcFAzZzAySURrZ2VV?=
 =?utf-8?B?VVoyNTEzek5Eenl3bkI1ZnNnd1lEZHFsZWcyLzg3TnVEQzVxWU44UkJ2UWU1?=
 =?utf-8?B?OWM3cEpqekJ5VFp5QkZWMjg5YjRKSUc1ZHArU3JLWHJLc216UytCYXQvdmJq?=
 =?utf-8?B?MVpBRGI5RlBhMFd2VXpvWUVwQXFoN3RCTGQvVVNmeVNGdmJPYUJ0WXY0bjNG?=
 =?utf-8?B?dGRaNmtyMEFlcDhIQUlsRWRBclMrYjVMaGh2V1RyOFNtd2RNRzJCNWdvdHhp?=
 =?utf-8?B?MkVKcTJtc2hTVjdwbEk5QWRBMU5OalFWQXpmV1M1b255cXRic0lDUllscnpr?=
 =?utf-8?B?bkxNQStCaXh5SnZXbVJnRzYwZ0xNdFV0NlgrcXlVTVpVSS9rZWkvSWdCdnhV?=
 =?utf-8?B?UXRsZU5lRiszNHdOWlRFT3F5ZVg0dDlUZVZBN1FUVFA2SEdzbkREWXR5OW1M?=
 =?utf-8?B?K1piUmJxUmMySVh6NlF1d082ZVd3eGU5NFJrWi9yRkJDTWhiWlZwclFpTmdG?=
 =?utf-8?B?clR5NysyOFlzM2NxR2k1T0szTVZMbmFlVUY0MElndFk1UE5LaGkrdXFQaHYv?=
 =?utf-8?B?TVVPaTNtUU8zdmZ0cHhYcURJWmlhWFN4ZVNadXl0UlNoVURwSkpGTndMN0Vn?=
 =?utf-8?B?VktmaUpoNDg4cDUzVUdQdW9vYmtrQVhubld4b3I5VG5wMHlPRTM5SDRKMTFj?=
 =?utf-8?B?TGZJeUdyeGhkeHZ5ZHhWeUVsUHd4TUVDK0l1WWwyb3p5YTRySWRnbEc0dzIx?=
 =?utf-8?B?R3hKcytLbzNtV0xUTENFc3FjOGdkZllhVklXeUp1anFPQVBGOHFxV3RSN0pQ?=
 =?utf-8?B?R3JRU29XeGoxSVY3NkcwRStMKzgxamU5c3Mrc3RUdFc2eXhOdGl5dVBDQXJq?=
 =?utf-8?B?SDJvcWJ4dFZVZ1p3ZnlERVJ4NE9GV2RvZms2UDVEakg3eHR1RWY4UmQyY1lE?=
 =?utf-8?B?RjBYZEVHSjVsc2h1U2x0a1VUZGQvbWUzTm9uWTJtR2FnZnc4SXM1WTJlNGNk?=
 =?utf-8?B?MHF0MS9LU1ZvREJEa0FqeUNmOWZ2aTVWazc5ZlRJSVAvL0VhL2dEbmx1QW9k?=
 =?utf-8?B?cFpweHdsY2xPWEpjL0c5dEdzNE9nSU5IRWJvekJNd004OWp2NHN6cmVmSlBz?=
 =?utf-8?B?M2JEV3FvSzYxN0VqVE9Xa1VKdzFOQmk4c0FUdFhzTEtkNlBBeEZkS01zMi9H?=
 =?utf-8?B?TTB3MVVSMTFoeFNKdHdqVHNrdHlHWStXMkQxYkVEOFQ1WGtZaTBuaEtETjJr?=
 =?utf-8?B?bldOeEZhclo5Q2Z1blRRR1pLUzJ3RXVkMHZLL2xGc21iYnpnb1lTbE40THJF?=
 =?utf-8?B?R3lIV2dzeEttUGprZjRxa3g1N2l6Q0pIT0l4eDFqZ3hjREwrQ3JaVGhieXVI?=
 =?utf-8?B?Vmptemhjc2lCVXlDK3puU3JNRkpqTk1JRGVXMWhjVGhNY0g3U3JwSGVzYW1D?=
 =?utf-8?B?OU5ZMUM2QlFBUlZnM2tXWjJJY0l1bUJRbUJKWFY5Z2tkTkR3UVRldXRENWdj?=
 =?utf-8?B?VzF3M240cUJ0TFJ1cEIrYzhtTTB5VzhISElFWnhhUVBIZVUwVVI0Tm9ycms4?=
 =?utf-8?B?ZHh3ODJqelhQc2ZreEkzU3lsQ2dxdkRXY3FKbWFHSWRTTVR1TWVhL0E5bkRQ?=
 =?utf-8?B?OFduN0ZwSHN1YnZlUXlMMHlKVDRiYW90OFFUSllaUUMwWUJrU1ZMekhRWFVC?=
 =?utf-8?B?NTdlaUgyVWo3bFpBTk94RE1FMUJIWmg4UTJiRUU5SXNvL3FsODNlR3I3alBU?=
 =?utf-8?B?K2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 16a4cb15-3464-4258-915f-08dc54fc968d
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2024 23:11:41.5309
 (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: Py/GIXeurSYioApkJdaSJZoKRxsej6AlHQ+zvES6Vbgr+XxBySEU2ugZjoOp6SCxT9AJOtFEVcyjiNBvXnKRGALg4h//4drOwHkGVcdnF24=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4847
X-OriginatorOrg: intel.com

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> Moving a monitoring group to a different parent control assumes that the
> monitors will not be impacted. This is not the case on MPAM where the
> PMG is an extension of the PARTID.
> 
> Detect this situation by requiring the change in CLOSID not to affect
> the result of resctrl_arch_rmid_idx_encode(), otherwise return
> -EOPNOTSUPP.
> 

Thanks for catching this. This seems out of place in this series. It sounds
more like an independent fix that should go in separately.

> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9b1969e4235a..8d6979dbfd02 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3879,6 +3879,19 @@ static int rdtgroup_rename(struct kernfs_node *kn,
>  		goto out;
>  	}
>  
> +	/*
> +	 * If changing the CLOSID impacts the RMID, this operation is not
> +	 * supported.
> +	 */
> +	if (resctrl_arch_rmid_idx_encode(rdtgrp->mon.parent->closid,
> +					 rdtgrp->mon.rmid) !=
> +	    resctrl_arch_rmid_idx_encode(new_prdtgrp->closid,
> +					 rdtgrp->mon.rmid)) {
> +		rdt_last_cmd_puts("changing parent control group not supported\n");

Please start message with capital letter.

> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
>  	/*
>  	 * If the MON group is monitoring CPUs, the CPUs must be assigned to the
>  	 * current parent CTRL_MON group and therefore cannot be assigned to

Reinette