Received-SPF: pass (google.com: domain of linux-kernel+bounces-71755-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 19 Feb 2024 12:28:25 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: wenyang.linux@foxmail.com, Masami Hiramatsu <mhiramat@kernel.org>, Ingo
 Molnar <mingo@kernel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@efficios.com>, Mel Gorman <mgorman@techsingularity.net>,
 Peter Zijlstra <peterz@infradead.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] coredump debugging: add a tracepoint to report the
 coredumping
Message-ID: <20240219122825.31579a1e@gandalf.local.home>
In-Reply-To: <20240219170038.GA710@redhat.com>
References: <tencent_5CD40341EC9384E9B7CC127EA5CF2655B408@qq.com>
	<20240217104924.GB10393@redhat.com>
	<20240219112926.77ac16f8@gandalf.local.home>
	<20240219170038.GA710@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 19 Feb 2024 18:00:38 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> > void __noreturn do_exit(long code)
> > {
> > 	struct task_struct *tsk = current;
> > 	int group_dead;
> >
> > [...]
> > 	acct_collect(code, group_dead);
> > 	if (group_dead)
> > 		tty_audit_exit();
> > 	audit_free(tsk);
> >
> > 	tsk->exit_code = code;
> > 	taskstats_exit(tsk, group_dead);
> >
> > 	exit_mm();
> >
> > 	if (group_dead)
> > 		acct_process();
> > 	trace_sched_process_exit(tsk);
> >
> > There's a lot that happens before we trigger the above event.  
> 
> and a lot after.

True. There really isn't a meaningful location here is there?

I actually use this tracepoint in my pid tracing.

The set_ftrace_pid and set_event_pid from /sys/kernel/tracing will add and
remove PIDs if the options function-fork or event-fork are set respectively.

I hook to the sched_process_fork tracepoint to add new PIDs if the parent
pid is already in one of the files, and remove a PID via the
sched_process_exit function.

Honestly, if anything, it should probably be moved down right next to
perf_event_exit_task() (I never understood why perf needed its own hooks
and not just use tracepoints).

> 
> To me the current placement of trace_sched_process_exit() looks absolutely
> random.

Agreed.

> 
> > I could
> > imagine that there are users expecting those actions to have taken place by
> > the time the event triggered. Like the "exit_mm()" call, as well as many
> > others.
> >
> > I would be leery of moving that tracepoint.  
> 
> And I agree. I am always scared of every user-visible change, simply
> because it is user-visbible.
> 
> If it was not clear, I didn't try to nack this patch. I simply do not know
> how people use the tracepoints and for what. Apart from debugging.
> 
> But if we add the new one into coredump_task_exit(), then we probably want
> another one in ptrace_event(PTRACE_EVENT_EXIT) ? It too can "take some time"
> before the exiting task actually exits.
> 
> So I think this needs some discussion, and the changelog should probably say
> more.
> 
> In short: I am glad you are here, I leave this to you and Wen ;)

I still would like to have your input too ;-)

-- Steve