From: Wen Yang <wenyang.linux@foxmail.com>

Currently coredump_task_exit() takes some time to wait for the generation
of the dump file. But if the user-space wants to receive a notification
as soon as possible it maybe inconvenient.

Move trace_sched_process_exit() earlier in do_exit().
This way a user-space monitor could detect the exits and
potentially make some preparations in advance.

Oleg initially proposed this suggestion, and Steven further provided some
detailed suggestions, and Mathieu carefully checked the historical code
and said:
: I've checked with Matthew Khouzam (maintainer of Trace Compass)
: which care about this tracepoint, and we have not identified any
: significant impact of moving it on its model of the scheduler, other
: than slightly changing its timing.
: I've also checked quickly in lttng-analyses and have not found
: any code that care about its specific placement.
: So I would say go ahead and move it earlier in do_exit(), it's
: fine by me.

Suggested-by: Oleg Nesterov <oleg@redhat.com>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 493647fd7c07..2cff6533cb39 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -826,6 +826,7 @@ void __noreturn do_exit(long code)
 
 	WARN_ON(tsk->plug);
 
+	trace_sched_process_exit(tsk);
 	kcov_task_exit(tsk);
 	kmsan_task_exit(tsk);
 
@@ -866,7 +867,6 @@ void __noreturn do_exit(long code)
 
 	if (group_dead)
 		acct_process();
-	trace_sched_process_exit(tsk);
 
 	exit_sem(tsk);
 	exit_shm(tsk);
-- 
2.25.1

On Sun, 10 Mar 2024 13:25:29 +0800 wenyang.linux@foxmail.com wrote:

> From: Wen Yang <wenyang.linux@foxmail.com>
> 
> Currently coredump_task_exit() takes some time to wait for the generation
> of the dump file. But if the user-space wants to receive a notification
> as soon as possible it maybe inconvenient.

If userspace is awaiting this notification to say "it's now OK to read
the dump file" then it could break things?

> Move trace_sched_process_exit() earlier in do_exit().
> This way a user-space monitor could detect the exits and
> potentially make some preparations in advance.
> 
> Oleg initially proposed this suggestion, and Steven further provided some
> detailed suggestions, and Mathieu carefully checked the historical code
> and said:
> : I've checked with Matthew Khouzam (maintainer of Trace Compass)
> : which care about this tracepoint, and we have not identified any
> : significant impact of moving it on its model of the scheduler, other
> : than slightly changing its timing.
> : I've also checked quickly in lttng-analyses and have not found
> : any code that care about its specific placement.
> : So I would say go ahead and move it earlier in do_exit(), it's
> : fine by me.

I'm not seeing a clear need for this change.  "maybe inconveniant" is
quite thin.  Please fully describe what motivated you to work on this?

On 2024/3/20 05:57, Andrew Morton wrote:
> On Sun, 10 Mar 2024 13:25:29 +0800 wenyang.linux@foxmail.com wrote:
> 
>> From: Wen Yang <wenyang.linux@foxmail.com>
>>
>> Currently coredump_task_exit() takes some time to wait for the generation
>> of the dump file. But if the user-space wants to receive a notification
>> as soon as possible it maybe inconvenient.
> 
> If userspace is awaiting this notification to say "it's now OK to read
> the dump file" then it could break things?
> 

Thanks for your comments.

This could also be achieved in the nearby proc_exit_connector().

In addition, this issue has been discussed in detail in the following link:
https://lkml.org/lkml/2024/2/23/1018


>> Move trace_sched_process_exit() earlier in do_exit().
>> This way a user-space monitor could detect the exits and
>> potentially make some preparations in advance.
>>
>> Oleg initially proposed this suggestion, and Steven further provided some
>> detailed suggestions, and Mathieu carefully checked the historical code
>> and said:
>> : I've checked with Matthew Khouzam (maintainer of Trace Compass)
>> : which care about this tracepoint, and we have not identified any
>> : significant impact of moving it on its model of the scheduler, other
>> : than slightly changing its timing.
>> : I've also checked quickly in lttng-analyses and have not found
>> : any code that care about its specific placement.
>> : So I would say go ahead and move it earlier in do_exit(), it's
>> : fine by me.
> 
> I'm not seeing a clear need for this change.  "maybe inconveniant" is
> quite thin.  Please fully describe what motivated you to work on this?
> 

Thanks.
We hope to address this issue in our Advanced Driver Assistance System:
When certain critical processes exit abnormally, prompt information can 
be reported to the monitoring program as soon as possible.

We have also attempted to add a new TP, as follows:
https://lkml.org/lkml/2024/2/23/5

But after some discussion, it is still considered more reasonable to 
move trace_sched_process_exit() earlier.

We also look forward to your suggestions, thanks again.


--
Best wishes,
Wen

On Sun, 10 Mar 2024 13:25:29 +0800
wenyang.linux@foxmail.com wrote:

> From: Wen Yang <wenyang.linux@foxmail.com>
> 
> Currently coredump_task_exit() takes some time to wait for the generation
> of the dump file. But if the user-space wants to receive a notification
> as soon as possible it maybe inconvenient.
> 
> Move trace_sched_process_exit() earlier in do_exit().
> This way a user-space monitor could detect the exits and
> potentially make some preparations in advance.
> 
> Oleg initially proposed this suggestion, and Steven further provided some
> detailed suggestions, and Mathieu carefully checked the historical code
> and said:
> : I've checked with Matthew Khouzam (maintainer of Trace Compass)
> : which care about this tracepoint, and we have not identified any
> : significant impact of moving it on its model of the scheduler, other
> : than slightly changing its timing.
> : I've also checked quickly in lttng-analyses and have not found
> : any code that care about its specific placement.
> : So I would say go ahead and move it earlier in do_exit(), it's
> : fine by me.
> 

I'm putting together last minute minor patches for this merge window. I can
take this if nobody has any objections.

-- Steve


> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/exit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 493647fd7c07..2cff6533cb39 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -826,6 +826,7 @@ void __noreturn do_exit(long code)
>  
>  	WARN_ON(tsk->plug);
>  
> +	trace_sched_process_exit(tsk);
>  	kcov_task_exit(tsk);
>  	kmsan_task_exit(tsk);
>  
> @@ -866,7 +867,6 @@ void __noreturn do_exit(long code)
>  
>  	if (group_dead)
>  		acct_process();
> -	trace_sched_process_exit(tsk);
>  
>  	exit_sem(tsk);
>  	exit_shm(tsk);