Received-SPF: pass (google.com: domain of linux-kernel+bounces-200911-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <419b80da-9cbf-4bb2-aabb-dc04f0fb0f37@efficios.com>
Date: Tue, 4 Jun 2024 11:02:16 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/3] tracing/fprobe: Support raw tracepoint events on
 modules
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>, don <zds100@gmail.com>,
 linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
References: <171723014778.258703.6731294779199848686.stgit@devnote2>
 <171723016594.258703.1629777910752596529.stgit@devnote2>
 <fbfec8d9-d0ed-4384-bbd2-dd5c1e568ed1@efficios.com>
 <20240604084955.29b9440687522a1347e0e7cd@kernel.org>
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Content-Language: en-US
In-Reply-To: <20240604084955.29b9440687522a1347e0e7cd@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024-06-03 19:49, Masami Hiramatsu (Google) wrote:
> On Mon, 3 Jun 2024 15:50:55 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>> On 2024-06-01 04:22, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>>
>>> Support raw tracepoint event on module by fprobe events.
>>> Since it only uses for_each_kernel_tracepoint() to find a tracepoint,
>>> the tracepoints on modules are not handled. Thus if user specified a
>>> tracepoint on a module, it shows an error.
>>> This adds new for_each_module_tracepoint() API to tracepoint subsystem,
>>> and uses it to find tracepoints on modules.
>>
>> Hi Masami,
>>
>> Why prevent module unload when a fprobe tracepoint is attached to a
>> module ? This changes the kernel's behavior significantly just for the
>> sake of instrumentation.
> 
> I don't prevent module unloading all the time, just before registering
> tracepoint handler (something like booking a ticket :-) ).
> See the last hunk of this patch, it puts the module before exiting
> __trace_fprobe_create().

So at least this is transient, but I still have concerns, see below.

>>
>> As an alternative, LTTng-modules attach/detach to/from modules with the
>> coming/going notifiers, so the instrumentation gets removed when a
>> module is unloaded rather than preventing its unload by holding a module
>> reference count. I would recommend a similar approach for fprobe.
> 
> Yes, since tracepoint subsystem provides a notifier API to notify the
> tracepoint is gone, fprobe already uses it to find unloading and
> unregister the target function. (see __tracepoint_probe_module_cb)

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.

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.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com