| Message ID | 20180326220845.678423-1-ast@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf, tracing: unbreak lttng | expand |
On Mon, 26 Mar 2018 15:08:45 -0700 Alexei Starovoitov <ast@kernel.org> wrote: > 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> I'm curious, why can't you rebase? The first patch was never acked. -- Steve
On 3/26/18 3:15 PM, Steven Rostedt wrote: > On Mon, 26 Mar 2018 15:08:45 -0700 > Alexei Starovoitov <ast@kernel.org> wrote: > >> 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> > > I'm curious, why can't you rebase? The first patch was never acked. because I think it makes sense to keep such things in the commit log and in the separate diff, so next developer is aware of what kind of minefield the tracpoints are. No wonder some maintainers refuse to add them.
----- On Mar 26, 2018, at 6:08 PM, Alexei Starovoitov ast@kernel.org wrote: [...] > > #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), > +static inline void > +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), > void *priv) > { > return NULL; > } This patch is not reverting to the old code properly. It introduces a static inline void function that returns NULL. Please compile-test with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints. But this patch should not even be needed in the first place, because it partially reverts changes that were introduced in the bpf-next tree without any Acked-by from the tracing maintainers. I don't see any need to obfuscate the git log of tracepoint.{c,h}. Thanks, Mathieu
On 3/26/18 3:30 PM, Mathieu Desnoyers wrote: > ----- On Mar 26, 2018, at 6:08 PM, Alexei Starovoitov ast@kernel.org wrote: > [...] >> >> #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), >> +static inline void >> +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), >> void *priv) >> { >> return NULL; >> } > > This patch is not reverting to the old code properly. It introduces a > static inline void function that returns NULL. Please compile-test > with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints. right. good catch. v2 is coming.
----- On Mar 26, 2018, at 6:25 PM, Alexei Starovoitov ast@fb.com wrote: > On 3/26/18 3:15 PM, Steven Rostedt wrote: >> On Mon, 26 Mar 2018 15:08:45 -0700 >> Alexei Starovoitov <ast@kernel.org> wrote: >> >>> 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> >> >> I'm curious, why can't you rebase? The first patch was never acked. > > because I think it makes sense to keep such things in the commit log > and in the separate diff, so next developer is aware of what kind of > minefield the tracpoints are. > No wonder some maintainers refuse to add them. Since when has it become accepted to push commits into maintainer's subsystems without their acknowledgment first ? The minefield you are currently walking through appears to be of your own making, so please just rework your initial patch before it reaches upstream. Thanks, Mathieu
On Mon, 26 Mar 2018 15:25:32 -0700 Alexei Starovoitov <ast@fb.com> wrote: > On 3/26/18 3:15 PM, Steven Rostedt wrote: > > On Mon, 26 Mar 2018 15:08:45 -0700 > > Alexei Starovoitov <ast@kernel.org> wrote: > > > >> 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> > > > > I'm curious, why can't you rebase? The first patch was never acked. > > because I think it makes sense to keep such things in the commit log > and in the separate diff, so next developer is aware of what kind of > minefield the tracpoints are. This is a bunch of BS. It's not a minefield, and you can change that function. Mathieu is perfectly fine in modifying his code to deal with it. He has several times in the past. But I did not agree with the approach you were taking, that is why I'm against it. You are playing the straw man with this. > No wonder some maintainers refuse to add them. Good grief. No! The reason maintainers refuse to add them is that userspace can depend on them, and if that happens, it becomes an ABI. Stop with this nonsense. -- Steve
On Mon, 26 Mar 2018 15:35:56 -0700 Alexei Starovoitov <ast@fb.com> wrote: > > This patch is not reverting to the old code properly. It introduces a > > static inline void function that returns NULL. Please compile-test > > with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints. > > right. good catch. v2 is coming. Either fold the patch into the original patch, or I'm pulling in Mathieu's patch and pushing it to Linus come the merge window. -- Steve
On 3/26/18 5:08 PM, Steven Rostedt wrote: > On Mon, 26 Mar 2018 15:35:56 -0700 > Alexei Starovoitov <ast@fb.com> wrote: > > >>> This patch is not reverting to the old code properly. It introduces a >>> static inline void function that returns NULL. Please compile-test >>> with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints. >> >> right. good catch. v2 is coming. > > Either fold the patch into the original patch, or I'm pulling in > Mathieu's patch and pushing it to Linus come the merge window. Ok. I will fold this patch into previous set and rebase the tree.
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index 2194e7c31484..035887dc4ed3 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -42,16 +42,22 @@ 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), +static inline void +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv), void *priv) { return NULL; } +static inline struct tracepoint * +kernel_tracepoint_find_by_name(const char *name) +{ + return NULL; +} #endif #ifdef CONFIG_MODULES 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> --- include/linux/tracepoint.h | 14 ++++++++++---- kernel/bpf/syscall.c | 11 +---------- kernel/tracepoint.c | 36 ++++++++++++++++++++---------------- 3 files changed, 31 insertions(+), 30 deletions(-)