bpf, tracing: unbreak lttng

PATCH In review Alexei Starovoitov 3 participants 4 messages Mar 26, 2018 – Mar 27, 2018

Subsystems (6)

Networking Network drivers BPF Tracing +2 more subsystems

lore

Share & tools

lore

Alexei Starovoitov

author maintainer

2018-03-26 23:02 UTC #0 lore raw

PATCH v2 1/1 bpf, tracing: unbreak lttng

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 <[email protected]>

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

include/linux/tracepoint.h +8 -5

index 2194e7c31484..d578a962091e 100644
@@ -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;
 }

kernel/bpf/syscall.c +1 ~~-10~~

index ae8b43f1cee3..644311777d8e 100644
@@ -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;

kernel/tracepoint.c +20 ~~-16~~

index 3f2dc5738c2b..764d02fbe782 100644
@@ -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 */

Signature · 1 line

2.9.5

Steven Rostedt

maintainer

2018-03-27 00:07 UTC #1 lore raw

Quoted 2 lines · On Mon, 26 Mar 2018 16:02:20 -0700 Alexei Starovoitov <[email protected]> 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

Quoted 8 lines

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 <[email protected]>
---
v1->v2: fix 'undef CONFIG_TRACEPOINTS' build as spotted by Mathieu

Daniel Borkmann

maintainer

2018-03-27 09:39 UTC #2 lore raw

Quoted 7 lines · Steven Rostedt

Quoted 2 lines · On Mon, 26 Mar 2018 16:02:20 -0700 Alexei Starovoitov <[email protected]> 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

Steven Rostedt

maintainer

2018-03-27 19:35 UTC #3 lore raw

Quoted 19 lines · On Tue, 27 Mar 2018 11:39:19 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 7 lines · Steven Rostedt
Quoted 2 lines · On Mon, 26 Mar 2018 16:02:20 -0700 Alexei Starovoitov <[email protected]> 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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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