* [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name @ 2018-03-26 19:10 Mathieu Desnoyers 2018-03-26 20:48 ` Alexei Starovoitov 2018-03-27 1:35 ` Joel Fernandes (Google) 0 siblings, 2 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2018-03-26 19:10 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Mathieu Desnoyers, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar Provide an API allowing eBPF to lookup core kernel tracepoints by name. Given that a lookup by name explicitly requires tracepoint definitions to be unique for a given name (no duplicate keys), include a WARN_ON_ONCE() check that only a single match is encountered at runtime. This should always be the case, given that a DEFINE_TRACE emits a __tracepoint_##name symbol, which would cause a link-time error if more than one instance is found. Nevertheless, check this at runtime with WARN_ON_ONCE() to stay on the safe side. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: Steven Rostedt <rostedt@goodmis.org> CC: Alexei Starovoitov <ast@kernel.org> CC: Peter Zijlstra <peterz@infradead.org> CC: Ingo Molnar <mingo@kernel.org> --- include/linux/tracepoint.h | 1 + kernel/tracepoint.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index c94f466d57ef..1b4ae64b7c6a 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -43,6 +43,7 @@ tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data); extern void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), void *priv); +extern struct tracepoint *tracepoint_kernel_find_by_name(const char *name); #ifdef CONFIG_MODULES struct tp_module { diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 671b13457387..0a59f988055a 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -60,6 +60,11 @@ struct tp_probes { struct tracepoint_func probes[0]; }; +struct tp_find_args { + const char *name; + struct tracepoint *tp; +}; + static inline void *allocate_probes(int count) { struct tp_probes *p = kmalloc(count * sizeof(struct tracepoint_func) @@ -528,6 +533,36 @@ void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), } EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint); +static void find_tp(struct tracepoint *tp, void *priv) +{ + struct tp_find_args *args = priv; + + if (!strcmp(tp->name, args->name)) { + WARN_ON_ONCE(args->tp); + args->tp = tp; + } +} + +/** + * tracepoint_kernel_find_by_name - lookup a core kernel tracepoint by name + * @name: tracepoint name + * + * Returns the tracepoint structure associated with the name received as + * parameter, or NULL if not found. Lookup is only performed throughout + * core kernel tracepoints. + */ +struct tracepoint *tracepoint_kernel_find_by_name(const char *name) +{ + struct tp_find_args args = { + .name = name, + .tp = NULL, + }; + + for_each_tracepoint_range(__start___tracepoints_ptrs, + __stop___tracepoints_ptrs, find_tp, &args); + return args.tp; +} + #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS /* NB: reg/unreg are called while guarded with the tracepoints_mutex */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-26 19:10 [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name Mathieu Desnoyers @ 2018-03-26 20:48 ` Alexei Starovoitov 2018-03-26 21:05 ` Steven Rostedt 2018-03-27 1:35 ` Joel Fernandes (Google) 1 sibling, 1 reply; 7+ messages in thread From: Alexei Starovoitov @ 2018-03-26 20:48 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Steven Rostedt, linux-kernel, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar On Mon, Mar 26, 2018 at 03:10:31PM -0400, Mathieu Desnoyers wrote: > Provide an API allowing eBPF to lookup core kernel tracepoints by name. > > Given that a lookup by name explicitly requires tracepoint definitions > to be unique for a given name (no duplicate keys), include a > WARN_ON_ONCE() check that only a single match is encountered at runtime. > This should always be the case, given that a DEFINE_TRACE emits a > __tracepoint_##name symbol, which would cause a link-time error if more > than one instance is found. Nevertheless, check this at runtime with > WARN_ON_ONCE() to stay on the safe side. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Alexei Starovoitov <ast@kernel.org> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Ingo Molnar <mingo@kernel.org> Mathieu, that's not enough. Commit log is also wrong. It needs to state that something like this is needed only because changing for_each_tracepoint_range() semantics will break lttng. I'll post a follow up patch shortly. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-26 20:48 ` Alexei Starovoitov @ 2018-03-26 21:05 ` Steven Rostedt 0 siblings, 0 replies; 7+ messages in thread From: Steven Rostedt @ 2018-03-26 21:05 UTC (permalink / raw) To: Alexei Starovoitov Cc: Mathieu Desnoyers, linux-kernel, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar On Mon, 26 Mar 2018 13:48:36 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > Mathieu, that's not enough. > Commit log is also wrong. > It needs to state that something like this is needed only because > changing for_each_tracepoint_range() semantics will break lttng. > I'll post a follow up patch shortly. No it does not. This is needed because I find it cleaner. If you don't want to pull this into your patch set. I'll pull it into mine. -- Steve ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-26 19:10 [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name Mathieu Desnoyers 2018-03-26 20:48 ` Alexei Starovoitov @ 2018-03-27 1:35 ` Joel Fernandes (Google) 2018-03-27 13:27 ` Mathieu Desnoyers 1 sibling, 1 reply; 7+ messages in thread From: Joel Fernandes (Google) @ 2018-03-27 1:35 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Steven Rostedt, Linux Kernel Mailing List, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar Hi Mathieu, On Mon, Mar 26, 2018 at 12:10 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > Provide an API allowing eBPF to lookup core kernel tracepoints by name. > > Given that a lookup by name explicitly requires tracepoint definitions > to be unique for a given name (no duplicate keys), include a > WARN_ON_ONCE() check that only a single match is encountered at runtime. > This should always be the case, given that a DEFINE_TRACE emits a > __tracepoint_##name symbol, which would cause a link-time error if more > than one instance is found. Nevertheless, check this at runtime with > WARN_ON_ONCE() to stay on the safe side. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Alexei Starovoitov <ast@kernel.org> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Ingo Molnar <mingo@kernel.org> > --- > include/linux/tracepoint.h | 1 + > kernel/tracepoint.c | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index c94f466d57ef..1b4ae64b7c6a 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -43,6 +43,7 @@ tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data); > extern void > for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), > void *priv); > +extern struct tracepoint *tracepoint_kernel_find_by_name(const char *name); > > #ifdef CONFIG_MODULES > struct tp_module { > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 671b13457387..0a59f988055a 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -60,6 +60,11 @@ struct tp_probes { > struct tracepoint_func probes[0]; > }; > > +struct tp_find_args { > + const char *name; > + struct tracepoint *tp; > +}; > + > static inline void *allocate_probes(int count) > { > struct tp_probes *p = kmalloc(count * sizeof(struct tracepoint_func) > @@ -528,6 +533,36 @@ void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), > } > EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint); > > +static void find_tp(struct tracepoint *tp, void *priv) > +{ > + struct tp_find_args *args = priv; > + > + if (!strcmp(tp->name, args->name)) { > + WARN_ON_ONCE(args->tp); > + args->tp = tp; I think this runtime check is not needed if it really can't happen (linker verifies that already as you mentioned) although I'm not opposed if you still want to keep it, because there's no way to break out of the outer loop anyway so I guess your call.. I would just do: if (args->tp) return; if find_tp already found the tracepoint. Tried to also create a duplicate tracepoint and I get: CC init/version.o AR init/built-in.o AR built-in.o LD vmlinux.o block/blk-core.o:(__tracepoints+0x440): multiple definition of `__tracepoint_sched_switch' kernel/sched/core.o:(__tracepoints+0x440): first defined here Makefile:1032: recipe for target 'vmlinux' failed make: *** [vmlinux] Error 1 For this senseless diff: diff --git a/include/trace/events/block.h b/include/trace/events/block.h index 81b43f5bdf23..2b855c05197d 100644 --- a/include/trace/events/block.h +++ b/include/trace/events/block.h @@ -49,6 +49,13 @@ DEFINE_EVENT(block_buffer, block_touch_buffer, TP_ARGS(bh) ); +DEFINE_EVENT(block_buffer, sched_switch, + + TP_PROTO(struct buffer_head *bh), + + TP_ARGS(bh) +); + /** * block_dirty_buffer - mark a buffer dirty * @bh: buffer_head being dirtied thanks, - Joel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-27 1:35 ` Joel Fernandes (Google) @ 2018-03-27 13:27 ` Mathieu Desnoyers 2018-03-27 15:28 ` Joel Fernandes (Google) 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Desnoyers @ 2018-03-27 13:27 UTC (permalink / raw) To: joel opensrc Cc: rostedt, linux-kernel, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar ----- On Mar 26, 2018, at 9:35 PM, joel opensrc joel.opensrc@gmail.com wrote: > Hi Mathieu, Hi Joel, > > On Mon, Mar 26, 2018 at 12:10 PM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: >> Provide an API allowing eBPF to lookup core kernel tracepoints by name. >> >> Given that a lookup by name explicitly requires tracepoint definitions >> to be unique for a given name (no duplicate keys), include a >> WARN_ON_ONCE() check that only a single match is encountered at runtime. >> This should always be the case, given that a DEFINE_TRACE emits a >> __tracepoint_##name symbol, which would cause a link-time error if more >> than one instance is found. Nevertheless, check this at runtime with >> WARN_ON_ONCE() to stay on the safe side. >> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >> CC: Steven Rostedt <rostedt@goodmis.org> >> CC: Alexei Starovoitov <ast@kernel.org> >> CC: Peter Zijlstra <peterz@infradead.org> >> CC: Ingo Molnar <mingo@kernel.org> >> --- >> include/linux/tracepoint.h | 1 + >> kernel/tracepoint.c | 35 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h >> index c94f466d57ef..1b4ae64b7c6a 100644 >> --- a/include/linux/tracepoint.h >> +++ b/include/linux/tracepoint.h >> @@ -43,6 +43,7 @@ tracepoint_probe_unregister(struct tracepoint *tp, void >> *probe, void *data); >> extern void >> for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), >> void *priv); >> +extern struct tracepoint *tracepoint_kernel_find_by_name(const char *name); >> >> #ifdef CONFIG_MODULES >> struct tp_module { >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >> index 671b13457387..0a59f988055a 100644 >> --- a/kernel/tracepoint.c >> +++ b/kernel/tracepoint.c >> @@ -60,6 +60,11 @@ struct tp_probes { >> struct tracepoint_func probes[0]; >> }; >> >> +struct tp_find_args { >> + const char *name; >> + struct tracepoint *tp; >> +}; >> + >> static inline void *allocate_probes(int count) >> { >> struct tp_probes *p = kmalloc(count * sizeof(struct tracepoint_func) >> @@ -528,6 +533,36 @@ void for_each_kernel_tracepoint(void (*fct)(struct >> tracepoint *tp, void *priv), >> } >> EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint); >> >> +static void find_tp(struct tracepoint *tp, void *priv) >> +{ >> + struct tp_find_args *args = priv; >> + >> + if (!strcmp(tp->name, args->name)) { >> + WARN_ON_ONCE(args->tp); >> + args->tp = tp; > > I think this runtime check is not needed if it really can't happen > (linker verifies that already as you mentioned) although I'm not > opposed if you still want to keep it, because there's no way to break > out of the outer loop anyway so I guess your call.. We can change the outer loop and break from it if needed, that's not an issue. > I would just do: > > if (args->tp) > return; > > if find_tp already found the tracepoint. > > Tried to also create a duplicate tracepoint and I get: > CC init/version.o > AR init/built-in.o > AR built-in.o > LD vmlinux.o > block/blk-core.o:(__tracepoints+0x440): multiple definition of > `__tracepoint_sched_switch' > kernel/sched/core.o:(__tracepoints+0x440): first defined here > Makefile:1032: recipe for target 'vmlinux' failed > make: *** [vmlinux] Error 1 Yeah, as I state in my changelog, I'm very well aware that the linker is able to catch those. This was the purpose of emitting a __tracepoint_##name symbol in the tracepoint definition macro. This WARN_ON_ONCE() is a redundant check for an invariant that we expect is provided by the linker. Given that it's not a fast path, I would prefer to keep this redundant check in place, given that an average factor 2 speedup on a slow path should really not be something we need to optimize for. IMHO, for slow paths, robustness is more important than speed (unless the slow path becomes so slow that it really affects the user). I envision that a way to break this invariant would be to compile a kernel object with gcc -fvisibility=hidden or such. I admit this is particular scenario is really far fetched, but it illustrates why I prefer to keep an extra safety net at runtime for linker-based validations when possible. If a faster tracepoint lookup function is needed, we should consider perfect hashing algorithms done post-build, but so far nobody has complained about speed of this lookup operation. Anyhow a factor 2 in the speed of this lookup should really not matter, right ? Thanks, Mathieu > > For this senseless diff: > diff --git a/include/trace/events/block.h b/include/trace/events/block.h > index 81b43f5bdf23..2b855c05197d 100644 > --- a/include/trace/events/block.h > +++ b/include/trace/events/block.h > @@ -49,6 +49,13 @@ DEFINE_EVENT(block_buffer, block_touch_buffer, > TP_ARGS(bh) > ); > > +DEFINE_EVENT(block_buffer, sched_switch, > + > + TP_PROTO(struct buffer_head *bh), > + > + TP_ARGS(bh) > +); > + > /** > * block_dirty_buffer - mark a buffer dirty > * @bh: buffer_head being dirtied > > > thanks, > > - Joel -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-27 13:27 ` Mathieu Desnoyers @ 2018-03-27 15:28 ` Joel Fernandes (Google) 2018-03-27 15:40 ` Mathieu Desnoyers 0 siblings, 1 reply; 7+ messages in thread From: Joel Fernandes (Google) @ 2018-03-27 15:28 UTC (permalink / raw) To: Mathieu Desnoyers Cc: rostedt, linux-kernel, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar On Tue, Mar 27, 2018 at 6:27 AM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: <snip> >>> +static void find_tp(struct tracepoint *tp, void *priv) >>> +{ >>> + struct tp_find_args *args = priv; >>> + >>> + if (!strcmp(tp->name, args->name)) { >>> + WARN_ON_ONCE(args->tp); >>> + args->tp = tp; >> >> I think this runtime check is not needed if it really can't happen >> (linker verifies that already as you mentioned) although I'm not >> opposed if you still want to keep it, because there's no way to break >> out of the outer loop anyway so I guess your call.. > > We can change the outer loop and break from it if needed, that's not > an issue. > >> I would just do: >> >> if (args->tp) >> return; >> >> if find_tp already found the tracepoint. >> >> Tried to also create a duplicate tracepoint and I get: >> CC init/version.o >> AR init/built-in.o >> AR built-in.o >> LD vmlinux.o >> block/blk-core.o:(__tracepoints+0x440): multiple definition of >> `__tracepoint_sched_switch' >> kernel/sched/core.o:(__tracepoints+0x440): first defined here >> Makefile:1032: recipe for target 'vmlinux' failed >> make: *** [vmlinux] Error 1 > > Yeah, as I state in my changelog, I'm very well aware that the linker > is able to catch those. This was the purpose of emitting a > __tracepoint_##name symbol in the tracepoint definition macro. This > WARN_ON_ONCE() is a redundant check for an invariant that we expect > is provided by the linker. Given that it's not a fast path, I would > prefer to keep this redundant check in place, given that an average > factor 2 speedup on a slow path should really not be something we > need to optimize for. IMHO, for slow paths, robustness is more important > than speed (unless the slow path becomes so slow that it really affects > the user). > > I envision that a way to break this invariant would be to compile a > kernel object with gcc -fvisibility=hidden or such. I admit this is > particular scenario is really far fetched, but it illustrates why I > prefer to keep an extra safety net at runtime for linker-based > validations when possible. > > If a faster tracepoint lookup function is needed, we should consider > perfect hashing algorithms done post-build, but so far nobody has > complained about speed of this lookup operation. Anyhow a factor 2 in > the speed of this lookup should really not matter, right ? Yes, I agree with all the great reasons. So lets do it your way then. thanks, - Joel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name 2018-03-27 15:28 ` Joel Fernandes (Google) @ 2018-03-27 15:40 ` Mathieu Desnoyers 0 siblings, 0 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2018-03-27 15:40 UTC (permalink / raw) To: joel opensrc Cc: rostedt, linux-kernel, Alexei Starovoitov, Peter Zijlstra, Ingo Molnar ----- On Mar 27, 2018, at 11:28 AM, joel opensrc joel.opensrc@gmail.com wrote: > On Tue, Mar 27, 2018 at 6:27 AM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: > <snip> >>>> +static void find_tp(struct tracepoint *tp, void *priv) >>>> +{ >>>> + struct tp_find_args *args = priv; >>>> + >>>> + if (!strcmp(tp->name, args->name)) { >>>> + WARN_ON_ONCE(args->tp); >>>> + args->tp = tp; >>> >>> I think this runtime check is not needed if it really can't happen >>> (linker verifies that already as you mentioned) although I'm not >>> opposed if you still want to keep it, because there's no way to break >>> out of the outer loop anyway so I guess your call.. >> >> We can change the outer loop and break from it if needed, that's not >> an issue. >> >>> I would just do: >>> >>> if (args->tp) >>> return; >>> >>> if find_tp already found the tracepoint. >>> >>> Tried to also create a duplicate tracepoint and I get: >>> CC init/version.o >>> AR init/built-in.o >>> AR built-in.o >>> LD vmlinux.o >>> block/blk-core.o:(__tracepoints+0x440): multiple definition of >>> `__tracepoint_sched_switch' >>> kernel/sched/core.o:(__tracepoints+0x440): first defined here >>> Makefile:1032: recipe for target 'vmlinux' failed >>> make: *** [vmlinux] Error 1 >> >> Yeah, as I state in my changelog, I'm very well aware that the linker >> is able to catch those. This was the purpose of emitting a >> __tracepoint_##name symbol in the tracepoint definition macro. This >> WARN_ON_ONCE() is a redundant check for an invariant that we expect >> is provided by the linker. Given that it's not a fast path, I would >> prefer to keep this redundant check in place, given that an average >> factor 2 speedup on a slow path should really not be something we >> need to optimize for. IMHO, for slow paths, robustness is more important >> than speed (unless the slow path becomes so slow that it really affects >> the user). >> >> I envision that a way to break this invariant would be to compile a >> kernel object with gcc -fvisibility=hidden or such. I admit this is >> particular scenario is really far fetched, but it illustrates why I >> prefer to keep an extra safety net at runtime for linker-based >> validations when possible. >> >> If a faster tracepoint lookup function is needed, we should consider >> perfect hashing algorithms done post-build, but so far nobody has >> complained about speed of this lookup operation. Anyhow a factor 2 in >> the speed of this lookup should really not matter, right ? > > Yes, I agree with all the great reasons. So lets do it your way then. Alexei proposed an alternative patch. I don't strongly mind one approach or the other. Thanks, Mathieu > > thanks, > > - Joel -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-27 15:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-26 19:10 [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name Mathieu Desnoyers 2018-03-26 20:48 ` Alexei Starovoitov 2018-03-26 21:05 ` Steven Rostedt 2018-03-27 1:35 ` Joel Fernandes (Google) 2018-03-27 13:27 ` Mathieu Desnoyers 2018-03-27 15:28 ` Joel Fernandes (Google) 2018-03-27 15:40 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).