bpf, tracing: introduce bpf raw tracepoints

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

Signed-off-by Alexei Starovoitov <[email protected]>

---
 include/linux/tracepoint-defs.h |  1 +
 include/linux/tracepoint.h      | 28 +++++++++++++++++++---------
 include/trace/define_trace.h    | 14 +++++++-------
 kernel/tracepoint.c             | 27 ++++++++++++++++-----------
 4 files changed, 43 insertions(+), 27 deletions(-)

include/linux/tracepoint-defs.h +1 -0

index 64ed7064f1fa..39a283c61c51 100644
@@ -33,6 +33,7 @@ struct tracepoint {
 	int (*regfunc)(void);
 	void (*unregfunc)(void);
 	struct tracepoint_func __rcu *funcs;
+	u32 num_args;
 };
 
 #endif

include/linux/tracepoint.h +19 -9

index c94f466d57ef..2194e7c31484 100644
@@ -40,9 +40,19 @@ tracepoint_probe_register_prio(struct tracepoint *tp, void *probe, void *data,
 			       int prio);
 extern int
 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);
+
+#ifdef CONFIG_TRACEPOINTS
+void *
+for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+			   void *priv);
+#else
+static inline void *
+for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+			   void *priv)
+{
+	return NULL;
+}
+#endif
 
 #ifdef CONFIG_MODULES
 struct tp_module {

@@ -230,18 +240,18 @@ extern void syscall_unregfunc(void);
  * structures, so we create an array of pointers that will be used for iteration
  * on the tracepoints.
  */
-#define DEFINE_TRACE_FN(name, reg, unreg)				 \
+#define DEFINE_TRACE_FN(name, reg, unreg, num_args)			 \
 	static const char __tpstrtab_##name[]				 \
 	__attribute__((section("__tracepoints_strings"))) = #name;	 \
 	struct tracepoint __tracepoint_##name				 \
 	__attribute__((section("__tracepoints"))) =			 \
-		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
+		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
 	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
 	__attribute__((section("__tracepoints_ptrs"))) =		 \
 		&__tracepoint_##name;
 
-#define DEFINE_TRACE(name)						\
-	DEFINE_TRACE_FN(name, NULL, NULL);
+#define DEFINE_TRACE(name, num_args)					\
+	DEFINE_TRACE_FN(name, NULL, NULL, num_args);
 
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)				\
 	EXPORT_SYMBOL_GPL(__tracepoint_##name)

@@ -275,8 +285,8 @@ extern void syscall_unregfunc(void);
 		return false;						\
 	}
 
-#define DEFINE_TRACE_FN(name, reg, unreg)
-#define DEFINE_TRACE(name)
+#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
+#define DEFINE_TRACE(name, num_args)
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
 #define EXPORT_TRACEPOINT_SYMBOL(name)
 

include/trace/define_trace.h +7 -7

index d9e3d4aa3f6e..96b22ace9ae7 100644
@@ -25,7 +25,7 @@
 
 #undef TRACE_EVENT
 #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef TRACE_EVENT_CONDITION
 #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \

@@ -39,24 +39,24 @@
 #undef TRACE_EVENT_FN
 #define TRACE_EVENT_FN(name, proto, args, tstruct,		\
 		assign, print, reg, unreg)			\
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef TRACE_EVENT_FN_COND
 #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct,		\
 		assign, print, reg, unreg)			\
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args) \
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_FN
 #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_CONDITION
 #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \

@@ -64,7 +64,7 @@
 
 #undef DECLARE_TRACE
 #define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE

kernel/tracepoint.c +16 -11

index 671b13457387..3f2dc5738c2b 100644
@@ -502,17 +502,22 @@ 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;
-	for (iter = begin; iter < end; iter++)
-		fct(*iter, priv);
+		return NULL;
+	for (iter = begin; iter < end; iter++) {
+		ret = fct(*iter, priv);
+		if (ret)
+			return ret;
+	}
+	return NULL;
 }
 
 /**

@@ -520,11 +525,11 @@ 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)
 {
-	for_each_tracepoint_range(__start___tracepoints_ptrs,
-		__stop___tracepoints_ptrs, fct, priv);
+	return for_each_tracepoint_range(__start___tracepoints_ptrs,
+					 __stop___tracepoints_ptrs, fct, priv);
 }
 EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
 

Quoted 7 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

Quoted 4 lines

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

I believe this is used by LTTng.

-- Steve

Quoted 186 lines

Signed-off-by Alexei Starovoitov <[email protected]>

---
 include/linux/tracepoint-defs.h |  1 +
 include/linux/tracepoint.h      | 28 +++++++++++++++++++---------
 include/trace/define_trace.h    | 14 +++++++-------
 kernel/tracepoint.c             | 27 ++++++++++++++++-----------
 4 files changed, 43 insertions(+), 27 deletions(-)

include/linux/tracepoint-defs.h +1 -0

index 64ed7064f1fa..39a283c61c51 100644
@@ -33,6 +33,7 @@ struct tracepoint {
 	int (*regfunc)(void);
 	void (*unregfunc)(void);
 	struct tracepoint_func __rcu *funcs;
+	u32 num_args;
 };
 
 #endif

include/linux/tracepoint.h +13 -3

index c94f466d57ef..2194e7c31484 100644
@@ -40,9 +40,19 @@ tracepoint_probe_register_prio(struct tracepoint *tp, void
 *probe, void *data,
 			       int prio);
 extern int
 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);
+
+#ifdef CONFIG_TRACEPOINTS
+void *
+for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+			   void *priv);
+#else
+static inline void *
+for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+			   void *priv)
+{
+	return NULL;
+}
+#endif
 
 #ifdef CONFIG_MODULES

struct tp_module {

include/linux/tracepoint.h +6 -6

@@ -230,18 +240,18 @@ extern void syscall_unregfunc(void);
  * structures, so we create an array of pointers that will be used for iteration
  * on the tracepoints.
  */
-#define DEFINE_TRACE_FN(name, reg, unreg)				 \
+#define DEFINE_TRACE_FN(name, reg, unreg, num_args)			 \
 	static const char __tpstrtab_##name[]				 \
 	__attribute__((section("__tracepoints_strings"))) = #name;	 \
 	struct tracepoint __tracepoint_##name				 \
 	__attribute__((section("__tracepoints"))) =			 \
-		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
+		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
 	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
 	__attribute__((section("__tracepoints_ptrs"))) =		 \
 		&__tracepoint_##name;
 
-#define DEFINE_TRACE(name)						\
-	DEFINE_TRACE_FN(name, NULL, NULL);
+#define DEFINE_TRACE(name, num_args)					\
+	DEFINE_TRACE_FN(name, NULL, NULL, num_args);
 
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)				\
 	EXPORT_SYMBOL_GPL(__tracepoint_##name)

@@ -275,8 +285,8 @@ extern void syscall_unregfunc(void);
 		return false;						\
 	}
 
-#define DEFINE_TRACE_FN(name, reg, unreg)
-#define DEFINE_TRACE(name)
+#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
+#define DEFINE_TRACE(name, num_args)
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
 #define EXPORT_TRACEPOINT_SYMBOL(name)
 

include/trace/define_trace.h +7 -7

index d9e3d4aa3f6e..96b22ace9ae7 100644
@@ -25,7 +25,7 @@
 
 #undef TRACE_EVENT
 #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef TRACE_EVENT_CONDITION
 #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \

@@ -39,24 +39,24 @@
 #undef TRACE_EVENT_FN
 #define TRACE_EVENT_FN(name, proto, args, tstruct,		\
 		assign, print, reg, unreg)			\
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef TRACE_EVENT_FN_COND
 #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct,		\
 		assign, print, reg, unreg)			\
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args) \
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_FN
 #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
-	DEFINE_TRACE_FN(name, reg, unreg)
+	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef DEFINE_EVENT_CONDITION
 #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \

@@ -64,7 +64,7 @@
 
 #undef DECLARE_TRACE
 #define DECLARE_TRACE(name, proto, args)	\
-	DEFINE_TRACE(name)
+	DEFINE_TRACE(name, COUNT_ARGS(args))
 
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE

kernel/tracepoint.c +16 -11

index 671b13457387..3f2dc5738c2b 100644
@@ -502,17 +502,22 @@ 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;
-	for (iter = begin; iter < end; iter++)
-		fct(*iter, priv);
+		return NULL;
+	for (iter = begin; iter < end; iter++) {
+		ret = fct(*iter, priv);
+		if (ret)
+			return ret;
+	}
+	return NULL;
 }
 
 /**

@@ -520,11 +525,11 @@ 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)
 {
-	for_each_tracepoint_range(__start___tracepoints_ptrs,
-		__stop___tracepoints_ptrs, fct, priv);
+	return for_each_tracepoint_range(__start___tracepoints_ptrs,
+					 __stop___tracepoints_ptrs, fct, priv);
 }

EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);

Quoted 15 lines · ----- On Mar 26, 2018, at 11:02 AM, rostedt [email protected] wrote:

Quoted 11 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

Quoted 1 line

I believe this is used by LTTng.

Indeed, and by SystemTAP as well.

What justifies the need to stop mid-iteration ? A less intrusive alternative would be to use the "priv" data pointer to keep state telling further calls to return immediately. Does performance of iteration over tracepoints really matter here so much that stopping iteration immediately is worth it ?

Quoted 17 lines · Mathieu Desnoyers

Quoted 15 lines · ----- On Mar 26, 2018, at 11:02 AM, rostedt [email protected] wrote:

Quoted 11 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

Quoted 8 lines

Quoted 1 line

I believe this is used by LTTng.

Indeed, and by SystemTAP as well.

What justifies the need to stop mid-iteration ? A less intrusive alternative would be to use the "priv" data pointer to keep state telling further calls to return immediately. Does performance of iteration over tracepoints really matter here so much that stopping iteration immediately is worth it ?

I'm sure both you and Steven are not serious when you object to _in-tree_ change to for_each_kernel_tracepoint() that affects _out-of_tree_ modules?

Just change your module to 'return NULL' instead of plain 'return'.

Quoted 27 lines · ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov [email protected] wrote:

Quoted 17 lines · Mathieu Desnoyers

Quoted 15 lines · ----- On Mar 26, 2018, at 11:02 AM, rostedt [email protected] wrote:

Quoted 11 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

Quoted 8 lines

Quoted 1 line

I believe this is used by LTTng.

Indeed, and by SystemTAP as well.

What justifies the need to stop mid-iteration ? A less intrusive alternative would be to use the "priv" data pointer to keep state telling further calls to return immediately. Does performance of iteration over tracepoints really matter here so much that stopping iteration immediately is worth it ?

Quoted 5 lines

I'm sure both you and Steven are not serious when you object to _in-tree_ change to for_each_kernel_tracepoint() that affects _out-of_tree_ modules?

Just change your module to 'return NULL' instead of plain 'return'.

I never said I objected to adapt the LTTng out of tree code. If there is a solid reason for changing the kernel API, I will adapt my code to those changes.

What I'm trying to understand here is whether there is solid ground for the added complexity you are proposing. Is it a performance enhancement ? If so, explanation of the use cases targeted, and numbers that measure performance improvements are needed.

How is your patch making tracepoints "more useful" ?

Quoted 44 lines · Mathieu Desnoyers

Quoted 27 lines · ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov [email protected] wrote:

Quoted 17 lines · Mathieu Desnoyers

Quoted 15 lines · ----- On Mar 26, 2018, at 11:02 AM, rostedt [email protected] wrote:

Quoted 11 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

From: Alexei Starovoitov <[email protected]>

add fancy macro to compute number of arguments passed into tracepoint at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch.

Quoted 8 lines

Quoted 1 line

I believe this is used by LTTng.

Indeed, and by SystemTAP as well.

What justifies the need to stop mid-iteration ? A less intrusive alternative would be to use the "priv" data pointer to keep state telling further calls to return immediately. Does performance of iteration over tracepoints really matter here so much that stopping iteration immediately is worth it ?

Quoted 5 lines

I'm sure both you and Steven are not serious when you object to _in-tree_ change to for_each_kernel_tracepoint() that affects _out-of_tree_ modules?

Just change your module to 'return NULL' instead of plain 'return'.

I never said I objected to adapt the LTTng out of tree code. If there is a solid reason for changing the kernel API, I will adapt my code to those changes.

What I'm trying to understand here is whether there is solid ground for the added complexity you are proposing. Is it a performance enhancement ? If so, explanation of the use cases targeted, and numbers that measure performance improvements are needed.

Quoted 1 line

How is your patch making tracepoints "more useful" ?

are you arguing about the whole set overall or about a change to for_each_kernel_tracepoint() ?

I'm hearing your arguments as that now changes to all exported functions need to be "solid" (not sure what exactly means 'solid' here) to justify breakage of out-of-tree modules?

re: 'added complexity'...

kernel/tracepoint.c +7 -2

-       for (iter = begin; iter < end; iter++)
-               fct(*iter, priv);
+               return NULL;
+       for (iter = begin; iter < end; iter++) {
+               ret = fct(*iter, priv);
+               if (ret)
+                       return ret;
+       }
+       return NULL;

where do you see 'added complexity' ?
Isn't the above diff self-explanatory that for_each_tracepoint_range() can be used not only to iterate over all tracepoints
(just do 'return NULL') from callback _and_ to find one particular tracepoint as patch 7 does ?

Quoted 12 lines

re: 'added complexity'...

(quoted diff) +7 -2

-       for (iter = begin; iter < end; iter++)
-               fct(*iter, priv);
+               return NULL;
+       for (iter = begin; iter < end; iter++) {
+               ret = fct(*iter, priv);
+               if (ret)
+                       return ret;
+       }
+       return NULL;

Quoted 5 lines

where do you see 'added complexity' ?
Isn't the above diff self-explanatory that for_each_tracepoint_range() can be used not only to iterate over all tracepoints
(just do 'return NULL') from callback _and_ to find one particular tracepoint as patch 7 does ?

I am not arguing about your proposed implementation. I am arguing about the lack of justification behind this change. Why is this change needed ? What is it allowing you to do that cannot be done using the private data pointer ?

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

(quoted diff) +12 -0

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

still not obvious?

Quoted 24 lines · On Mon, 26 Mar 2018 09:25:07 -0700 Alexei Starovoitov <[email protected]> wrote:

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

(quoted diff) +12 -0

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

Quoted 1 line

still not obvious?

Please just create a new function called tracepoint_find_by_name(), and use that. I don't see any benefit in using a for_each* function for such a simple routine. Not to mention, you then don't need to know the internals of a tracepoint in kernel/bpf/syscall.c.

I don't mind to _rename_ for_each_kernel_tracepoint() into tracepoint_find_by_name(), but keeping exported function just to be used by out-of-tree modules would be wrong message for the kernel community in general.
With my patch the for_each_kernel_tracepoint() will be used by bpf side and out-of-tree can trivially hack their callbacks to keep working. imo that's a better approach then renaming it.

Quoted 24 lines · On Mon, 26 Mar 2018 09:25:07 -0700 Alexei Starovoitov <[email protected]> wrote:

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

(quoted diff) +12 -0

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

Quoted 1 line

still not obvious?

Please just create a new function called tracepoint_find_by_name(), and use that. I don't see any benefit in using a for_each* function for such a simple routine. Not to mention, you then don't need to know the internals of a tracepoint in kernel/bpf/syscall.c.

-- Steve

Quoted 27 lines · ----- On Mar 26, 2018, at 12:35 PM, rostedt [email protected] wrote:

Quoted 24 lines · On Mon, 26 Mar 2018 09:25:07 -0700 Alexei Starovoitov <[email protected]> wrote:

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel. Make it more useful with ability to stop for_each() loop depending via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

(quoted diff) +12 -0

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

Quoted 6 lines

Quoted 1 line

still not obvious?

Please just create a new function called tracepoint_find_by_name(), and use that. I don't see any benefit in using a for_each* function for such a simple routine. Not to mention, you then don't need to know the internals of a tracepoint in kernel/bpf/syscall.c.

Steven's approach is fine by me, considering there should never be duplicated tracepoint definitions (it emits a __tracepoint_##name symbol which would cause multiple symbols definition errors at link time if there are more than a single definition per tracepoint name in the core kernel). The exported API should probably be named "kernel_tracepoint_find_by_name()" or something similar, thus indicating that it only lookup tracepoints in the core kernel.

Which brings the next question: what are Alexei's plan to handle tracepoints in modules, considering module load/unload scenarios ? The tracepoint API has module notifiers for this, but it does not appear to be used in this patch series.

An email ago you were ok to s/return/return NULL/ in your out-of-tree module, but now flip flop to add new function approach just to reduce the work you need to do in lttng?
We're not talking about changing __kmalloc signature here. My patch extends for_each_kernel_tracepoint() api similar to other for_each_*() iterators and improves possible uses of it.

One thing is to be nice to out-of-tree and do not break them for no reason, but arguing that kernel shouldn't add a minor extension to for_each_kernel_tracepoint() api is really taking the whole thing to next level.

Quoted 6 lines · On Mon, 26 Mar 2018 10:55:51 -0700 Alexei Starovoitov <[email protected]> wrote:

An email ago you were ok to s/return/return NULL/ in your out-of-tree module, but now flip flop to add new function approach just to reduce the work you need to do in lttng?
We're not talking about changing __kmalloc signature here. My patch extends for_each_kernel_tracepoint() api similar to other for_each_*() iterators and improves possible uses of it.

Alexei, do you have another use case for using for_each_kernel_tracepoint() other than the find_tp? If so, then I'm sure Mathieu can handle the change.

But I think it's cleaner to add a tracepoint_find_by_name() function. If you come up with another use case for using the for_each* function then we'll consider changing it then.

Quoted 14 lines · Steven Rostedt

Quoted 6 lines · On Mon, 26 Mar 2018 10:55:51 -0700 Alexei Starovoitov <[email protected]> wrote:

An email ago you were ok to s/return/return NULL/ in your out-of-tree module, but now flip flop to add new function approach just to reduce the work you need to do in lttng?
We're not talking about changing __kmalloc signature here. My patch extends for_each_kernel_tracepoint() api similar to other for_each_*() iterators and improves possible uses of it.

Alexei, do you have another use case for using for_each_kernel_tracepoint() other than the find_tp? If so, then I'm sure Mathieu can handle the change.

Quoted 3 lines

But I think it's cleaner to add a tracepoint_find_by_name() function. If you come up with another use case for using the for_each* function then we'll consider changing it then.

another use case ?! Frankly such reasoning smells.

I'm fine doing quick followup patch to add tracepoint_find_by_name()

and restore 'return void' behavior of for_each_kernel_tracepoint's

callback, but I'm struggling to accept the precedent it will create that all exported functions of kernel/tracepoint.c are really lttng extensions and we cannot easily change them.

I'd like to hear Linus take on this.

correct. this set deals with in-kernel tracepoints only. No attempt to do anything with tracepoints inside modules.

Also I hope you noticed that the patch is doing:
+++ b/include/linux/tracepoint-defs.h

(quoted diff) +1 -0

@@ -33,6 +33,7 @@ struct tracepoint {
        int (*regfunc)(void);
        void (*unregfunc)(void);
        struct tracepoint_func __rcu *funcs;
+       u32 num_args;
 };
 

To make sure that bpf programs are safe I need to do a static check in the verifier that programs don't access arguments beyond those specified by the tracepoint.

That was mentioned in the commit log of patch 6 too:
"
compute number of arguments passed into tracepoint
at compile time and store it as part of 'struct tracepoint'. The number is necessary to check safety of bpf program access that is coming in subsequent patch.
"

kernel/tracepoint.c +5 -0

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

kernel/tracepoint.c +2 -0

+	}
+	return NULL;
 }

Quoted 8 lines · On Fri, 23 Mar 2018 19:30:34 -0700 Alexei Starovoitov <[email protected]> wrote:

kernel/tracepoint.c +5 -0

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

kernel/tracepoint.c static __init int init_tracepoints(void) · line 518

Quoted 9 lines

kernel/tracepoint.c +5 -3

 	if (!begin)
-		return;
-	for (iter = begin; iter < end; iter++)
-		fct(*iter, priv);
+		return NULL;
+	for (iter = begin; iter < end; iter++) {
+		ret = fct(*iter, priv);
+		if (ret)
+			return ret;  

So you just stopped the loop here. You have an inconsistent state. What about the functions that were called before. How do you undo them? Or what about the rest that haven't been touched. This function gives no feedback to the caller.

From bpf program point of view the access to the arguments look like:

From: Alexei Starovoitov <[email protected]>

v4->v5:
- adopted Daniel's fancy REPEAT macro in bpf_trace.c in patch 7

From: Alexei Starovoitov <[email protected]>

v4->v5:
- adopted Daniel's fancy REPEAT macro in bpf_trace.c in patch 7

v3->v4:
- adopted Linus's CAST_TO_U64 macro to cast any integer, pointer, or small struct to u64. That nicely reduced the size of patch 1

v2->v3:
- with Linus's suggestion introduced generic COUNT_ARGS and CONCATENATE macros (or rather moved them from apparmor)
that cleaned up patches 6 and 7
- added patch 4 to refactor trace_iwlwifi_dev_ucode_error() from 17 args to 4 Now any tracepoint with >12 args will have build error

v1->v2:
- simplified api by combing bpf_raw_tp_open(name) + bpf_attach(prog_fd) into bpf_raw_tp_open(name, prog_fd) as suggested by Daniel.
That simplifies bpf_detach as well which is now simple close() of fd.
- fixed memory leak in error path which was spotted by Daniel.
- fixed bpf_get_stackid(), bpf_perf_event_output() called from raw tracepoints
- added more tests
- fixed allyesconfig build caught by buildbot

v1:
This patch set is a different way to address the pressing need to access task_struct pointers in sched tracepoints from bpf programs.

The first approach simply added these pointers to sched tracepoints:
https://lkml.org/lkml/2017/12/14/753
which Peter nacked.
Few options were discussed and eventually the discussion converged on doing bpf specific tracepoint_probe_register() probe functions. Details here:
https://lkml.org/lkml/2017/12/20/929

Patch 1 is kernel wide cleanup of pass-struct-by-value into pass-struct-by-reference into tracepoints.

Patches 2 and 3 are minor cleanups to address allyesconfig build

Patch 4 refactor trace_iwlwifi_dev_ucode_error from 17 to 4 args

Patch 5 introduces COUNT_ARGS macro

Patch 6 minor prep work to expose number of arguments passed into tracepoints.

Patch 7 introduces BPF_RAW_TRACEPOINT api.
the auto-cleanup and multiple concurrent users are must have features of tracing api. For bpf raw tracepoints it looks like:
// load bpf prog with BPF_PROG_TYPE_RAW_TRACEPOINT type prog_fd = bpf_prog_load(...);

// receive anon_inode fd for given bpf_raw_tracepoint // and attach bpf program to it
raw_tp_fd = bpf_raw_tracepoint_open("xdp_exception", prog_fd);

Ctrl-C of tracing daemon or cmdline tool will automatically detach bpf program, unload it and unregister tracepoint probe. More details in patch 7.

Patch 8 - trivial support in libbpf
Patches 9, 10 - user space tests

samples/bpf/test_overhead performance on 1 cpu:

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K

Quoted 3 lines

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K  

Applied to bpf-next, thanks Alexei!

Quoted 5 lines · On Mon, 26 Mar 2018 10:28:03 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 3 lines

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K  

Applied to bpf-next, thanks Alexei!

Please wait till you have the proper acks. Some of this affects tracing.

Quoted 7 lines · Steven Rostedt

Quoted 4 lines · On Mon, 26 Mar 2018 10:28:03 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 3 lines

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K    

Quoted 4 lines

Quoted 1 line

Applied to bpf-next, thanks Alexei!

Please wait till you have the proper acks. Some of this affects tracing.

Ok, I thought time up to v5 was long enough. Anyway, in case there are objections I can still toss out the series from bpf-next tree worst case should e.g. follow-up fixups not be appropriate.

Quoted 13 lines · On Mon, 26 Mar 2018 17:32:02 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 11 lines · Steven Rostedt

Quoted 5 lines · On Mon, 26 Mar 2018 10:28:03 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 3 lines

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K

Applied to bpf-next, thanks Alexei!

Please wait till you have the proper acks. Some of this affects tracing.

Quoted 3 lines

Ok, I thought time up to v5 was long enough. Anyway, in case there are objections I can still toss out the series from bpf-next tree worst case should e.g. follow-up fixups not be appropriate.

Yeah, I've been traveling a bit which slowed down my review process (trying to catch up).

My main concern is with patch 6, as there are
external users of those functions. Although, we generally don't cater to out of tree code, we play nice with LTTng, and I don't want to break it.

I also should probably pull in the patches and run them through my tests to make sure they don't have any other side effects.

Quoted 17 lines · Steven Rostedt

On Mon, 26 Mar 2018 17:32:02 +0200
Daniel Borkmann <[email protected]>

Quoted 13 lines · Steven Rostedt

Quoted 11 lines

Quoted 5 lines · On Mon, 26 Mar 2018 10:28:03 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 3 lines

tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
task_rename   1.1M   769K        947K            1.0M
urandom_read  789K   697K        750K            755K  

Applied to bpf-next, thanks Alexei!

Please wait till you have the proper acks. Some of this affects tracing.

Quoted 6 lines

Quoted 3 lines

Ok, I thought time up to v5 was long enough. Anyway, in case there are objections I can still toss out the series from bpf-next tree worst case should e.g. follow-up fixups not be appropriate.

Yeah, I've been traveling a bit which slowed down my review process (trying to catch up).

v1 of this set was posted Feb 28.

imo one month is not an acceptable delay for maintainer to review the patches. You really need to consider group maintainership as we do with Daniel for bpf tree.

Quoted 5 lines

My main concern is with patch 6, as there are
external users of those functions. Although, we generally don't cater to out of tree code, we play nice with LTTng, and I don't want to break it.

out-of-tree module is out of tree. I'm beyond surprised that you propose to keep for_each_kernel_tracepoint() as-is with zero in-tree users in order to keep lttng working.

Quoted 3 lines

I also should probably pull in the patches and run them through my tests to make sure they don't have any other side effects.

so let me rephrase.
You're saying that a change to a function with zero in-tree users can somehow break your tests?
How is that possible?
Does it mean you also have some out-of-tree modules that will break? and that _is_ the real reason for objection?