[PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-Let-tracepoints-have-data-passed-to-tracepoi.patch --]
[-- Type: text/plain, Size: 16979 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

This patch allows data to be passed to the tracepoint callbacks
if the tracepoint was created to do so.

The DECLARE_TRACE() now adds two new functions:

	register_trace_mytracepoint_data()
	unregister_trace_mytracepoint_data()

These two are the same as the original

	register_trace_mytracepoint()
	unregister_trace_mytracepoint()

But now allow you to pass a private data pointer that will
be passed to the callback handle. For example:

DECLARE_TRACE(mytracepoint, int value, value);

will create a function called trace_mytracepoint()

	void trace_mytracepoint(int value);

If the user wants to pass data to register a function to this tracepoint
and have data also passed to this callback, they can use:

	int mycallback(int value, void *data);

	register_trace_mytracepoint_data(mycallback, mydata);

Then the mycallback() will receive the "mydata" as the parameter after
the args.

A more detailed example:

  DECLARE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));

  /* In the C file */

  DEFINE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));

  [...]

       trace_mytacepoint(status);

  /* In a file registering this tracepoint */

  int my_callback(int status, void *data)
  {
	struct my_struct my_data = data;
	[...]
  }

  [...]
	my_data = kmalloc(sizeof(*my_data), GFP_KERNEL);
	init_my_data(my_data);
	register_trace_mytracepoint_data(my_callback, my_data);

The same callback can also be registered to the same tracepoint as long
as the data registered is the different. Note, the data must also be used
to unregister the callback:

	unregister_trace_mytracepoint_data(my_callback, my_data);

Because of the data parameter, tracepoints declared this way can not have
no args. That is:

  DECLARE_TRACE(mytracepoint, TP_PROTO(void), TP_ARGS());

will cause an error.

If no arguments are needed, a new macro can be used instead:

  DECLARE_TRACE_NOARGS(mytracepoint);

Since there are no arguments, the proto and args fields are left out.

This is part of a series to make the tracepoint footprint smaller:

   text	   data	    bss	    dec	    hex	filename
5788186	1337252	9351592	16477030	 fb6b66	vmlinux.orig
5792282	1333796	9351592	16477670	 fb6de6	vmlinux.class
5793448	1333780	9351592	16478820	 fb7264	vmlinux.tracepoint

Again, this patch also increases the size of the kernel, but
lays the ground work for decreasing it.

 v2: Made the DECLARE_TRACE() have the ability to pass arguments
     and added a new DECLARE_TRACE_NOARGS() for tracepoints that
     do not need any arguments.

Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/tracepoint.h             |   94 +++++++++++++++++++++++++-------
 kernel/tracepoint.c                    |   91 +++++++++++++++++--------------
 samples/tracepoints/tp-samples-trace.h |    4 +-
 3 files changed, 126 insertions(+), 63 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 78b4bd3..ee8059a 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -20,12 +20,17 @@
 struct module;
 struct tracepoint;
 
+struct tracepoint_func {
+	void *func;
+	void *data;
+};
+
 struct tracepoint {
 	const char *name;		/* Tracepoint name */
 	int state;			/* State. */
 	void (*regfunc)(void);
 	void (*unregfunc)(void);
-	void **funcs;
+	struct tracepoint_func *funcs;
 } __attribute__((aligned(32)));		/*
 					 * Aligned on 32 bytes because it is
 					 * globally visible and gcc happily
@@ -46,14 +51,18 @@ struct tracepoint {
  */
 #define __DO_TRACE(tp, proto, args)					\
 	do {								\
-		void **it_func;						\
+		struct tracepoint_func *it_func_ptr;			\
+		void *it_func;						\
+		void *__data;						\
 									\
 		rcu_read_lock_sched_notrace();				\
-		it_func = rcu_dereference_sched((tp)->funcs);		\
-		if (it_func) {						\
+		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
+		if (it_func_ptr) {					\
 			do {						\
-				((void(*)(proto))(*it_func))(args);	\
-			} while (*(++it_func));				\
+				it_func = (it_func_ptr)->func;		\
+				__data = (it_func_ptr)->data;		\
+				((void(*)(proto))(it_func))(args);	\
+			} while ((++it_func_ptr)->func);		\
 		}							\
 		rcu_read_unlock_sched_notrace();			\
 	} while (0)
@@ -63,23 +72,47 @@ struct tracepoint {
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  */
-#define DECLARE_TRACE(name, proto, args)				\
+#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
 	extern struct tracepoint __tracepoint_##name;			\
 	static inline void trace_##name(proto)				\
 	{								\
 		if (unlikely(__tracepoint_##name.state))		\
 			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(proto), TP_ARGS(args));	\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args));			\
 	}								\
 	static inline int register_trace_##name(void (*probe)(proto))	\
 	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
+		return tracepoint_probe_register(#name, (void *)probe,	\
+						 NULL);			\
+	}								\
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{								\
+		return tracepoint_probe_unregister(#name, (void *)probe,\
+						   NULL);		\
 	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
+	static inline int						\
+	register_trace_##name##_data(void (*probe)(data_proto),		\
+				     void *data)			\
 	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
+		return tracepoint_probe_register(#name, (void *)probe,	\
+						 data);			\
+	}								\
+	static inline int						\
+	unregister_trace_##name##_data(void (*probe)(data_proto),	\
+				       void *data)			\
+	{								\
+		return tracepoint_probe_unregister(#name, (void *)probe,\
+						   data);		\
 	}
 
+#define DECLARE_TRACE_NOARGS(name)					\
+		__DECLARE_TRACE(name, void, , void *__data, __data)
+
+#define DECLARE_TRACE(name, proto, args)				\
+		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
+				PARAMS(proto, void *__data),		\
+				PARAMS(args, __data))
 
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
@@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
 	struct tracepoint *end);
 
 #else /* !CONFIG_TRACEPOINTS */
-#define DECLARE_TRACE(name, proto, args)				\
-	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
-	{ }								\
+#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
 	static inline void trace_##name(proto)				\
-	{ }								\
+	{								\
+	}								\
 	static inline int register_trace_##name(void (*probe)(proto))	\
 	{								\
 		return -ENOSYS;						\
 	}								\
-	static inline int unregister_trace_##name(void (*probe)(proto))	\
+	static inline int unregister_trace_##name(void (*probe)(proto)) \
+	{								\
+		return -ENOSYS;						\
+	}								\
+	static inline int						\
+	register_trace_##name##_data(void (*probe)(data_proto),		\
+				     void *data)			\
+	{								\
+		return -ENOSYS;						\
+	}								\
+	static inline int						\
+	unregister_trace_##name##_data(void (*probe)(data_proto),	\
+				       void *data)			\
 	{								\
 		return -ENOSYS;						\
 	}
+#define DECLARE_TRACE_NOARGS(name)					\
+		__DECLARE_TRACE(name, void, , void *__data, __data)
+
+#define DECLARE_TRACE(name, proto, args)				\
+		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
+				PARAMS(proto, void *__data),		\
+				PARAMS(args, __data))
 
 #define DEFINE_TRACE_FN(name, reg, unreg)
 #define DEFINE_TRACE(name)
@@ -129,16 +180,19 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
  * Connect a probe to a tracepoint.
  * Internal API, should not be used directly.
  */
-extern int tracepoint_probe_register(const char *name, void *probe);
+extern int tracepoint_probe_register(const char *name, void *probe, void *data);
 
 /*
  * Disconnect a probe from a tracepoint.
  * Internal API, should not be used directly.
  */
-extern int tracepoint_probe_unregister(const char *name, void *probe);
+extern int
+tracepoint_probe_unregister(const char *name, void *probe, void *data);
 
-extern int tracepoint_probe_register_noupdate(const char *name, void *probe);
-extern int tracepoint_probe_unregister_noupdate(const char *name, void *probe);
+extern int tracepoint_probe_register_noupdate(const char *name, void *probe,
+					      void *data);
+extern int tracepoint_probe_unregister_noupdate(const char *name, void *probe,
+						void *data);
 extern void tracepoint_probe_update_all(void);
 
 struct tracepoint_iter {
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index cc89be5..c77f3ec 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -54,7 +54,7 @@ static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
  */
 struct tracepoint_entry {
 	struct hlist_node hlist;
-	void **funcs;
+	struct tracepoint_func *funcs;
 	int refcount;	/* Number of times armed. 0 if disarmed. */
 	char name[0];
 };
@@ -64,12 +64,12 @@ struct tp_probes {
 		struct rcu_head rcu;
 		struct list_head list;
 	} u;
-	void *probes[0];
+	struct tracepoint_func probes[0];
 };
 
 static inline void *allocate_probes(int count)
 {
-	struct tp_probes *p  = kmalloc(count * sizeof(void *)
+	struct tp_probes *p  = kmalloc(count * sizeof(struct tracepoint_func)
 			+ sizeof(struct tp_probes), GFP_KERNEL);
 	return p == NULL ? NULL : p->probes;
 }
@@ -79,7 +79,7 @@ static void rcu_free_old_probes(struct rcu_head *head)
 	kfree(container_of(head, struct tp_probes, u.rcu));
 }
 
-static inline void release_probes(void *old)
+static inline void release_probes(struct tracepoint_func *old)
 {
 	if (old) {
 		struct tp_probes *tp_probes = container_of(old,
@@ -95,15 +95,16 @@ static void debug_print_probes(struct tracepoint_entry *entry)
 	if (!tracepoint_debug || !entry->funcs)
 		return;
 
-	for (i = 0; entry->funcs[i]; i++)
-		printk(KERN_DEBUG "Probe %d : %p\n", i, entry->funcs[i]);
+	for (i = 0; entry->funcs[i].func; i++)
+		printk(KERN_DEBUG "Probe %d : %p\n", i, entry->funcs[i].func);
 }
 
-static void *
-tracepoint_entry_add_probe(struct tracepoint_entry *entry, void *probe)
+static struct tracepoint_func *
+tracepoint_entry_add_probe(struct tracepoint_entry *entry,
+			   void *probe, void *data)
 {
 	int nr_probes = 0;
-	void **old, **new;
+	struct tracepoint_func *old, *new;
 
 	WARN_ON(!probe);
 
@@ -111,8 +112,9 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, void *probe)
 	old = entry->funcs;
 	if (old) {
 		/* (N -> N+1), (N != 0, 1) probes */
-		for (nr_probes = 0; old[nr_probes]; nr_probes++)
-			if (old[nr_probes] == probe)
+		for (nr_probes = 0; old[nr_probes].func; nr_probes++)
+			if (old[nr_probes].func == probe &&
+			    old[nr_probes].data == data)
 				return ERR_PTR(-EEXIST);
 	}
 	/* + 2 : one for new probe, one for NULL func */
@@ -120,9 +122,10 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, void *probe)
 	if (new == NULL)
 		return ERR_PTR(-ENOMEM);
 	if (old)
-		memcpy(new, old, nr_probes * sizeof(void *));
-	new[nr_probes] = probe;
-	new[nr_probes + 1] = NULL;
+		memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
+	new[nr_probes].func = probe;
+	new[nr_probes].data = data;
+	new[nr_probes + 1].func = NULL;
 	entry->refcount = nr_probes + 1;
 	entry->funcs = new;
 	debug_print_probes(entry);
@@ -130,10 +133,11 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, void *probe)
 }
 
 static void *
-tracepoint_entry_remove_probe(struct tracepoint_entry *entry, void *probe)
+tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
+			      void *probe, void *data)
 {
 	int nr_probes = 0, nr_del = 0, i;
-	void **old, **new;
+	struct tracepoint_func *old, *new;
 
 	old = entry->funcs;
 
@@ -142,8 +146,10 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry, void *probe)
 
 	debug_print_probes(entry);
 	/* (N -> M), (N > 1, M >= 0) probes */
-	for (nr_probes = 0; old[nr_probes]; nr_probes++) {
-		if ((!probe || old[nr_probes] == probe))
+	for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+		if (!probe ||
+		    (old[nr_probes].func == probe &&
+		     old[nr_probes].data == data))
 			nr_del++;
 	}
 
@@ -160,10 +166,11 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry, void *probe)
 		new = allocate_probes(nr_probes - nr_del + 1);
 		if (new == NULL)
 			return ERR_PTR(-ENOMEM);
-		for (i = 0; old[i]; i++)
-			if ((probe && old[i] != probe))
+		for (i = 0; old[i].func; i++)
+			if (probe &&
+			    (old[i].func != probe || old[i].data != data))
 				new[j++] = old[i];
-		new[nr_probes - nr_del] = NULL;
+		new[nr_probes - nr_del].func = NULL;
 		entry->refcount = nr_probes - nr_del;
 		entry->funcs = new;
 	}
@@ -315,18 +322,19 @@ static void tracepoint_update_probes(void)
 	module_update_tracepoints();
 }
 
-static void *tracepoint_add_probe(const char *name, void *probe)
+static struct tracepoint_func *
+tracepoint_add_probe(const char *name, void *probe, void *data)
 {
 	struct tracepoint_entry *entry;
-	void *old;
+	struct tracepoint_func *old;
 
 	entry = get_tracepoint(name);
 	if (!entry) {
 		entry = add_tracepoint(name);
 		if (IS_ERR(entry))
-			return entry;
+			return (struct tracepoint_func *)entry;
 	}
-	old = tracepoint_entry_add_probe(entry, probe);
+	old = tracepoint_entry_add_probe(entry, probe, data);
 	if (IS_ERR(old) && !entry->refcount)
 		remove_tracepoint(entry);
 	return old;
@@ -340,12 +348,12 @@ static void *tracepoint_add_probe(const char *name, void *probe)
  * Returns 0 if ok, error value on error.
  * The probe address must at least be aligned on the architecture pointer size.
  */
-int tracepoint_probe_register(const char *name, void *probe)
+int tracepoint_probe_register(const char *name, void *probe, void *data)
 {
-	void *old;
+	struct tracepoint_func *old;
 
 	mutex_lock(&tracepoints_mutex);
-	old = tracepoint_add_probe(name, probe);
+	old = tracepoint_add_probe(name, probe, data);
 	mutex_unlock(&tracepoints_mutex);
 	if (IS_ERR(old))
 		return PTR_ERR(old);
@@ -356,15 +364,16 @@ int tracepoint_probe_register(const char *name, void *probe)
 }
 EXPORT_SYMBOL_GPL(tracepoint_probe_register);
 
-static void *tracepoint_remove_probe(const char *name, void *probe)
+static struct tracepoint_func *
+tracepoint_remove_probe(const char *name, void *probe, void *data)
 {
 	struct tracepoint_entry *entry;
-	void *old;
+	struct tracepoint_func *old;
 
 	entry = get_tracepoint(name);
 	if (!entry)
 		return ERR_PTR(-ENOENT);
-	old = tracepoint_entry_remove_probe(entry, probe);
+	old = tracepoint_entry_remove_probe(entry, probe, data);
 	if (IS_ERR(old))
 		return old;
 	if (!entry->refcount)
@@ -382,12 +391,12 @@ static void *tracepoint_remove_probe(const char *name, void *probe)
  * itself uses stop_machine(), which insures that every preempt disabled section
  * have finished.
  */
-int tracepoint_probe_unregister(const char *name, void *probe)
+int tracepoint_probe_unregister(const char *name, void *probe, void *data)
 {
-	void *old;
+	struct tracepoint_func *old;
 
 	mutex_lock(&tracepoints_mutex);
-	old = tracepoint_remove_probe(name, probe);
+	old = tracepoint_remove_probe(name, probe, data);
 	mutex_unlock(&tracepoints_mutex);
 	if (IS_ERR(old))
 		return PTR_ERR(old);
@@ -418,12 +427,13 @@ static void tracepoint_add_old_probes(void *old)
  *
  * caller must call tracepoint_probe_update_all()
  */
-int tracepoint_probe_register_noupdate(const char *name, void *probe)
+int tracepoint_probe_register_noupdate(const char *name, void *probe,
+				       void *data)
 {
-	void *old;
+	struct tracepoint_func *old;
 
 	mutex_lock(&tracepoints_mutex);
-	old = tracepoint_add_probe(name, probe);
+	old = tracepoint_add_probe(name, probe, data);
 	if (IS_ERR(old)) {
 		mutex_unlock(&tracepoints_mutex);
 		return PTR_ERR(old);
@@ -441,12 +451,13 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_register_noupdate);
  *
  * caller must call tracepoint_probe_update_all()
  */
-int tracepoint_probe_unregister_noupdate(const char *name, void *probe)
+int tracepoint_probe_unregister_noupdate(const char *name, void *probe,
+					 void *data)
 {
-	void *old;
+	struct tracepoint_func *old;
 
 	mutex_lock(&tracepoints_mutex);
-	old = tracepoint_remove_probe(name, probe);
+	old = tracepoint_remove_probe(name, probe, data);
 	if (IS_ERR(old)) {
 		mutex_unlock(&tracepoints_mutex);
 		return PTR_ERR(old);
diff --git a/samples/tracepoints/tp-samples-trace.h b/samples/tracepoints/tp-samples-trace.h
index dffdc49..4d46be9 100644
--- a/samples/tracepoints/tp-samples-trace.h
+++ b/samples/tracepoints/tp-samples-trace.h
@@ -7,7 +7,5 @@
 DECLARE_TRACE(subsys_event,
 	TP_PROTO(struct inode *inode, struct file *file),
 	TP_ARGS(inode, file));
-DECLARE_TRACE(subsys_eventb,
-	TP_PROTO(void),
-	TP_ARGS());
+DECLARE_TRACE_NOARGS(subsys_eventb);
 #endif
-- 
1.7.0

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Frederic Weisbecker]

On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> This patch allows data to be passed to the tracepoint callbacks
> if the tracepoint was created to do so.
> 
> The DECLARE_TRACE() now adds two new functions:
> 
> 	register_trace_mytracepoint_data()
> 	unregister_trace_mytracepoint_data()
> 
> These two are the same as the original
> 
> 	register_trace_mytracepoint()
> 	unregister_trace_mytracepoint()
> 
> But now allow you to pass a private data pointer that will
> be passed to the callback handle. For example:
> 
> DECLARE_TRACE(mytracepoint, int value, value);
> 
> will create a function called trace_mytracepoint()
> 
> 	void trace_mytracepoint(int value);
> 
> If the user wants to pass data to register a function to this tracepoint
> and have data also passed to this callback, they can use:
> 
> 	int mycallback(int value, void *data);
> 
> 	register_trace_mytracepoint_data(mycallback, mydata);
> 
> Then the mycallback() will receive the "mydata" as the parameter after
> the args.
> 
> A more detailed example:
> 
>   DECLARE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
> 
>   /* In the C file */
> 
>   DEFINE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
> 
>   [...]
> 
>        trace_mytacepoint(status);
> 
>   /* In a file registering this tracepoint */
> 
>   int my_callback(int status, void *data)
>   {
> 	struct my_struct my_data = data;
> 	[...]
>   }
> 
>   [...]
> 	my_data = kmalloc(sizeof(*my_data), GFP_KERNEL);
> 	init_my_data(my_data);
> 	register_trace_mytracepoint_data(my_callback, my_data);
> 
> The same callback can also be registered to the same tracepoint as long
> as the data registered is the different. Note, the data must also be used
> to unregister the callback:
> 
> 	unregister_trace_mytracepoint_data(my_callback, my_data);
> 
> Because of the data parameter, tracepoints declared this way can not have
> no args. That is:
> 
>   DECLARE_TRACE(mytracepoint, TP_PROTO(void), TP_ARGS());
> 
> will cause an error.
> 
> If no arguments are needed, a new macro can be used instead:
> 
>   DECLARE_TRACE_NOARGS(mytracepoint);
> 
> Since there are no arguments, the proto and args fields are left out.
> 
> This is part of a series to make the tracepoint footprint smaller:
> 
>    text	   data	    bss	    dec	    hex	filename
> 5788186	1337252	9351592	16477030	 fb6b66	vmlinux.orig
> 5792282	1333796	9351592	16477670	 fb6de6	vmlinux.class
> 5793448	1333780	9351592	16478820	 fb7264	vmlinux.tracepoint
> 
> Again, this patch also increases the size of the kernel, but
> lays the ground work for decreasing it.
> 
>  v2: Made the DECLARE_TRACE() have the ability to pass arguments
>      and added a new DECLARE_TRACE_NOARGS() for tracepoints that
>      do not need any arguments.
> 
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/linux/tracepoint.h             |   94 +++++++++++++++++++++++++-------
>  kernel/tracepoint.c                    |   91 +++++++++++++++++--------------
>  samples/tracepoints/tp-samples-trace.h |    4 +-
>  3 files changed, 126 insertions(+), 63 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 78b4bd3..ee8059a 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -20,12 +20,17 @@
>  struct module;
>  struct tracepoint;
>  
> +struct tracepoint_func {
> +	void *func;
> +	void *data;
> +};
> +
>  struct tracepoint {
>  	const char *name;		/* Tracepoint name */
>  	int state;			/* State. */
>  	void (*regfunc)(void);
>  	void (*unregfunc)(void);
> -	void **funcs;
> +	struct tracepoint_func *funcs;
>  } __attribute__((aligned(32)));		/*
>  					 * Aligned on 32 bytes because it is
>  					 * globally visible and gcc happily
> @@ -46,14 +51,18 @@ struct tracepoint {
>   */
>  #define __DO_TRACE(tp, proto, args)					\
>  	do {								\
> -		void **it_func;						\
> +		struct tracepoint_func *it_func_ptr;			\
> +		void *it_func;						\
> +		void *__data;						\
>  									\
>  		rcu_read_lock_sched_notrace();				\
> -		it_func = rcu_dereference_sched((tp)->funcs);		\
> -		if (it_func) {						\
> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> +		if (it_func_ptr) {					\
>  			do {						\
> -				((void(*)(proto))(*it_func))(args);	\
> -			} while (*(++it_func));				\
> +				it_func = (it_func_ptr)->func;		\
> +				__data = (it_func_ptr)->data;		\
> +				((void(*)(proto))(it_func))(args);	\


So, we had a talk about this and we concluded that it is probably fine
on every archs to push one more argument than needed in a function.

But I think it would be nice to add a comment about this. Firstly
because this line breaks all the self-explanation of the code, I mean
I tried hard to find how the non-data callback case was handled :)
Secondly to also to avoid people asking what happens here.




> +			} while ((++it_func_ptr)->func);		\
>  		}							\
>  		rcu_read_unlock_sched_notrace();			\
>  	} while (0)
> @@ -63,23 +72,47 @@ struct tracepoint {
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   */
> -#define DECLARE_TRACE(name, proto, args)				\
> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
>  	extern struct tracepoint __tracepoint_##name;			\
>  	static inline void trace_##name(proto)				\
>  	{								\
>  		if (unlikely(__tracepoint_##name.state))		\
>  			__DO_TRACE(&__tracepoint_##name,		\
> -				TP_PROTO(proto), TP_ARGS(args));	\
> +				TP_PROTO(data_proto),			\
> +				TP_ARGS(data_args));			\
>  	}								\
>  	static inline int register_trace_##name(void (*probe)(proto))	\
>  	{								\
> -		return tracepoint_probe_register(#name, (void *)probe);	\
> +		return tracepoint_probe_register(#name, (void *)probe,	\
> +						 NULL);			\
> +	}								\
> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> +	{								\
> +		return tracepoint_probe_unregister(#name, (void *)probe,\
> +						   NULL);		\
>  	}								\
> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> +	static inline int						\
> +	register_trace_##name##_data(void (*probe)(data_proto),		\
> +				     void *data)			\
>  	{								\
> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> +		return tracepoint_probe_register(#name, (void *)probe,	\
> +						 data);			\
> +	}								\
> +	static inline int						\
> +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> +				       void *data)			\
> +	{								\
> +		return tracepoint_probe_unregister(#name, (void *)probe,\
> +						   data);		\
>  	}
>  
> +#define DECLARE_TRACE_NOARGS(name)					\
> +		__DECLARE_TRACE(name, void, , void *__data, __data)



That too, may be, deserves a small comment :)



> +
> +#define DECLARE_TRACE(name, proto, args)				\
> +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> +				PARAMS(proto, void *__data),		\
> +				PARAMS(args, __data))
>  
>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
>  	static const char __tpstrtab_##name[]				\
> @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>  	struct tracepoint *end);
>  
>  #else /* !CONFIG_TRACEPOINTS */
> -#define DECLARE_TRACE(name, proto, args)				\
> -	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> -	{ }								\
> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
>  	static inline void trace_##name(proto)				\
> -	{ }								\
> +	{								\
> +	}								\
>  	static inline int register_trace_##name(void (*probe)(proto))	\
>  	{								\
>  		return -ENOSYS;						\
>  	}								\
> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> +	{								\
> +		return -ENOSYS;						\
> +	}								\
> +	static inline int						\
> +	register_trace_##name##_data(void (*probe)(data_proto),		\
> +				     void *data)			\
> +	{								\
> +		return -ENOSYS;						\
> +	}								\
> +	static inline int						\
> +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> +				       void *data)			\
>  	{								\
>  		return -ENOSYS;						\
>  	}
> +#define DECLARE_TRACE_NOARGS(name)					\
> +		__DECLARE_TRACE(name, void, , void *__data, __data)
> +
> +#define DECLARE_TRACE(name, proto, args)				\
> +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> +				PARAMS(proto, void *__data),		\
> +				PARAMS(args, __data))




It seems that the on and off cases are exactly the same for DECLARE_TRACE*(),
you could provide a single version and let the __DECLARE_TRACE() do
the on/off trick.

Thanks.

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 9336 bytes --]



"Frederic Weisbecker" <fweisbec@gmail.com> wrote:

>On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
>> From: Steven Rostedt <srostedt@redhat.com>
>> 
>> This patch allows data to be passed to the tracepoint callbacks
>> if the tracepoint was created to do so.
>> 
>> The DECLARE_TRACE() now adds two new functions:
>> 
>> 	register_trace_mytracepoint_data()
>> 	unregister_trace_mytracepoint_data()
>> 
>> These two are the same as the original
>> 
>> 	register_trace_mytracepoint()
>> 	unregister_trace_mytracepoint()
>> 
>> But now allow you to pass a private data pointer that will
>> be passed to the callback handle. For example:
>> 
>> DECLARE_TRACE(mytracepoint, int value, value);
>> 
>> will create a function called trace_mytracepoint()
>> 
>> 	void trace_mytracepoint(int value);
>> 
>> If the user wants to pass data to register a function to this tracepoint
>> and have data also passed to this callback, they can use:
>> 
>> 	int mycallback(int value, void *data);
>> 
>> 	register_trace_mytracepoint_data(mycallback, mydata);
>> 
>> Then the mycallback() will receive the "mydata" as the parameter after
>> the args.
>> 
>> A more detailed example:
>> 
>>   DECLARE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
>> 
>>   /* In the C file */
>> 
>>   DEFINE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
>> 
>>   [...]
>> 
>>        trace_mytacepoint(status);
>> 
>>   /* In a file registering this tracepoint */
>> 
>>   int my_callback(int status, void *data)
>>   {
>> 	struct my_struct my_data = data;
>> 	[...]
>>   }
>> 
>>   [...]
>> 	my_data = kmalloc(sizeof(*my_data), GFP_KERNEL);
>> 	init_my_data(my_data);
>> 	register_trace_mytracepoint_data(my_callback, my_data);
>> 
>> The same callback can also be registered to the same tracepoint as long
>> as the data registered is the different. Note, the data must also be used
>> to unregister the callback:
>> 
>> 	unregister_trace_mytracepoint_data(my_callback, my_data);
>> 
>> Because of the data parameter, tracepoints declared this way can not have
>> no args. That is:
>> 
>>   DECLARE_TRACE(mytracepoint, TP_PROTO(void), TP_ARGS());
>> 
>> will cause an error.
>> 
>> If no arguments are needed, a new macro can be used instead:
>> 
>>   DECLARE_TRACE_NOARGS(mytracepoint);
>> 
>> Since there are no arguments, the proto and args fields are left out.
>> 
>> This is part of a series to make the tracepoint footprint smaller:
>> 
>>    text	   data	    bss	    dec	    hex	filename
>> 5788186	1337252	9351592	16477030	 fb6b66	vmlinux.orig
>> 5792282	1333796	9351592	16477670	 fb6de6	vmlinux.class
>> 5793448	1333780	9351592	16478820	 fb7264	vmlinux.tracepoint
>> 
>> Again, this patch also increases the size of the kernel, but
>> lays the ground work for decreasing it.
>> 
>>  v2: Made the DECLARE_TRACE() have the ability to pass arguments
>>      and added a new DECLARE_TRACE_NOARGS() for tracepoints that
>>      do not need any arguments.
>> 
>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>> ---
>>  include/linux/tracepoint.h             |   94 +++++++++++++++++++++++++-------
>>  kernel/tracepoint.c                    |   91 +++++++++++++++++--------------
>>  samples/tracepoints/tp-samples-trace.h |    4 +-
>>  3 files changed, 126 insertions(+), 63 deletions(-)
>> 
>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>> index 78b4bd3..ee8059a 100644
>> --- a/include/linux/tracepoint.h
>> +++ b/include/linux/tracepoint.h
>> @@ -20,12 +20,17 @@
>>  struct module;
>>  struct tracepoint;
>>  
>> +struct tracepoint_func {
>> +	void *func;
>> +	void *data;
>> +};
>> +
>>  struct tracepoint {
>>  	const char *name;		/* Tracepoint name */
>>  	int state;			/* State. */
>>  	void (*regfunc)(void);
>>  	void (*unregfunc)(void);
>> -	void **funcs;
>> +	struct tracepoint_func *funcs;
>>  } __attribute__((aligned(32)));		/*
>>  					 * Aligned on 32 bytes because it is
>>  					 * globally visible and gcc happily
>> @@ -46,14 +51,18 @@ struct tracepoint {
>>   */
>>  #define __DO_TRACE(tp, proto, args)					\
>>  	do {								\
>> -		void **it_func;						\
>> +		struct tracepoint_func *it_func_ptr;			\
>> +		void *it_func;						\
>> +		void *__data;						\
>>  									\
>>  		rcu_read_lock_sched_notrace();				\
>> -		it_func = rcu_dereference_sched((tp)->funcs);		\
>> -		if (it_func) {						\
>> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
>> +		if (it_func_ptr) {					\
>>  			do {						\
>> -				((void(*)(proto))(*it_func))(args);	\
>> -			} while (*(++it_func));				\
>> +				it_func = (it_func_ptr)->func;		\
>> +				__data = (it_func_ptr)->data;		\
>> +				((void(*)(proto))(it_func))(args);	\
>
>
>So, we had a talk about this and we concluded that it is probably fine
>on every archs to push one more argument than needed in a function.
>

Yeah, I'm hoping it's fine.

>But I think it would be nice to add a comment about this. Firstly
>because this line breaks all the self-explanation of the code, I mean
>I tried hard to find how the non-data callback case was handled :)
>Secondly to also to avoid people asking what happens here.
>

OK I'll add a bit of comments to the macros. So much for my job security ;-)

>
>
>
>> +			} while ((++it_func_ptr)->func);		\
>>  		}							\
>>  		rcu_read_unlock_sched_notrace();			\
>>  	} while (0)
>> @@ -63,23 +72,47 @@ struct tracepoint {
>>   * not add unwanted padding between the beginning of the section and the
>>   * structure. Force alignment to the same alignment as the section start.
>>   */
>> -#define DECLARE_TRACE(name, proto, args)				\
>> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
>>  	extern struct tracepoint __tracepoint_##name;			\
>>  	static inline void trace_##name(proto)				\
>>  	{								\
>>  		if (unlikely(__tracepoint_##name.state))		\
>>  			__DO_TRACE(&__tracepoint_##name,		\
>> -				TP_PROTO(proto), TP_ARGS(args));	\
>> +				TP_PROTO(data_proto),			\
>> +				TP_ARGS(data_args));			\
>>  	}								\
>>  	static inline int register_trace_##name(void (*probe)(proto))	\
>>  	{								\
>> -		return tracepoint_probe_register(#name, (void *)probe);	\
>> +		return tracepoint_probe_register(#name, (void *)probe,	\
>> +						 NULL);			\
>> +	}								\
>> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
>> +	{								\
>> +		return tracepoint_probe_unregister(#name, (void *)probe,\
>> +						   NULL);		\
>>  	}								\
>> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
>> +	static inline int						\
>> +	register_trace_##name##_data(void (*probe)(data_proto),		\
>> +				     void *data)			\
>>  	{								\
>> -		return tracepoint_probe_unregister(#name, (void *)probe);\
>> +		return tracepoint_probe_register(#name, (void *)probe,	\
>> +						 data);			\
>> +	}								\
>> +	static inline int						\
>> +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
>> +				       void *data)			\
>> +	{								\
>> +		return tracepoint_probe_unregister(#name, (void *)probe,\
>> +						   data);		\
>>  	}
>>  
>> +#define DECLARE_TRACE_NOARGS(name)					\
>> +		__DECLARE_TRACE(name, void, , void *__data, __data)
>
>
>
>That too, may be, deserves a small comment :)

OK

>
>
>
>> +
>> +#define DECLARE_TRACE(name, proto, args)				\
>> +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
>> +				PARAMS(proto, void *__data),		\
>> +				PARAMS(args, __data))
>>  
>>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
>>  	static const char __tpstrtab_##name[]				\
>> @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>>  	struct tracepoint *end);
>>  
>>  #else /* !CONFIG_TRACEPOINTS */
>> -#define DECLARE_TRACE(name, proto, args)				\
>> -	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
>> -	{ }								\
>> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
>>  	static inline void trace_##name(proto)				\
>> -	{ }								\
>> +	{								\
>> +	}								\
>>  	static inline int register_trace_##name(void (*probe)(proto))	\
>>  	{								\
>>  		return -ENOSYS;						\
>>  	}								\
>> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
>> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
>> +	{								\
>> +		return -ENOSYS;						\
>> +	}								\
>> +	static inline int						\
>> +	register_trace_##name##_data(void (*probe)(data_proto),		\
>> +				     void *data)			\
>> +	{								\
>> +		return -ENOSYS;						\
>> +	}								\
>> +	static inline int						\
>> +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
>> +				       void *data)			\
>>  	{								\
>>  		return -ENOSYS;						\
>>  	}
>> +#define DECLARE_TRACE_NOARGS(name)					\
>> +		__DECLARE_TRACE(name, void, , void *__data, __data)
>> +
>> +#define DECLARE_TRACE(name, proto, args)				\
>> +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
>> +				PARAMS(proto, void *__data),		\
>> +				PARAMS(args, __data))
>
>
>
>
>It seems that the on and off cases are exactly the same for DECLARE_TRACE*(),
>you could provide a single version and let the __DECLARE_TRACE() do
>the on/off trick.
>

I don't know what you mean here?

-- Steve 

>Thanks.
>

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Steven Rostedt]

Hmm, I replied originally from my Google phone, but I don't see it in
LKML. So apologies if this is duplicate.


On Fri, 2010-05-07 at 05:52 +0200, Frederic Weisbecker wrote:
> On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:

> >  #define __DO_TRACE(tp, proto, args)					\
> >  	do {								\
> > -		void **it_func;						\
> > +		struct tracepoint_func *it_func_ptr;			\
> > +		void *it_func;						\
> > +		void *__data;						\
> >  									\
> >  		rcu_read_lock_sched_notrace();				\
> > -		it_func = rcu_dereference_sched((tp)->funcs);		\
> > -		if (it_func) {						\
> > +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> > +		if (it_func_ptr) {					\
> >  			do {						\
> > -				((void(*)(proto))(*it_func))(args);	\
> > -			} while (*(++it_func));				\
> > +				it_func = (it_func_ptr)->func;		\
> > +				__data = (it_func_ptr)->data;		\
> > +				((void(*)(proto))(it_func))(args);	\
> 
> 
> So, we had a talk about this and we concluded that it is probably fine
> on every archs to push one more argument than needed in a function.

Yep, I'm hoping this is the case.

> 
> But I think it would be nice to add a comment about this. Firstly
> because this line breaks all the self-explanation of the code, I mean
> I tried hard to find how the non-data callback case was handled :)
> Secondly to also to avoid people asking what happens here.

OK, I'll add documentation here. So much for my job security ;-)

> 
> 
> 
> 
> > +			} while ((++it_func_ptr)->func);		\
> >  		}							\
> >  		rcu_read_unlock_sched_notrace();			\
> >  	} while (0)
> > @@ -63,23 +72,47 @@ struct tracepoint {
> >   * not add unwanted padding between the beginning of the section and the
> >   * structure. Force alignment to the same alignment as the section start.
> >   */
> > -#define DECLARE_TRACE(name, proto, args)				\
> > +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
> >  	extern struct tracepoint __tracepoint_##name;			\
> >  	static inline void trace_##name(proto)				\
> >  	{								\
> >  		if (unlikely(__tracepoint_##name.state))		\
> >  			__DO_TRACE(&__tracepoint_##name,		\
> > -				TP_PROTO(proto), TP_ARGS(args));	\
> > +				TP_PROTO(data_proto),			\
> > +				TP_ARGS(data_args));			\
> >  	}								\
> >  	static inline int register_trace_##name(void (*probe)(proto))	\
> >  	{								\
> > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > +		return tracepoint_probe_register(#name, (void *)probe,	\
> > +						 NULL);			\
> > +	}								\
> > +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> > +	{								\
> > +		return tracepoint_probe_unregister(#name, (void *)probe,\
> > +						   NULL);		\
> >  	}								\
> > -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> > +	static inline int						\
> > +	register_trace_##name##_data(void (*probe)(data_proto),		\
> > +				     void *data)			\
> >  	{								\
> > -		return tracepoint_probe_unregister(#name, (void *)probe);\
> > +		return tracepoint_probe_register(#name, (void *)probe,	\
> > +						 data);			\
> > +	}								\
> > +	static inline int						\
> > +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> > +				       void *data)			\
> > +	{								\
> > +		return tracepoint_probe_unregister(#name, (void *)probe,\
> > +						   data);		\
> >  	}
> >  
> > +#define DECLARE_TRACE_NOARGS(name)					\
> > +		__DECLARE_TRACE(name, void, , void *__data, __data)
> 
> 
> 
> That too, may be, deserves a small comment :)

OK

> 
> 
> 
> > +
> > +#define DECLARE_TRACE(name, proto, args)				\
> > +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> > +				PARAMS(proto, void *__data),		\
> > +				PARAMS(args, __data))
> >  
> >  #define DEFINE_TRACE_FN(name, reg, unreg)				\
> >  	static const char __tpstrtab_##name[]				\
> > @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> >  	struct tracepoint *end);
> >  
> >  #else /* !CONFIG_TRACEPOINTS */
> > -#define DECLARE_TRACE(name, proto, args)				\
> > -	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> > -	{ }								\
> > +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
> >  	static inline void trace_##name(proto)				\
> > -	{ }								\
> > +	{								\
> > +	}								\
> >  	static inline int register_trace_##name(void (*probe)(proto))	\
> >  	{								\
> >  		return -ENOSYS;						\
> >  	}								\
> > -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> > +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> > +	{								\
> > +		return -ENOSYS;						\
> > +	}								\
> > +	static inline int						\
> > +	register_trace_##name##_data(void (*probe)(data_proto),		\
> > +				     void *data)			\
> > +	{								\
> > +		return -ENOSYS;						\
> > +	}								\
> > +	static inline int						\
> > +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> > +				       void *data)			\
> >  	{								\
> >  		return -ENOSYS;						\
> >  	}
> > +#define DECLARE_TRACE_NOARGS(name)					\
> > +		__DECLARE_TRACE(name, void, , void *__data, __data)
> > +
> > +#define DECLARE_TRACE(name, proto, args)				\
> > +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> > +				PARAMS(proto, void *__data),		\
> > +				PARAMS(args, __data))
> 
> 
> 
> 
> It seems that the on and off cases are exactly the same for DECLARE_TRACE*(),
> you could provide a single version and let the __DECLARE_TRACE() do
> the on/off trick.


I don't know what you mean here. How would __DECLARE_TRACE() do what
both DECLARE_TRACE() and DECLARE_TRACE_NOARGS() do? It will fail the
compile if proto is "void".

-- Steve

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> 
> "Frederic Weisbecker" <fweisbec@gmail.com> wrote:
> 
> >On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
[...]
> >> 
> >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> >> index 78b4bd3..ee8059a 100644
> >> --- a/include/linux/tracepoint.h
> >> +++ b/include/linux/tracepoint.h
> >> @@ -20,12 +20,17 @@
> >>  struct module;
> >>  struct tracepoint;
> >>  
> >> +struct tracepoint_func {
> >> +	void *func;
> >> +	void *data;
> >> +};
> >> +
> >>  struct tracepoint {
> >>  	const char *name;		/* Tracepoint name */
> >>  	int state;			/* State. */
> >>  	void (*regfunc)(void);
> >>  	void (*unregfunc)(void);
> >> -	void **funcs;
> >> +	struct tracepoint_func *funcs;
> >>  } __attribute__((aligned(32)));		/*
> >>  					 * Aligned on 32 bytes because it is
> >>  					 * globally visible and gcc happily
> >> @@ -46,14 +51,18 @@ struct tracepoint {
> >>   */
> >>  #define __DO_TRACE(tp, proto, args)					\
> >>  	do {								\
> >> -		void **it_func;						\
> >> +		struct tracepoint_func *it_func_ptr;			\
> >> +		void *it_func;						\
> >> +		void *__data;						\
> >>  									\
> >>  		rcu_read_lock_sched_notrace();				\
> >> -		it_func = rcu_dereference_sched((tp)->funcs);		\
> >> -		if (it_func) {						\
> >> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> >> +		if (it_func_ptr) {					\
> >>  			do {						\
> >> -				((void(*)(proto))(*it_func))(args);	\
> >> -			} while (*(++it_func));				\
> >> +				it_func = (it_func_ptr)->func;		\
> >> +				__data = (it_func_ptr)->data;		\
> >> +				((void(*)(proto))(it_func))(args);	\
> >
> >
> >So, we had a talk about this and we concluded that it is probably fine
> >on every archs to push one more argument than needed in a function.
> >
> 
> Yeah, I'm hoping it's fine.

How about changing the callback prototypes to match the call arguments (changing
the type expected in register/unregister_trace, as well as an additional "check
type" that I proposed for Ftrace) ?

Otherwise, you basically expect here that:

void fct(void *foo, void *bar, etc etc) (N parameters expected)
{

}

called by:

fct(foo, bar, etc etc, foobar) (N + 1 parameters)

will always work.

Can you show me where the C standard says it is safe to do so ?

Thanks,

Mathieu

> 
> >But I think it would be nice to add a comment about this. Firstly
> >because this line breaks all the self-explanation of the code, I mean
> >I tried hard to find how the non-data callback case was handled :)
> >Secondly to also to avoid people asking what happens here.
> >
> 
> OK I'll add a bit of comments to the macros. So much for my job security ;-)
> 
> >
> >
> >
> >> +			} while ((++it_func_ptr)->func);		\
> >>  		}							\
> >>  		rcu_read_unlock_sched_notrace();			\
> >>  	} while (0)
> >> @@ -63,23 +72,47 @@ struct tracepoint {
> >>   * not add unwanted padding between the beginning of the section and the
> >>   * structure. Force alignment to the same alignment as the section start.
> >>   */
> >> -#define DECLARE_TRACE(name, proto, args)				\
> >> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
> >>  	extern struct tracepoint __tracepoint_##name;			\
> >>  	static inline void trace_##name(proto)				\
> >>  	{								\
> >>  		if (unlikely(__tracepoint_##name.state))		\
> >>  			__DO_TRACE(&__tracepoint_##name,		\
> >> -				TP_PROTO(proto), TP_ARGS(args));	\
> >> +				TP_PROTO(data_proto),			\
> >> +				TP_ARGS(data_args));			\
> >>  	}								\
> >>  	static inline int register_trace_##name(void (*probe)(proto))	\
> >>  	{								\
> >> -		return tracepoint_probe_register(#name, (void *)probe);	\
> >> +		return tracepoint_probe_register(#name, (void *)probe,	\
> >> +						 NULL);			\
> >> +	}								\
> >> +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> >> +	{								\
> >> +		return tracepoint_probe_unregister(#name, (void *)probe,\
> >> +						   NULL);		\
> >>  	}								\
> >> -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> >> +	static inline int						\
> >> +	register_trace_##name##_data(void (*probe)(data_proto),		\
> >> +				     void *data)			\
> >>  	{								\
> >> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> >> +		return tracepoint_probe_register(#name, (void *)probe,	\
> >> +						 data);			\
> >> +	}								\
> >> +	static inline int						\
> >> +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> >> +				       void *data)			\
> >> +	{								\
> >> +		return tracepoint_probe_unregister(#name, (void *)probe,\
> >> +						   data);		\
> >>  	}
> >>  
> >> +#define DECLARE_TRACE_NOARGS(name)					\
> >> +		__DECLARE_TRACE(name, void, , void *__data, __data)
> >
> >
> >
> >That too, may be, deserves a small comment :)
> 
> OK
> 
> >
> >
> >
> >> +
> >> +#define DECLARE_TRACE(name, proto, args)				\
> >> +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> >> +				PARAMS(proto, void *__data),		\
> >> +				PARAMS(args, __data))
> >>  
> >>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
> >>  	static const char __tpstrtab_##name[]				\
> >> @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> >>  	struct tracepoint *end);
> >>  
[...]

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Steven Rostedt]

On Fri, 2010-05-07 at 10:39 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > 
> > 
> > "Frederic Weisbecker" <fweisbec@gmail.com> wrote:
> > 
> > >On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> [...]
> > >> 
> > >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > >> index 78b4bd3..ee8059a 100644
> > >> --- a/include/linux/tracepoint.h
> > >> +++ b/include/linux/tracepoint.h
> > >> @@ -20,12 +20,17 @@
> > >>  struct module;
> > >>  struct tracepoint;
> > >>  
> > >> +struct tracepoint_func {
> > >> +	void *func;
> > >> +	void *data;
> > >> +};
> > >> +
> > >>  struct tracepoint {
> > >>  	const char *name;		/* Tracepoint name */
> > >>  	int state;			/* State. */
> > >>  	void (*regfunc)(void);
> > >>  	void (*unregfunc)(void);
> > >> -	void **funcs;
> > >> +	struct tracepoint_func *funcs;
> > >>  } __attribute__((aligned(32)));		/*
> > >>  					 * Aligned on 32 bytes because it is
> > >>  					 * globally visible and gcc happily
> > >> @@ -46,14 +51,18 @@ struct tracepoint {
> > >>   */
> > >>  #define __DO_TRACE(tp, proto, args)					\
> > >>  	do {								\
> > >> -		void **it_func;						\
> > >> +		struct tracepoint_func *it_func_ptr;			\
> > >> +		void *it_func;						\
> > >> +		void *__data;						\
> > >>  									\
> > >>  		rcu_read_lock_sched_notrace();				\
> > >> -		it_func = rcu_dereference_sched((tp)->funcs);		\
> > >> -		if (it_func) {						\
> > >> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> > >> +		if (it_func_ptr) {					\
> > >>  			do {						\
> > >> -				((void(*)(proto))(*it_func))(args);	\
> > >> -			} while (*(++it_func));				\
> > >> +				it_func = (it_func_ptr)->func;		\
> > >> +				__data = (it_func_ptr)->data;		\
> > >> +				((void(*)(proto))(it_func))(args);	\
> > >
> > >
> > >So, we had a talk about this and we concluded that it is probably fine
> > >on every archs to push one more argument than needed in a function.
> > >
> > 
> > Yeah, I'm hoping it's fine.
> 
> How about changing the callback prototypes to match the call arguments (changing
> the type expected in register/unregister_trace, as well as an additional "check
> type" that I proposed for Ftrace) ?

This can not happen!!!! As I said before, the register is done in C,
there is no macro that will help here. We create the call back with the
macro, but the registering is in kernel/trace/trace_events.c. One
register for ___ALL___ events!!!

 Thus there is no check.

Understand this yet?


> 
> Otherwise, you basically expect here that:
> 
> void fct(void *foo, void *bar, etc etc) (N parameters expected)
> {
> 
> }
> 
> called by:
> 
> fct(foo, bar, etc etc, foobar) (N + 1 parameters)
> 
> will always work.
> 
> Can you show me where the C standard says it is safe to do so ?

No, but it seems safe in the kernel ;-)

But that said. There is another option that will conform to this, and
that is to add flags to registering tracepoints. I already wrote a patch
for this in trying to do some other work (that I threw away).


So here's the proposal.

Change struct tracepoint_func to...

struct tracepoint_func {
	void *func;
	void *data;
	unsigned int flags;
};


The flags is set when registered. If a function is registered with data,
then the flags field will be set. Then the calling of the function can
be:

	if ((it_func_ptr)->flags & TP_FL_DATA)
		((void(*)(proto, void *))(it_func)(args, __data);
	else
		((void(*)(proto))(it_func)(args);

This would comply with the C standard.

-- Steve

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2010-05-07 at 10:39 -0400, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > 
> > > 
> > > "Frederic Weisbecker" <fweisbec@gmail.com> wrote:
> > > 
> > > >On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> > [...]
> > > >> 
> > > >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > >> index 78b4bd3..ee8059a 100644
> > > >> --- a/include/linux/tracepoint.h
> > > >> +++ b/include/linux/tracepoint.h
> > > >> @@ -20,12 +20,17 @@
> > > >>  struct module;
> > > >>  struct tracepoint;
> > > >>  
> > > >> +struct tracepoint_func {
> > > >> +	void *func;
> > > >> +	void *data;
> > > >> +};
> > > >> +
> > > >>  struct tracepoint {
> > > >>  	const char *name;		/* Tracepoint name */
> > > >>  	int state;			/* State. */
> > > >>  	void (*regfunc)(void);
> > > >>  	void (*unregfunc)(void);
> > > >> -	void **funcs;
> > > >> +	struct tracepoint_func *funcs;
> > > >>  } __attribute__((aligned(32)));		/*
> > > >>  					 * Aligned on 32 bytes because it is
> > > >>  					 * globally visible and gcc happily
> > > >> @@ -46,14 +51,18 @@ struct tracepoint {
> > > >>   */
> > > >>  #define __DO_TRACE(tp, proto, args)					\
> > > >>  	do {								\
> > > >> -		void **it_func;						\
> > > >> +		struct tracepoint_func *it_func_ptr;			\
> > > >> +		void *it_func;						\
> > > >> +		void *__data;						\
> > > >>  									\
> > > >>  		rcu_read_lock_sched_notrace();				\
> > > >> -		it_func = rcu_dereference_sched((tp)->funcs);		\
> > > >> -		if (it_func) {						\
> > > >> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> > > >> +		if (it_func_ptr) {					\
> > > >>  			do {						\
> > > >> -				((void(*)(proto))(*it_func))(args);	\
> > > >> -			} while (*(++it_func));				\
> > > >> +				it_func = (it_func_ptr)->func;		\
> > > >> +				__data = (it_func_ptr)->data;		\
> > > >> +				((void(*)(proto))(it_func))(args);	\
> > > >
> > > >
> > > >So, we had a talk about this and we concluded that it is probably fine
> > > >on every archs to push one more argument than needed in a function.
> > > >
> > > 
> > > Yeah, I'm hoping it's fine.
> > 
> > How about changing the callback prototypes to match the call arguments (changing
> > the type expected in register/unregister_trace, as well as an additional "check
> > type" that I proposed for Ftrace) ?
> 
> This can not happen!!!! As I said before, the register is done in C,
> there is no macro that will help here. We create the call back with the
> macro, but the registering is in kernel/trace/trace_events.c. One
> register for ___ALL___ events!!!
> 
>  Thus there is no check.
> 
> Understand this yet?

Clearly understood. I was referring to add a static inline
check_trace_##name##_callback_type(...) { } call within the callbacks you
generate. It generates no code and adds compiler type-checking (rather than
relying on CPP macro correctness for type-correctness).

> 
> 
> > 
> > Otherwise, you basically expect here that:
> > 
> > void fct(void *foo, void *bar, etc etc) (N parameters expected)
> > {
> > 
> > }
> > 
> > called by:
> > 
> > fct(foo, bar, etc etc, foobar) (N + 1 parameters)
> > 
> > will always work.
> > 
> > Can you show me where the C standard says it is safe to do so ?
> 
> No, but it seems safe in the kernel ;-)

The use of "seems" here does not give me a warm feeling of safety. ;)

> 
> But that said. There is another option that will conform to this, and
> that is to add flags to registering tracepoints. I already wrote a patch
> for this in trying to do some other work (that I threw away).
> 
> 
> So here's the proposal.
> 
> Change struct tracepoint_func to...
> 
> struct tracepoint_func {
> 	void *func;
> 	void *data;
> 	unsigned int flags;
> };
> 
> 
> The flags is set when registered. If a function is registered with data,
> then the flags field will be set. Then the calling of the function can
> be:
> 
> 	if ((it_func_ptr)->flags & TP_FL_DATA)
> 		((void(*)(proto, void *))(it_func)(args, __data);
> 	else
> 		((void(*)(proto))(it_func)(args);
> 
> This would comply with the C standard.

This would also add a branch on the tracing fast path, which I would like to
avoid. Why can't we simply change all prototypes to take an extra void *__data
parameter instead ?

Thanks,

Mathieu

> 
> -- Steve
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Steven Rostedt]

On Fri, 2010-05-07 at 11:08 -0400, Mathieu Desnoyers wrote:

> > > Can you show me where the C standard says it is safe to do so ?
> > 
> > No, but it seems safe in the kernel ;-)
> 
> The use of "seems" here does not give me a warm feeling of safety. ;)

Right, which is why I added the below.

> 
> > 
> > But that said. There is another option that will conform to this, and
> > that is to add flags to registering tracepoints. I already wrote a patch
> > for this in trying to do some other work (that I threw away).
> > 
> > 
> > So here's the proposal.
> > 
> > Change struct tracepoint_func to...
> > 
> > struct tracepoint_func {
> > 	void *func;
> > 	void *data;
> > 	unsigned int flags;
> > };
> > 
> > 
> > The flags is set when registered. If a function is registered with data,
> > then the flags field will be set. Then the calling of the function can
> > be:
> > 
> > 	if ((it_func_ptr)->flags & TP_FL_DATA)
> > 		((void(*)(proto, void *))(it_func)(args, __data);
> > 	else
> > 		((void(*)(proto))(it_func)(args);
> > 
> > This would comply with the C standard.
> 
> This would also add a branch on the tracing fast path, which I would like to
> avoid. Why can't we simply change all prototypes to take an extra void *__data
> parameter instead ?

I'm fine with making the data parameter mandatory with all tracers. Thus
the call back must require it. I would then move the data parameter from
the end to the beginning.

So a tracepoint with proto, will have a callback:

	void callback(void *data, proto);

I'm fine with forcing all callbacks to include a data parameter if you
are. This would also make the changes simpler.

-- Steve

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2010-05-07 at 11:08 -0400, Mathieu Desnoyers wrote:
> 
> > > > Can you show me where the C standard says it is safe to do so ?
> > > 
> > > No, but it seems safe in the kernel ;-)
> > 
> > The use of "seems" here does not give me a warm feeling of safety. ;)
> 
> Right, which is why I added the below.
> 
> > 
> > > 
> > > But that said. There is another option that will conform to this, and
> > > that is to add flags to registering tracepoints. I already wrote a patch
> > > for this in trying to do some other work (that I threw away).
> > > 
> > > 
> > > So here's the proposal.
> > > 
> > > Change struct tracepoint_func to...
> > > 
> > > struct tracepoint_func {
> > > 	void *func;
> > > 	void *data;
> > > 	unsigned int flags;
> > > };
> > > 
> > > 
> > > The flags is set when registered. If a function is registered with data,
> > > then the flags field will be set. Then the calling of the function can
> > > be:
> > > 
> > > 	if ((it_func_ptr)->flags & TP_FL_DATA)
> > > 		((void(*)(proto, void *))(it_func)(args, __data);
> > > 	else
> > > 		((void(*)(proto))(it_func)(args);
> > > 
> > > This would comply with the C standard.
> > 
> > This would also add a branch on the tracing fast path, which I would like to
> > avoid. Why can't we simply change all prototypes to take an extra void *__data
> > parameter instead ?
> 
> I'm fine with making the data parameter mandatory with all tracers. Thus
> the call back must require it. I would then move the data parameter from
> the end to the beginning.
> 
> So a tracepoint with proto, will have a callback:
> 
> 	void callback(void *data, proto);
> 
> I'm fine with forcing all callbacks to include a data parameter if you
> are. This would also make the changes simpler.

Yes, I am all for it.

As for the extra type checking, it is basically just trying to force you to
generate matching caller-callee prototypes in your CPP macros. The goal is
really to check that the data parameter type match in both the caller and
callee. I see that as a mean to make sure nobody is going to try to take
shortcuts by playing with the callback types in the "undefined behavior" zone of
the C standard in future TRACE_EVENT() modifications.

Thanks,

Mathieu

> 
> -- Steve
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

[Steven Rostedt]

On Fri, 2010-05-07 at 11:30 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:

> > I'm fine with forcing all callbacks to include a data parameter if you
> > are. This would also make the changes simpler.
> 
> Yes, I am all for it.

Great! I'm making the changes now.

> 
> As for the extra type checking, it is basically just trying to force you to
> generate matching caller-callee prototypes in your CPP macros. The goal is
> really to check that the data parameter type match in both the caller and
> callee. I see that as a mean to make sure nobody is going to try to take
> shortcuts by playing with the callback types in the "undefined behavior" zone of
> the C standard in future TRACE_EVENT() modifications.

OK, I'll wait for your patch. As my favorite saying goes...

"When people ask me what language my mother tongue is, I simply reply 'C'."

-- Steve

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Frederic Weisbecker]

On Fri, May 07, 2010 at 10:09:31AM -0400, Steven Rostedt wrote:
> > > +#define DECLARE_TRACE(name, proto, args)				\
> > > +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> > > +				PARAMS(proto, void *__data),		\
> > > +				PARAMS(args, __data))
> > >  
> > >  #define DEFINE_TRACE_FN(name, reg, unreg)				\
> > >  	static const char __tpstrtab_##name[]				\
> > > @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> > >  	struct tracepoint *end);
> > >  
> > >  #else /* !CONFIG_TRACEPOINTS */
> > > -#define DECLARE_TRACE(name, proto, args)				\
> > > -	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> > > -	{ }								\
> > > +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
> > >  	static inline void trace_##name(proto)				\
> > > -	{ }								\
> > > +	{								\
> > > +	}								\
> > >  	static inline int register_trace_##name(void (*probe)(proto))	\
> > >  	{								\
> > >  		return -ENOSYS;						\
> > >  	}								\
> > > -	static inline int unregister_trace_##name(void (*probe)(proto))	\
> > > +	static inline int unregister_trace_##name(void (*probe)(proto)) \
> > > +	{								\
> > > +		return -ENOSYS;						\
> > > +	}								\
> > > +	static inline int						\
> > > +	register_trace_##name##_data(void (*probe)(data_proto),		\
> > > +				     void *data)			\
> > > +	{								\
> > > +		return -ENOSYS;						\
> > > +	}								\
> > > +	static inline int						\
> > > +	unregister_trace_##name##_data(void (*probe)(data_proto),	\
> > > +				       void *data)			\
> > >  	{								\
> > >  		return -ENOSYS;						\
> > >  	}
> > > +#define DECLARE_TRACE_NOARGS(name)					\
> > > +		__DECLARE_TRACE(name, void, , void *__data, __data)
> > > +
> > > +#define DECLARE_TRACE(name, proto, args)				\
> > > +		__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),	\
> > > +				PARAMS(proto, void *__data),		\
> > > +				PARAMS(args, __data))
> > 
> > 
> > 
> > 
> > It seems that the on and off cases are exactly the same for DECLARE_TRACE*(),
> > you could provide a single version and let the __DECLARE_TRACE() do
> > the on/off trick.
> 
> 
> I don't know what you mean here. How would __DECLARE_TRACE() do what
> both DECLARE_TRACE() and DECLARE_TRACE_NOARGS() do? It will fail the
> compile if proto is "void".



No, what I meant is that you have:

#ifdef CONFIG_TRACEPOINTS
[...]
+#define DECLARE_TRACE_NOARGS(name)                                       \
      __DECLARE_TRACE(name, void, , void *__data, __data)

#define DECLARE_TRACE(name, proto, args)                         \
      __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),      \
                      PARAMS(proto, void *__data),            \
                      PARAMS(args, __data))
[...]
#else
[...]
+#define DECLARE_TRACE_NOARGS(name)                                       \
      __DECLARE_TRACE(name, void, , void *__data, __data)

#define DECLARE_TRACE(name, proto, args)                         \
      __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),      \
                      PARAMS(proto, void *__data),            \
                      PARAMS(args, __data)
[...]
#endif


See? They seem to be the exact same version, so this could be only
one version outside the ifdef.
And the CONFIG_TRACEPOINTS on/off case is dealt from __DECLARE_TRACE().

Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks

[Steven Rostedt]

On Fri, 2010-05-07 at 20:06 +0200, Frederic Weisbecker wrote:

> No, what I meant is that you have:
> 
> #ifdef CONFIG_TRACEPOINTS
> [...]
> +#define DECLARE_TRACE_NOARGS(name)                                       \
>       __DECLARE_TRACE(name, void, , void *__data, __data)
> 
> #define DECLARE_TRACE(name, proto, args)                         \
>       __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),      \
>                       PARAMS(proto, void *__data),            \
>                       PARAMS(args, __data))
> [...]
> #else
> [...]
> +#define DECLARE_TRACE_NOARGS(name)                                       \
>       __DECLARE_TRACE(name, void, , void *__data, __data)
> 
> #define DECLARE_TRACE(name, proto, args)                         \
>       __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),      \
>                       PARAMS(proto, void *__data),            \
>                       PARAMS(args, __data)
> [...]
> #endif
> 
> 
> See? They seem to be the exact same version, so this could be only
> one version outside the ifdef.
> And the CONFIG_TRACEPOINTS on/off case is dealt from __DECLARE_TRACE().

Ah, I see (said the blind man as he slipped and fell on the ice).

-- Steve