Received-SPF: pass (google.com: domain of linux-kernel+bounces-132238-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: <50298f2b-45dc-4b22-9383-b82791ee319d@intel.com>
Date: Thu, 4 Apr 2024 16:10:22 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 2/6] x86/resctrl: Add hook for releasing task_struct
 references
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-3-peternewman@google.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240325172707.73966-3-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?NGxFQVFKdXovN1BxZktpblhvWEJxajRnU3R6d1krWit0NlIzYlJhV0xlbml0?=
 =?utf-8?B?Nm9XT2M4ZVBSNHNGRjN6Kzg5SER0WVh1bzRlNStwa09tVW9mUFh4SW9LbGtx?=
 =?utf-8?B?ZGQzM3VVTnpxblRHMXY5dnU4cHRvTXhOT2hzazFhMkdMQUYzWmkrdEJySHNR?=
 =?utf-8?B?cnNFN3NoTWNPeFZRV0tnZ1VYY29wY05FL1Mva0VxL1dGbUNGNjFKUmd2MzlH?=
 =?utf-8?B?d3BRcERobTNESVcwMUJObmhvZFNvbVVWdFZFb3ZMZmYzdUYrZkI2cndtVGRE?=
 =?utf-8?B?T1pSNmJHQ3pwZmhyalY5bXVIaGlWUjNvdmIrNW0rMFdXeWQ0eHhsWE9JUTZs?=
 =?utf-8?B?MnlTUzFXOUFQcEY3NnJxMHhCUjY0Z2JTQUl5OVZnNWIyaHBOcHQ0bFFSczZN?=
 =?utf-8?B?Nk9CV2lQOHVpQndtNUZCd09XVmJWT2w2WU5VMnhxbXZNb2JNb29RbGg4RDVp?=
 =?utf-8?B?UTc1dTMxNTRBWXBmRFdkWkduM2FSTVN5My9mN2IvdjlOSmY2VkpqUXRhRTMx?=
 =?utf-8?B?cFRaamRnQ1JvS0hKeWkxcTk1RmpaK3ErSjlub1Q0YjVRb0RtM1RsWHhPNG1i?=
 =?utf-8?B?anNsWGpWSCtvRFBuckkzN09taUlwakRzSWVaa0hJSHIxWXZVQ3UxcGY0RHFE?=
 =?utf-8?B?NkdHZ0RucnlNOUVPRjBpaDVzOXJTNzAwWnc3RkR4Zk9HMjRabytnK3puM0Vh?=
 =?utf-8?B?czVFR042dURzaUpyenRWTXB3ODVBNVNpdm1hMnRTbzk2SnVwODJKNndPd3Y4?=
 =?utf-8?B?d0pSeXM4UHdCTXVKZ2FEbzk4QmcwQ3QrYUhwNklKMTZSdVNuOVZsRnZYMzVP?=
 =?utf-8?B?WHFSRHB6YURoVVF3QlB4ZU5XclIrN2xXQm5ZRFFjV3VjbWhCYjZkUE1PMUZY?=
 =?utf-8?B?dGY2ZjRIQkJiSzFod2p5TEUzT0hWQTBxU3B6OGxENDdQR0F1QlVMMDZibG5s?=
 =?utf-8?B?TWNReWpEUGxTTmUzSXNnMFZGMWhTMWxIZnFJRWFTTFdVYjV6Rk4wYVdMbGdp?=
 =?utf-8?B?NlMrL2k1U1Nadjc5UVFtejQvS245Y1dHd2FxM3ZuUWFYTzMvQmpRdi95eUQr?=
 =?utf-8?B?bHpXRWJWVVAyTXZIM0VZVU1qdGZWcWRWYXZ5K256U090dWVmZGt4REo1clJH?=
 =?utf-8?B?QnZFZHZYL2U0bEJpNzdoM1FSV1BSdG11VVRxMm9RUHVnTDRGd0FFZGdQRFFD?=
 =?utf-8?B?dkMyWHlJeVNQWEpZMFd1SE0wcmJydXVoLzhWTjYxUXQyUlc0Y0NkTUhhZktB?=
 =?utf-8?B?OHh4MHROdVBWUFJ3Y3YyQnVkbjlRdGhqR05rQWo3ZWIvNHFydUwyNFNyREYx?=
 =?utf-8?B?QVZ5MW9FZnVKWXBzNHdyZHFOVDdFaU5heXVreXJ5YytmQW1qZ1Baem9WSk04?=
 =?utf-8?B?T2ZHQ240cmpTZFlndHpOeWlSc1F0VmJZTEhNOUpOekU3U1pCa2ZZOW5wSVBQ?=
 =?utf-8?B?eUpWT1Z3Skk3MTFKeTJpQlB2VVhEQkRSR0xoRk9tcFp1MWhpdXJvQzc1cHBr?=
 =?utf-8?B?UldhMENNZ05HdUJ0SG1YYzNYSk1wK0lEb2M4Z3BtU2J3NWNoVzRiaWpKUEky?=
 =?utf-8?B?N0tjMGFzaHZGamhNY1NYbWlYVWJaaW9TWllpcStBYlV4SmNaNm5INEIrZHBs?=
 =?utf-8?B?UGZYKy91aFBZQVcxai81ZVM0QTJyMksxekpZSmJOTUU1RmhsbzQ0UnpZSU44?=
 =?utf-8?B?WnhaV2x2N0FYaFE5RHZhc0Nsa1lmWlZRS3EwT0lpd29BYW9reldEaGhSTk5Y?=
 =?utf-8?B?eklnTFRlNnlzWWRXbnhmbXFCek1ZenhpdjJzY1Qyek5EUjVEaDEzVGxMeHdm?=
 =?utf-8?B?VGNrZnFwR093ZFpyQU1GeE80eGJXNGhVTERoR28yd0dVbGFyT1llQTBmR05p?=
 =?utf-8?B?cHZwVUlXd0poYkFWVUhQcUVzeEZuYmdmSHBDazR0d0FhVUpycndXdEY1UC82?=
 =?utf-8?B?VDNVZFRGTzErOXBndVJkYnk1QjVDKzFmSlJCS0RDL1UyL0p1Z1FjQXFlU0dI?=
 =?utf-8?B?T1lid0V2ZE1yK2E5QmZhMkFWREM1UWRjbjRxL1NJcHFnak15NHFIV3BGOHJM?=
 =?utf-8?B?dTFOSzU3ckpKb0VnQWMvdWRXVnhucnRjQXJRM0dtbU1yYmFkZ3lURWtMNHlB?=
 =?utf-8?B?ekdsc0ZJZzh3dXk3aWVpdngxcDdCblJDNGZNbG1zcjNuTmxtclRIYnM4V29T?=
 =?utf-8?B?N0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f72d1c3-a76d-4c75-aaac-08dc54fc6955
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:10:25.7183
 (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: izs0i6C2zSYuDSpQ1b5j65SpOgAcgsVsUKO833wOIvzpVAFtiCvpjkRev73KKYti6WTxSvLe10QGnM7HSk0lXw1Gq6tDX5ClkYjdUtt0hYQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4847
X-OriginatorOrg: intel.com

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> In order for the task_struct to hold references to rdtgroups, it must be
> possible to release these references before a concurrent deletion causes
> them to be freed.

This is very hard to parse (for me). I found your description in the
cover letter to be much easier to understand.
Considering the core area of code changed with this patch the
changelog needs to be specific on what is needed and why.

I'd also like to recommend that the subject prefix is changed to "exit: ..."
to highlight to folks that this crosses into a different area of the kernel.

It is not obvious to me how changes are routed to this exit code but we
can start by highlighting this to not appear to want to sneak it in.

> 
> It is not possible for resctrl code to do this with
> for_each_process_thread() because the task can still switch in after it
> has been removed from the tasklist, at which point the task_struct could
> be referring to freed memory.
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 ++++++++++
>  include/linux/resctrl.h                |  6 ++++++
>  kernel/exit.c                          |  3 +++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5d599d99f94b..9b1969e4235a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2931,6 +2931,16 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
>  	read_unlock(&tasklist_lock);
>  }
>  
> +/**
> + * exit_resctrl() - called at thread destruction to release resources
> + *
> + * This hook is called just before the task is removed from the global tasklist
> + * and still reachable via for_each_process_thread().
> + */
> +void exit_resctrl(struct task_struct *tsk)
> +{
> +}
> +
>  static void free_all_child_rdtgrp(struct rdtgroup *rdtgrp)
>  {
>  	struct rdtgroup *sentry, *stmp;
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 62d607939a73..b2af1fbc7aa1 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -325,4 +325,10 @@ static inline void resctrl_sched_in(struct task_struct *tsk)
>  #endif
>  }
>  
> +#ifdef CONFIG_X86_CPU_RESCTRL
> +void exit_resctrl(struct task_struct *tsk);
> +#else
> +static inline void exit_resctrl(struct task_struct *tsk) {}
> +#endif

Scattering these ifdefs in the header file is not ideal. I think when there
are just the two sections as I mentioned in previous patch this will look better.

> +
>  #endif /* _RESCTRL_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 41a12630cbbc..ccdc90ff6d71 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -70,6 +70,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/user_events.h>
>  #include <linux/uaccess.h>
> +#include <linux/resctrl.h>
>  
>  #include <uapi/linux/wait.h>
>  
> @@ -862,6 +863,8 @@ void __noreturn do_exit(long code)
>  	tsk->exit_code = code;
>  	taskstats_exit(tsk, group_dead);
>  
> +	exit_resctrl(tsk);
> +

This seems fair but I am not familiar with the customs in this area of the kernel.
I see both exit_xxx() and xxx_task_exit() being used.

Reinette