Message-Id: <20240814110132.088d5acbd2df5dc5aeb63de3@kernel.org>
Date: Wed, 14 Aug 2024 11:01:32 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Luis Chamberlain
 <mcgrof@...nel.org>, linux-modules@...r.kernel.org,
 "Masami Hiramatsu (Google)" <mhiramat@...nel.org>, don <zds100@...il.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] tracing/fprobe: Support raw tracepoint events on
 modules

Hi,

Sorry I missed this thread. Thanks for your comments.

On Tue, 4 Jun 2024 14:03:05 -0400
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:

> On 2024-06-04 12:34, Steven Rostedt wrote:
> > On Tue, 4 Jun 2024 11:02:16 -0400
> > Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> > 
> >> I see.
> >>
> >> It looks like there are a few things we could improve there:
> >>
> >> 1) With your approach, modules need to be already loaded before
> >> attaching an fprobe event to them. This effectively prevents
> >> attaching to any module init code. Is there any way we could allow
> >> this by implementing a module coming notifier in fprobe as well ?
> >> This would require that fprobes are kept around in a data structure
> >> that matches the modules when they are loaded in the coming notifier.
> > 
> > The above sounds like a nice enhancement, but not something necessary for
> > this series.
> 
> IMHO it is nevertheless relevant to discuss the impact of supporting
> this kind of use-case on the ABI presented to userspace, at least to
> validate that what is exposed today can incrementally be enhanced
> towards that goal.
> 
> I'm not saying that it needs to be implemented today, but we should
> at least give it some thoughts right now to make sure the ABI is a
> good fit.

OK, let me try to update to handle module loading. I also need to add
a module which has a test tracepoint in init function.

> 
> >>
> >> 2) Given that the fprobe module going notifier is protected by the
> >> event_mutex, can we use locking rather than reference counting
> >> in fprobe attach to guarantee the target module is not reclaimed
> >> concurrently ? This would remove the transient side-effect of
> >> holding a module reference count which temporarily prevents module
> >> unload.

See trace_kprobe_module_callback()@kernel/trace/trace_kprobe.c. I think
we can filter the MODULE_STATE_COMING flag before locking event_mutex.
We anyway don't check the module is going because it would be a waste to
disarm the raw tracepoint events from the going module.

Thank you,

> > 
> > Why do we care about unloading modules during the transition? Note, module
> > unload has always been considered a second class citizen, and there's been
> > talks in the past to even rip it out.
> 
> As a general rule I try to ensure tracing has as little impact on the
> system behavior so issues that occur without tracing can be reproduced
> with instrumentation.
> 
> On systems where modules are loaded/unloaded with udev, holding
> references on modules can spuriously prevent module unload, which
> as a consequence changes the system behavior.
> 
> About the relative importance of the various kernel subsystems,
> following your reasoning that module unload is considered a
> second-class citizen within the kernel, I would argue that tracing
> is a third-class citizen and should not needlessly modify the
> behavior of classes above it.
> 
> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists