Received-SPF: pass (google.com: domain of linux-kernel+bounces-132235-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <4681c9c5-3364-4628-a581-0366d0024211@intel.com>
Date: Thu, 4 Apr 2024 16:09:12 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 0/6] x86/resctrl: Avoid searching tasklist during
 mongrp_reparent
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>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240325172707.73966-1-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?U1N1RnR3MUw2a3VEZ2lRR3l0UUorWHdVT0QvZzY5YXpaT3Q3aENnUlZKaDlt?=
 =?utf-8?B?aWlWYlAzSHNETXpTbkc3cVZ0and6MFZPOXlGbTlqVDhJTmVMRzhaeFZhUE5l?=
 =?utf-8?B?Zzc2dDl3NnJLRnhuK0dKaUZabm0wemg3MzFNNXgzTWpqQU1Vdm5HTUxpb1Jh?=
 =?utf-8?B?d0Jlcm1IemtyUVVkd05NcnBvc2htTUdnMnBDYVFpeERXRzhEVzkwQjE2bmtz?=
 =?utf-8?B?eVR2NGwwNHBYZkNNMXRqSUlOTzQ0SUozalRKWDh0MmZlOTRSSEgraU95dWov?=
 =?utf-8?B?anFLZTgvL09NNUdJQkdSNDJHSTkwaHNOZ2xJQmx3cnRxVTBoNXgreVdQWTJn?=
 =?utf-8?B?U01GOXd5eFIyWDRSNW1tSmJ5Qmt5VUpsRUxpWCtsRzc1S2lmWW5GUCsrV2Fp?=
 =?utf-8?B?S1ludmhTM0l5YnkrcDFVbmM4WndvL3hnK2hOZytGMWF3MENzNzhwRmZiWnVI?=
 =?utf-8?B?dThCRWd3Q1JmRXZ1MEdEMUw0Ni9UMm16K09XdG1xVE4zUkNsd2dIOEF5WjJZ?=
 =?utf-8?B?V001VXZ3WUlERmVIMnBFOThzaEhOVGFqRnFOa0NVUU95M0lnVVR4TTcxUXhJ?=
 =?utf-8?B?MFNrTFFoVVMxS1NqWXo2dHlxZE1rVTlaNGU4cE4rQ2gvZXFTQVltSmJMeU5C?=
 =?utf-8?B?cTRrWGlmSkR1RWdJbjJnL3F5clFrRERCRGRYOENsaWJHSFRmaFV1ZjRlMnlj?=
 =?utf-8?B?N0s1ejVLR3JvYWsvaS80UEVRNXF4T0NDdmp3MDVqYlA2ZDNudUpMWXM4OUVD?=
 =?utf-8?B?THpxV3pFdG1PWjZYQXNwUEtYQWgxdjFoSloxVUhqcXZoVkdUMDcwNEJhVkZG?=
 =?utf-8?B?YnJFNkVxa3Jmem9iYXJuWWY0bXNBc1RCQXVsL0pncDlFTkR6M1ROMlVHOGVC?=
 =?utf-8?B?V2tXUjViVDVyV0JpUEtZMU8xZ2RUb1VNdWZIN3pIL1JnUDgvdVBBQ2c2VDgz?=
 =?utf-8?B?azBaYTAxSWZUMmVZMEUyWXBVb3IzSWdLb3plaTdobGVTSkgwNlJDNkhjK3RV?=
 =?utf-8?B?Q0ZMS2JiWXpkbS91RVBUeVEwV0hTdmY1djM4OW1kMytTVi9sbWxHcXhiaWhX?=
 =?utf-8?B?Sk9zT1NMVDIrZHgza3FJZ0k2RzhYZGlneS9Rem9HM3BTay9QcHVvRUNKQVlu?=
 =?utf-8?B?OWxxYXR1MXdOelVlbGVJNlRVcENLSzNWWXBvT3V4NjVaeHBTN3lMTng2SjRS?=
 =?utf-8?B?UVhGVk1WZFlRT3pBTkZtYm5LZ3h2Z2dBQnFtd0ZqNEFKQWdMdHg2UUJCMXAw?=
 =?utf-8?B?ekdyNkNFTXUvV2hVdGgzd0RQeWtmUHlUZzFELzJrK2t4RGdIU0JRUGl1QjdT?=
 =?utf-8?B?K0pwZGpZbWVmOTY5RnJ4NDZISnNuYldxSTlyZWNQSmdkYVZWeGYvYmNKcjZY?=
 =?utf-8?B?MVZhaFBVNDNIaHptOGxhNDlWVGM4eEdYSEw2RXhDbVpKeS9hNURQNjB1M05x?=
 =?utf-8?B?Q2dwc1c4eDBGSmJCTHJJclZ5eXY2NEVWWE9lUUYxWEF4SkN0dHY1NitmYjlO?=
 =?utf-8?B?RCsydE05K2VMLy9WVW9XWEs2Z2VlZTBmLzdVTFBLWU5jdmg1MThCT0F5WVo3?=
 =?utf-8?B?MmFIaWZ1TWxuOW81UDE0ODVyMWhtVXpvSFJSQVZ1ZkF1eE8vU1N1N1dsa0tD?=
 =?utf-8?B?SHdPaXZtaTBRa2hsczdldGE4Qjc3cVdSWk85L1hGeTQxcUlkVFhJL25FMS9z?=
 =?utf-8?B?d1BkOUpyeDJKcThNVHVORzUzeU5ZaDF0TVNmNVNMd1FqS1VqVTVaSkpWd1d3?=
 =?utf-8?B?Nmlhd3RsVlBNNjJqTHBjeW1sN2xKR2xiaGFUalB1ODc0TStQRjBwV3Q0UjBv?=
 =?utf-8?B?Sm1pRnBvTzRGZGUvWngvUmZOTmpjblVBWTdiZzU5bE9iQlZDL0xyckhpaUtn?=
 =?utf-8?B?RHozU0E1QXpyOEUvZkVWUEFMNmtIU2x5QW51bXJucHhHc05WU2xJeHVudmcv?=
 =?utf-8?B?RDgzblc2V1UxVCtOelBZMkdkS3ZId0R5QmFBRUpuRW5aSHJzYmdtKzc0Unpx?=
 =?utf-8?B?emx0RUEvSXlMTGVnTDVwalVSSWFhTU9kc3Q5aUY2RWRWb0xXMUNUZEswbUhi?=
 =?utf-8?B?YjhJSnhWQ3BvTTVlNlNLbW5odHdOeUNCN3ZIOWRNd2cwcExzbnprOGRNblUx?=
 =?utf-8?B?UW5UUUZDcm9VK2ZmblphUkxubGhsQzBGckViUU1YbDRvUmIvbXQ2djFWNTdP?=
 =?utf-8?B?cWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 72dd1335-6f6a-49f6-e481-08dc54fc404c
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:09:16.8901
 (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: x9iNI4qWBgV1pyxzpPUu8bOX2fdHLrQYuHnPm2gTSKWRTwD1cnDAhLlJtv9CjmFZTN/YiKK8yY2eEHi7hoJHwiYtG6dcgLNod0GIpPmpwCM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5069
X-OriginatorOrg: intel.com

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> Hi Reinette,
> 
> I've been working with users of the recently-added mongroup rename
> operation[1] who have observed the impact of back-to-back operations on
> latency-sensitive, thread pool-based services. Because changing a
> resctrl group's CLOSID (or RMID) requires all task_structs in the system
> to be inspected with the tasklist_lock read-locked, a series of move
> operations can block out thread creation for long periods of time, as
> creating threads needs to write-lock the tasklist_lock.

Could you please give some insight into the delays experienced? "long
periods of time" mean different things to different people and this
series seems to get more ominous as is progresses with the cover letter
starting with "long periods of time" and by the time the final patch
appears it has become "disastrous".

> 
> To avoid this issue, this series replaces the CLOSID and RMID values
> cached in the task_struct with a pointer to the task's rdtgroup, through
> which the current CLOSID and RMID can be obtained indirectly during a

On a high level this seems fair but using a pointer to the rdtgroup in the
task_struct means that the scheduling code needs to dereference that pointer
without any lock held. The changes do take great care and it
would be valuable to clearly document how these accesses are safe. (please
see patch #4 and #6)

> context switch. Updating a group's ID then only requires the current
> task to be switched back in on all CPUs. On server hosts with very large
> thread counts, this is much less disruptive than making thread creation
> globally unavailable. However, this is less desirable on CPU-isolated,
> realtime workloads, so I am interested in suggestions on how to reach a
> compromise for the two use cases.

As I understand this only impacts moving a monitor group? To me this sounds
like a user space triggered event associated with a particular use case that
may not be relevant to the workloads that you refer to. I think this could be
something that can be documented for users with this type of workloads.
(please see patch #6)

> 
> Also note that the soft-ABMC[2] proposal is based on swapping the RMID
> values in mongroups when monitors are assigned to them, which will
> result in the same slowdown as was encountered with re-parenting
> monitoring groups.
> 
> Using pointers to rdtgroups from the task_struct been used internally at

"have been used internally"?

> Google for a few years to support an alternate CLOSID management
> technique[3], which was replaced by mongroup rename. Usage of this
> technique during production revealed an issue where threads in an
> exiting process become unreachable via for_each_process_thread() before
> destruction, but can still switch in and out. Consequently, an rmdir
> operation can fail to remove references to an rdtgroup before it frees
> it, causing the pointer to freed memory to be dereferenced after the
> structure is freed. This would result in invalid CLOSID/RMID values
> being written into MSRs, causing an exception. The workaround for this
> is to clear a task's rdtgroup pointer when it exits.

This does not sound like a problem unique to this new implementation but the
"invalid CLOSID/RMID values being written into MSRs" sounds like something
that happens today? Probably not worth pulling out for this reason because
in its current form the exiting process will keep using the original
CLOSID/RMID that have no issue being written to MSRs.

Reinette