[PATCH bpf-next] bpf, tracing: unbreak lttng

[PATCH bpf-next] bpf, tracing: unbreak lttng

[Alexei Starovoitov]

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(-)

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 */
-- 
2.9.5

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Steven Rostedt]

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

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Alexei Starovoitov]

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.

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Mathieu Desnoyers]

----- 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



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

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Steven Rostedt]

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

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Mathieu Desnoyers]

----- 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

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

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Alexei Starovoitov]

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.

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Steven Rostedt]

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

Re: [PATCH bpf-next] bpf, tracing: unbreak lttng

[Alexei Starovoitov]

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.