Received-SPF: pass (google.com: domain of linux-kernel+bounces-132251-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <52d0116f-8fa1-4ae0-8012-8f6cf2765cc6@intel.com>
Date: Thu, 4 Apr 2024 16:18:54 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 6/6] x86/resctrl: Don't search tasklist in mongroup
 rename
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-7-peternewman@google.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240325172707.73966-7-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?WnVyQVg3YjdKSktmeWo2OUpkSnNIdVFIdVZCRklNdTVGdWFud3IxS0JMa1FO?=
 =?utf-8?B?S1hjMTFzdWRjN2w1UE1PL0tRNFUxcllGZmQwbjBhbFgrMlRSRFFKMjUrMit4?=
 =?utf-8?B?YThiUTBjU0pUZDU2eW5mRHZzUExZRUlGNVlIeURmMFlrVitQbVZFRU1SdHBJ?=
 =?utf-8?B?aFlLanVIamVJeFlXN1Qwd1VrM3VMZ2x4djNzL1NZSExNQXlDcm1IVnZOVWc3?=
 =?utf-8?B?d1RCT1F1L1RDNjE2cGVXM0doSFcvVEttKyttL05acjRCd2xYUDhmZUFHQkR3?=
 =?utf-8?B?aUZteEc1cmZCemhNb0wvV2QwbXg3TTlIWW05c0U5aWJOSkRSS1Z0WWZaMWt4?=
 =?utf-8?B?V2VyL0d2THZFWG93Mkd6RFM3a1dkVW5XUnJvMytrM2J5MkxiQy9KcTRWNWpK?=
 =?utf-8?B?VHFUNHE2KzFSQjYxTmdHK1A2c3VFWUs1aUFONEpmTkZic1dRempObkV5OHFT?=
 =?utf-8?B?bTdzRldpTUpKcEFwNG9kY3pqTnVyOG0vREZXSDVaRDJmU3U5VUFEaWhQQnpr?=
 =?utf-8?B?eEZmMjhVbW1wT2hIL1pTSFQzTzVEWXp4anh5NjEydUJGR1pOcTZvTVIvV2Nr?=
 =?utf-8?B?c3QwZTY3MTYzSzErQXNya0JjKytYYVVMOG85Y0xhTE42SC9RL09Pc01maE1o?=
 =?utf-8?B?MTBpZHpTYlA1eE1jaS9RRlhCY0w4eWtoWVY3cWNHUzYzL05Xa0VmZDFhSjVz?=
 =?utf-8?B?SjFrVFVMRDJ0RnlkczFCWUUvVFZmS0xkM1RnSU81b1Nyc0t1c0syQlo4N2p6?=
 =?utf-8?B?SC9PUTBrY1U2Yy96aVp2YkNSa0UwQWx4LzFCQUUzR1NnMjlNdFVhc1kxZ0x5?=
 =?utf-8?B?TFh5MHBaekoxNWFJcWV5TVI5NWFEcjRWNnVLeUFkUzlFQ2grWTh4MkxKZW9w?=
 =?utf-8?B?WFR5ZWk1REQreFBHV3JULzlueS9IOEp6RW53WFZzMExQR0VCUUphMm9NVUFE?=
 =?utf-8?B?ZFY1UGpRYlBEYXVuUHh3VGJ4WWtxcHVNRHUwTGIyU25wY0kvSG1jY1c3anhV?=
 =?utf-8?B?VFdHY3NmdVpmQ3g2MFlxVWxVT2ZGT01XUHJpMUM0SHViSWxSRFlndmtVNFRN?=
 =?utf-8?B?OXZaT3JiZ3EvZEZxRDJVbHptWGlxaHc3LzRkSnB0MVV3aXZOTWtNdWhJU0JV?=
 =?utf-8?B?MVRmd3V1bXpaaWkwVGs4eUlZNjJ5ZFJSeVRCM0FrSm1BQTdlVnZJYU4yUTBM?=
 =?utf-8?B?K25yaXBlT25FRHlKOForOHN4WDM2U3UxOVVmRGJCWkZGTjJGamErY1RiQzg5?=
 =?utf-8?B?SGRHN2YzZnR1S0NPUnR5WmNucVRaelpCNWMzOWxPMkdWcHFUTlN1N3JibmVp?=
 =?utf-8?B?Qkd0WDJKN2N5UVNOSTFZaEN2Znd4ZjJMUzRNTEF3bWdsb2YxcHB3dEQ1WVIx?=
 =?utf-8?B?WjcxSkJGakJvOGdkSlgxb1RpcVZLbXRKZVJQS3IvSDdnVlgzbDZUSkFLTHcy?=
 =?utf-8?B?eXgzalhvZkkrcCt3OURoR1l2b2x2anRkUVlnSGxFNDgvNVRRbmFOMVgrbUs2?=
 =?utf-8?B?VFZLQ05aTXdTTDVIcEhLV0svMW81N0NaODRUQVduTVZSWFlZUmhhME9FeGox?=
 =?utf-8?B?ZHVUYUN0QzJWMFkrbmFlKzlkWE4wUzZYRGVnRDV5UkxWMERCOVdzZVo2cERF?=
 =?utf-8?B?eHNmdjQ0eUgyTHYwUEtsUnYwbzQ1N2JCeXA3YjdPc1VJZ0hiT2k3bXNyOTRt?=
 =?utf-8?B?WjlOR0tYUEdueFZNRlVBTFNBTzlIQ1hyRDlYZVdWdlJUZ0VYOFNabUdTVEZn?=
 =?utf-8?B?dW1ZUkNaQ2ppV1hKYXNnNUFrMjQ5a2RaTVp2OWJQcEpMc2FsSjFjYVQ1WWth?=
 =?utf-8?B?N1hxNC96MTRUTGdLQlUybWJIRS96c1NXUXdqR3g2VEYwSndHdnh0V29aVnBy?=
 =?utf-8?B?N1g2UTQ5ZnF6R3lLb2luc2tDYkdOOGhRQ2s4Um16MElHYkZ6TDY2K3BrK1dw?=
 =?utf-8?B?VkRQMUVpQXZVem9jamdqSTJtS3JiT1M3TjUzbzBHL0tYRUdqNFJhOUJmVVM0?=
 =?utf-8?B?NWpvTXlaVkpEYnZNQ2VLaGxFZ0ErQ1o1RTFNdWVXeFZGREl1Sm5BSFJyL3ZE?=
 =?utf-8?B?UDlGeVp4Q2dkMkF2WTlOeUd1Q0RIYkVucU5DRHAwcHE4MHJ5K3J4cDdCdGtT?=
 =?utf-8?B?N3dzdVdSUTEybEtXa0IxTFhTYUsvbXlrWWNYWUkrNGNXTzI0czVkN0YwUkps?=
 =?utf-8?B?bUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 029ee531-6d20-4720-bf4e-08dc54fd9a82
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:18:57.7112
 (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: uQGptaPkUOzYx83K2t80g5cJXvGsDtUTs1PEtOvoRjSneYoqoGD6Ie6k5Bux9k7L4VsLP0V7R0SPTWg8xLtUuaU7sB9C8K8cKpfWh2Q1FQk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6570
X-OriginatorOrg: intel.com

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> Iterating over all task_structs while read-locking the tasklist_lock
> results in significant task creation/destruction latency. Back-to-back
> move operations can thus be disastrous to the responsiveness of
> threadpool-based services.

Please be specific with claims.

> 
> Now that the CLOSID is determined indirectly through a reference to the
> task's current rdtgroup, it is not longer necessary to update the CLOSID
> in all tasks belonging to the moved mongroup. The context switch handler
> just needs to be prepared for concurrent writes to the parent pointer.

(insert text explanation how context switch handler is prepared for
concurrent writes)

> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 30 +++++++-------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index bd067f7ed5b6..a007c0ec478f 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -388,8 +388,11 @@ void __resctrl_sched_in(struct task_struct *tsk)
>  				 * by a full barrier and synchronous IPI
>  				 * broadcast before proceeding to free the
>  				 * group.
> +				 *
> +				 * parent can be concurrently updated to a new
> +				 * group as a result of mongrp_reparent().
>  				 */
> -				closid = rgrp->mon.parent->closid;
> +				closid = READ_ONCE(rgrp->mon.parent)->closid;
>  		} else {
>  			closid = rgrp->closid;
>  		}
> @@ -3809,8 +3812,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>   * Monitoring data for the group is unaffected by this operation.
>   */
>  static void mongrp_reparent(struct rdtgroup *rdtgrp,
> -			    struct rdtgroup *new_prdtgrp,
> -			    cpumask_var_t cpus)
> +			    struct rdtgroup *new_prdtgrp)
>  {
>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
>  
> @@ -3825,13 +3827,10 @@ static void mongrp_reparent(struct rdtgroup *rdtgrp,
>  	list_move_tail(&rdtgrp->mon.crdtgrp_list,
>  		       &new_prdtgrp->mon.crdtgrp_list);
>  
> -	rdtgrp->mon.parent = new_prdtgrp;
> +	WRITE_ONCE(rdtgrp->mon.parent, new_prdtgrp);
>  	rdtgrp->closid = new_prdtgrp->closid;
>  
> -	/* Propagate updated closid to all tasks in this group. */
> -	rdt_move_group_tasks(rdtgrp, rdtgrp, cpus);
> -
> -	update_closid_rmid(cpus, NULL);
> +	update_closid_rmid(cpu_online_mask, NULL);

This deserves a mention in changelog.

There is a section in the documentation, "Resource alloc and monitor groups"
that describes moving monitor groups. Unless you receive better ideas to address
the concern about this impact on CPU-isolated realtime workloads I would like
to suggest that you add a snippet there about the consequences of a move.
 
Reinette