Re: [PATCH v2 2/3] tracing/fprobe: Support raw tracepoint events on modules
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2024-06-04 15:12:52
Also in:
lkml
On Tue, 4 Jun 2024 08:49:55 +0900 Masami Hiramatsu (Google) [off-list ref] wrote:
On Mon, 3 Jun 2024 15:50:55 -0400 Mathieu Desnoyers [off-list ref] wrote:quoted
On 2024-06-01 04:22, Masami Hiramatsu (Google) wrote:quoted
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().quoted
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)
Ah, it only prevents module unloading in __trace_fprobe_create()
+static void __find_tracepoint_module_cb(struct tracepoint *tp, void *priv)
+{
+ struct __find_tracepoint_cb_data *data = priv;
+
+ if (!data->tpoint && !strcmp(data->tp_name, tp->name)) {
+ data->tpoint = tp;
+ data->mod = __module_text_address((unsigned long)tp->probestub);
+ if (!try_module_get(data->mod)) {Here it gets the module. Should only happen once, as it sets data->tpoint.
quoted hunk ↗ jump to hunk
+ data->tpoint = NULL; + data->mod = NULL; + } + } +} + static void __find_tracepoint_cb(struct tracepoint *tp, void *priv) { struct __find_tracepoint_cb_data *data = priv;@@ -905,14 +922,28 @@ static void __find_tracepoint_cb(struct tracepoint *tp, void *priv) data->tpoint = tp; } -static struct tracepoint *find_tracepoint(const char *tp_name) +/* + * Find a tracepoint from kernel and module. If the tracepoint is in a module, + * this increments the module refcount to prevent unloading until the + * trace_fprobe is registered to the list. After registering the trace_fprobe + * on the trace_fprobe list, the module refcount is decremented because + * tracepoint_probe_module_cb will handle it. + */ +static struct tracepoint *find_tracepoint(const char *tp_name, + struct module **tp_mod) { struct __find_tracepoint_cb_data data = { .tp_name = tp_name, + .mod = NULL, }; for_each_kernel_tracepoint(__find_tracepoint_cb, &data); + if (!data.tpoint && IS_ENABLED(CONFIG_MODULES)) { + for_each_module_tracepoint(__find_tracepoint_module_cb, &data); + *tp_mod = data.mod; + } + return data.tpoint; }@@ -996,6 +1027,7 @@ static int __trace_fprobe_create(int argc, const char *argv[]) char abuf[MAX_BTF_ARGS_LEN]; char *dbuf = NULL; bool is_tracepoint = false; + struct module *tp_mod = NULL; struct tracepoint *tpoint = NULL; struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL | TPARG_FL_FPROBE,@@ -1080,7 +1112,7 @@ static int __trace_fprobe_create(int argc, const char *argv[]) if (is_tracepoint) { ctx.flags |= TPARG_FL_TPOINT; - tpoint = find_tracepoint(symbol); + tpoint = find_tracepoint(symbol, &tp_mod); if (!tpoint) { trace_probe_log_set_index(1); trace_probe_log_err(0, NO_TRACEPOINT);@@ -1110,8 +1142,8 @@ static int __trace_fprobe_create(int argc, const char *argv[]) goto out; /* setup a probe */ - tf = alloc_trace_fprobe(group, event, symbol, tpoint, maxactive, - argc, is_return); + tf = alloc_trace_fprobe(group, event, symbol, tpoint, tp_mod, + maxactive, argc, is_return); if (IS_ERR(tf)) { ret = PTR_ERR(tf); /* This must return -ENOMEM, else there is a bug */@@ -1119,10 +1151,6 @@ static int __trace_fprobe_create(int argc, const char *argv[]) goto out; /* We know tf is not allocated */ } - if (is_tracepoint) - tf->mod = __module_text_address( - (unsigned long)tf->tpoint->probestub); - /* parse arguments */ for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) { trace_probe_log_set_index(i + 2);@@ -1155,6 +1183,8 @@ static int __trace_fprobe_create(int argc, const char *argv[]) } out: + if (tp_mod) + module_put(tp_mod);
And on the way out, it puts it. -- Steve
traceprobe_finish_parse(&ctx); trace_probe_log_clear(); kfree(new_argv);