| Message ID | 20180326230220.1069180-1-ast@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v2,bpf-next] bpf, tracing: unbreak lttng | expand |
On Mon, 26 Mar 2018 16:02:20 -0700 Alexei Starovoitov <ast@kernel.org> wrote: > for_each_kernel_tracepoint() is used by out-of-tree lttng module > and therefore cannot be changed. This is false and misleading. NACK. -- Steve > Instead introduce kernel_tracepoint_find_by_name() to find > tracepoint by name. > > Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time") > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > v1->v2: fix 'undef CONFIG_TRACEPOINTS' build as spotted by Mathieu >
On 03/27/2018 02:07 AM, Steven Rostedt wrote: > On Mon, 26 Mar 2018 16:02:20 -0700 > Alexei Starovoitov <ast@kernel.org> wrote: > >> for_each_kernel_tracepoint() is used by out-of-tree lttng module >> and therefore cannot be changed. > > This is false and misleading. NACK. Steven, while I really don't care about this particular function, you wrote a few emails ago, quote: Look, the tracepoint code was written by Mathieu for LTTng, and perf and ftrace were able to benefit because of it, as well as your bpf code. For this, we agreed to keep this function around for his use, as its the only thing he requires. Everyone has been fine with that. [...] Having that function for LTTng does not hurt us. And I will NACK removing it. So later saying that "this is false and misleading" is kind of misleading by itself. ;-) Anyway, it would probably make sense to add a comment to for_each_kernel_tracepoint() that this is used by LTTng so that people don't accidentally remove it due to missing in-tree user. I would think some sort of clarification/background in a comment or such would have avoided the whole confusion and resulting discussion around this in the first place. Btw, in networking land, as soon as there is no in-tree user for a particular kernel function, it will get ripped out, no matter what. Given this is also the typical convention in the kernel, it may have caused some confusion with above preference. Anyway, given v6 is out now, I've tossed the old series from bpf-next tree. So I hope we can all move on with some more constructive discussion. :-) Thanks everyone, Daniel
On Tue, 27 Mar 2018 11:39:19 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 03/27/2018 02:07 AM, Steven Rostedt wrote: > > On Mon, 26 Mar 2018 16:02:20 -0700 > > Alexei Starovoitov <ast@kernel.org> wrote: > > > >> for_each_kernel_tracepoint() is used by out-of-tree lttng module > >> and therefore cannot be changed. > > > > This is false and misleading. NACK. > > Steven, while I really don't care about this particular function, you wrote > a few emails ago, quote: > > Look, the tracepoint code was written by Mathieu for LTTng, and perf > and ftrace were able to benefit because of it, as well as your bpf > code. For this, we agreed to keep this function around for his use, > as its the only thing he requires. Everyone has been fine with that. > [...] Having that function for LTTng does not hurt us. And I will NACK > removing it. > > So later saying that "this is false and misleading" is kind of misleading > by itself. ;-) Anyway, it would probably make sense to add a comment to > for_each_kernel_tracepoint() that this is used by LTTng so that people > don't accidentally remove it due to missing in-tree user. I would think > some sort of clarification/background in a comment or such would have > avoided the whole confusion and resulting discussion around this in the > first place. I agree, a comment should be added. But the function can still be modified, which is what I meant here by being misleading. > > Btw, in networking land, as soon as there is no in-tree user for a particular > kernel function, it will get ripped out, no matter what. Given this is also > the typical convention in the kernel, it may have caused some confusion with > above preference. This is usually the case with me too. This came from Mathieu doing a lot of work to help perf and ftrace, but keep this for him to maintain LTTng. This was the solution to a long drawn out flame war. Yes, there's a lot of history behind that function, and I agree that we should comment the history behind it. > > Anyway, given v6 is out now, I've tossed the old series from bpf-next tree. > So I hope we can all move on with some more constructive discussion. :-) Yes, which we are doing around the kallsyms part. ;-) -- Steve
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index 2194e7c31484..d578a962091e 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -42,13 +42,16 @@ extern int tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data); #ifdef CONFIG_TRACEPOINTS -void * -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv), +void +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), void *priv); +struct tracepoint *kernel_tracepoint_find_by_name(const char *name); #else -static inline void * -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv), - void *priv) +static inline void +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), + void *priv) {} +static inline struct tracepoint * +kernel_tracepoint_find_by_name(const char *name) { return NULL; } diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index ae8b43f1cee3..644311777d8e 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1334,15 +1334,6 @@ static const struct file_operations bpf_raw_tp_fops = { .write = bpf_dummy_write, }; -static void *__find_tp(struct tracepoint *tp, void *priv) -{ - char *name = priv; - - if (!strcmp(tp->name, name)) - return tp; - return NULL; -} - #define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd static int bpf_raw_tracepoint_open(const union bpf_attr *attr) @@ -1358,7 +1349,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) return -EFAULT; tp_name[sizeof(tp_name) - 1] = 0; - tp = for_each_kernel_tracepoint(__find_tp, tp_name); + tp = kernel_tracepoint_find_by_name(tp_name); if (!tp) return -ENOENT; diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 3f2dc5738c2b..764d02fbe782 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -502,22 +502,17 @@ static __init int init_tracepoints(void) __initcall(init_tracepoints); #endif /* CONFIG_MODULES */ -static void *for_each_tracepoint_range(struct tracepoint * const *begin, - struct tracepoint * const *end, - void *(*fct)(struct tracepoint *tp, void *priv), - void *priv) +static void for_each_tracepoint_range(struct tracepoint * const *begin, + struct tracepoint * const *end, + void (*fct)(struct tracepoint *tp, void *priv), + void *priv) { struct tracepoint * const *iter; - void *ret; if (!begin) - return NULL; - for (iter = begin; iter < end; iter++) { - ret = fct(*iter, priv); - if (ret) - return ret; - } - return NULL; + return; + for (iter = begin; iter < end; iter++) + fct(*iter, priv); } /** @@ -525,14 +520,23 @@ static void *for_each_tracepoint_range(struct tracepoint * const *begin, * @fct: callback * @priv: private data */ -void *for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv), - void *priv) +void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), + void *priv) { - return for_each_tracepoint_range(__start___tracepoints_ptrs, - __stop___tracepoints_ptrs, fct, priv); + for_each_tracepoint_range(__start___tracepoints_ptrs, + __stop___tracepoints_ptrs, fct, priv); } EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint); +struct tracepoint *kernel_tracepoint_find_by_name(const char *name) +{ + struct tracepoint * const *tp = __start___tracepoints_ptrs; + + for (; tp < __stop___tracepoints_ptrs; tp++) + if (!strcmp((*tp)->name, name)) + return *tp; + return NULL; +} #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
for_each_kernel_tracepoint() is used by out-of-tree lttng module and therefore cannot be changed. Instead introduce kernel_tracepoint_find_by_name() to find tracepoint by name. Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time") Signed-off-by: Alexei Starovoitov <ast@kernel.org> --- v1->v2: fix 'undef CONFIG_TRACEPOINTS' build as spotted by Mathieu --- include/linux/tracepoint.h | 13 ++++++++----- kernel/bpf/syscall.c | 11 +---------- kernel/tracepoint.c | 36 ++++++++++++++++++++---------------- 3 files changed, 29 insertions(+), 31 deletions(-)