[PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[Zhaolei]

TRACE_EVENT is more unify and generic for define a tracepoint.
It also add ftrace support for this tracepoint.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 include/trace/skb.h               |    4 +--
 include/trace/skb_event_types.h   |   38 +++++++++++++++++++++++++++++++++++++
 include/trace/trace_event_types.h |    1 +
 include/trace/trace_events.h      |    1 +
 4 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 include/trace/skb_event_types.h

diff --git a/include/trace/skb.h b/include/trace/skb.h
index b66206d..d2de717 100644
--- a/include/trace/skb.h
+++ b/include/trace/skb.h
@@ -4,8 +4,6 @@
 #include <linux/skbuff.h>
 #include <linux/tracepoint.h>
 
-DECLARE_TRACE(kfree_skb,
-	TP_PROTO(struct sk_buff *skb, void *location),
-	TP_ARGS(skb, location));
+#include <trace/skb_event_types.h>
 
 #endif
diff --git a/include/trace/skb_event_types.h b/include/trace/skb_event_types.h
new file mode 100644
index 0000000..4a1c504
--- /dev/null
+++ b/include/trace/skb_event_types.h
@@ -0,0 +1,38 @@
+
+/* use <trace/skb.h> instead */
+#ifndef TRACE_EVENT
+# error Do not include this file directly.
+# error Unless you know what you are doing.
+#endif
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM skb
+
+/*
+ * Tracepoint for free an sk_buff:
+ */
+TRACE_EVENT(kfree_skb,
+
+	TP_PROTO(struct sk_buff *skb, void *location),
+
+	TP_ARGS(skb, location),
+
+	TP_STRUCT__entry(
+		__field(	void *,		skbaddr		)
+		__field(	unsigned short,	protocol	)
+		__field(	void *,		location	)
+	),
+
+	TP_fast_assign(
+		__entry->skbaddr = skb;
+		if (skb) {
+			__entry->protocol = ntohs(skb->protocol);
+		}
+		__entry->location = location;
+	),
+
+	TP_printk("skbaddr=%p protocol=%u location=%p",
+		__entry->skbaddr, __entry->protocol, __entry->location)
+);
+
+#undef TRACE_SYSTEM
diff --git a/include/trace/trace_event_types.h b/include/trace/trace_event_types.h
index df56f56..33b6bfc 100644
--- a/include/trace/trace_event_types.h
+++ b/include/trace/trace_event_types.h
@@ -3,3 +3,4 @@
 #include <trace/sched_event_types.h>
 #include <trace/irq_event_types.h>
 #include <trace/lockdep_event_types.h>
+#include <trace/skb_event_types.h>
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index fd13750..0e2aa80 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -3,3 +3,4 @@
 #include <trace/sched.h>
 #include <trace/irq.h>
 #include <trace/lockdep.h>
+#include <trace/skb.h>
-- 
1.5.5.3

Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[Ingo Molnar]

* Zhaolei <zhaolei@cn.fujitsu.com> wrote:

> TRACE_EVENT is more unify and generic for define a tracepoint.
> It also add ftrace support for this tracepoint.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  include/trace/skb.h               |    4 +--
>  include/trace/skb_event_types.h   |   38 +++++++++++++++++++++++++++++++++++++
>  include/trace/trace_event_types.h |    1 +
>  include/trace/trace_events.h      |    1 +
>  4 files changed, 41 insertions(+), 3 deletions(-)
>  create mode 100644 include/trace/skb_event_types.h

I've Cc:-ed more networking folks, the acks of them are needed.

David, Neil: the point of this patch is to make the kfree_skb() 
tracepoint show up in the general event tracing framework that got 
introduced upstream in this merge window.

One advantage is that this event will/can show up under the function 
graph tracer and other generic tracers as well:

     0)               |          handle_IRQ_event() {
     0)               |            /* irq_handler_entry: irq=48 handler=eth0 */
     0)               |            e1000_intr() {
     0)   0.926 us    |              __napi_schedule();
     0)   3.888 us    |            }
     0)               |            /* irq_handler_exit: irq=48 return=handled */
     0)   0.655 us    |            runqueue_is_locked();
     0)               |            __wake_up() {
     0)   0.831 us    |              _spin_lock_irqsave();

This tracepoint [when enabled] would show up as:

   skbaddr=%p protocol=%u location=%p

Another advantage is that its output can also be high-bandwidth 
per-cpu zero-copy traced via a no-vsprintf direct tracing path and 
via splice(), using the /debug/tracing/per_cpu/cpu*/trace_pipe_raw 
mechanism. (this is what the TP_fast_assign() portion of the 
TRACE_EVENT() macro achieves. )

Yet another advantage is that such tracepoints also show up under 
/debug/tracing/events/. For example the existing IRQ-handler-exit 
tracepoint shows up as:

 aldebaran:/debug/tracing/events/irq/irq_handler_exit> ls -l
 total 0
 -rw-r--r-- 1 root root 0 2009-04-08 08:56 enable
 -rw-r--r-- 1 root root 0 2009-04-08 08:56 filter
 -r--r--r-- 1 root root 0 2009-04-08 08:56 format
 -r--r--r-- 1 root root 0 2009-04-08 08:56 id

It can be toggled on/off, its format is exposed in a user-space-tool 
parse-able way:

 aldebaran:/debug/tracing/events/irq/irq_handler_exit> cat format

name: irq_handler_exit
ID: 27
format:
	field:unsigned char common_type;	offset:0;	size:1;
	field:unsigned char common_flags;	offset:1;	size:1;
	field:unsigned char common_preempt_count;	offset:2;	size:1;
	field:int common_pid;	offset:4;	size:4;
	field:int common_tgid;	offset:8;	size:4;

	field:int irq;	offset:12;	size:4;
	field:int ret;	offset:16;	size:4;

print fmt: "irq=%d return=%s", __entry->irq, __entry->ret ? "handled" : "unhandled"

The 'filter' file can be used to dynamically add optional filter 
expressions, which are evaluated by the kernel at the tracepoint to 
filter data. For example:

   echo 'irq == 1' > filter

Restricts this IRQ tracepoint to keyboard interrupts only. Doing:

   echo '|| irq == 0' >> filter

   cat filter 
   irq == 1
   || irq == 0

extends the filter expression to also include timer interrupts. Etc. 

All the fields enumerated in the 'format' descriptor can be used in 
the filter language. (there's also per subsystem filter expression 
to simplify the handling of a group of tracepoints, and other 
details.)

There's no fastpath overhead difference to old-style tracepoints in 
the disabled case, except some additional off-site section size 
increase. External tools can still hook in as well.

	Ingo

Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[Neil Horman]

On Thu, Apr 09, 2009 at 08:40:41AM +0200, Ingo Molnar wrote:
> 
> * Zhaolei <zhaolei@cn.fujitsu.com> wrote:
> 
> > TRACE_EVENT is more unify and generic for define a tracepoint.
> > It also add ftrace support for this tracepoint.
> > 
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  include/trace/skb.h               |    4 +--
> >  include/trace/skb_event_types.h   |   38 +++++++++++++++++++++++++++++++++++++
> >  include/trace/trace_event_types.h |    1 +
> >  include/trace/trace_events.h      |    1 +
> >  4 files changed, 41 insertions(+), 3 deletions(-)
> >  create mode 100644 include/trace/skb_event_types.h
> 
> I've Cc:-ed more networking folks, the acks of them are needed.
> 
> David, Neil: the point of this patch is to make the kfree_skb() 
> tracepoint show up in the general event tracing framework that got 
> introduced upstream in this merge window.
> 
> One advantage is that this event will/can show up under the function 
> graph tracer and other generic tracers as well:
> 
>      0)               |          handle_IRQ_event() {
>      0)               |            /* irq_handler_entry: irq=48 handler=eth0 */
>      0)               |            e1000_intr() {
>      0)   0.926 us    |              __napi_schedule();
>      0)   3.888 us    |            }
>      0)               |            /* irq_handler_exit: irq=48 return=handled */
>      0)   0.655 us    |            runqueue_is_locked();
>      0)               |            __wake_up() {
>      0)   0.831 us    |              _spin_lock_irqsave();
> 
> This tracepoint [when enabled] would show up as:
> 
>    skbaddr=%p protocol=%u location=%p
> 
> Another advantage is that its output can also be high-bandwidth 
> per-cpu zero-copy traced via a no-vsprintf direct tracing path and 
> via splice(), using the /debug/tracing/per_cpu/cpu*/trace_pipe_raw 
> mechanism. (this is what the TP_fast_assign() portion of the 
> TRACE_EVENT() macro achieves. )
> 
> Yet another advantage is that such tracepoints also show up under 
> /debug/tracing/events/. For example the existing IRQ-handler-exit 
> tracepoint shows up as:
> 
>  aldebaran:/debug/tracing/events/irq/irq_handler_exit> ls -l
>  total 0
>  -rw-r--r-- 1 root root 0 2009-04-08 08:56 enable
>  -rw-r--r-- 1 root root 0 2009-04-08 08:56 filter
>  -r--r--r-- 1 root root 0 2009-04-08 08:56 format
>  -r--r--r-- 1 root root 0 2009-04-08 08:56 id
> 
> It can be toggled on/off, its format is exposed in a user-space-tool 
> parse-able way:
> 
>  aldebaran:/debug/tracing/events/irq/irq_handler_exit> cat format
> 
> name: irq_handler_exit
> ID: 27
> format:
> 	field:unsigned char common_type;	offset:0;	size:1;
> 	field:unsigned char common_flags;	offset:1;	size:1;
> 	field:unsigned char common_preempt_count;	offset:2;	size:1;
> 	field:int common_pid;	offset:4;	size:4;
> 	field:int common_tgid;	offset:8;	size:4;
> 
> 	field:int irq;	offset:12;	size:4;
> 	field:int ret;	offset:16;	size:4;
> 
> print fmt: "irq=%d return=%s", __entry->irq, __entry->ret ? "handled" : "unhandled"
> 
> The 'filter' file can be used to dynamically add optional filter 
> expressions, which are evaluated by the kernel at the tracepoint to 
> filter data. For example:
> 
>    echo 'irq == 1' > filter
> 
> Restricts this IRQ tracepoint to keyboard interrupts only. Doing:
> 
>    echo '|| irq == 0' >> filter
> 
>    cat filter 
>    irq == 1
>    || irq == 0
> 
> extends the filter expression to also include timer interrupts. Etc. 
> 
> All the fields enumerated in the 'format' descriptor can be used in 
> the filter language. (there's also per subsystem filter expression 
> to simplify the handling of a group of tracepoints, and other 
> details.)
> 
> There's no fastpath overhead difference to old-style tracepoints in 
> the disabled case, except some additional off-site section size 
> increase. External tools can still hook in as well.
> 
> 	Ingo
> 

I think this seems like a reasonable idea, as long as the sites that register a
hook for the tracepoint don't need to be updated (and it looks like they dont).
I do have one nit though: include/trace/skb.h has been emptied and now includes
include/trace/skb_event_types.h instead.  For the sake of neatness I'd say we
should either not make that change, and just dump the contents of
skb_event_types.h into trace/skb.h, or alternatively, remove trace/skb.h
entirely, and have files that need trace/skb.h include skb_event_types.h
instead.  Do that and it has my ACK.

Thanks
Neil

Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[Steven Rostedt]

On Thu, 9 Apr 2009, Neil Horman wrote:
> 
> I think this seems like a reasonable idea, as long as the sites that register a
> hook for the tracepoint don't need to be updated (and it looks like they dont).
> I do have one nit though: include/trace/skb.h has been emptied and now includes
> include/trace/skb_event_types.h instead.  For the sake of neatness I'd say we
> should either not make that change, and just dump the contents of
> skb_event_types.h into trace/skb.h, or alternatively, remove trace/skb.h
> entirely, and have files that need trace/skb.h include skb_event_types.h
> instead.  Do that and it has my ACK.

Actually that trace/skb.h change is needed. It is not emptied, it still 
has:

 #include <linux/skbuff.h>
 #include <linux/tracepoint.h>

before the include of include/trace/skb_event_types.h

For ftrace to do its magic with the TRACE_EVENT macro, the file that 
contains the TRACE_EVENT can not include tracepoint.h.  ftrace runs the 
skb_event_types.h through a series of redefines of the TRACE_EVENT to 
automate the creation of the directory and files in 
debugfs/tracing/events/*.

We've already changed most of the files in include/trace/ to this format.

I hope we can still have your Ack without making your requested update.

Thanks,

-- Steve

Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

[Neil Horman]

On Thu, Apr 09, 2009 at 11:05:08AM -0400, Steven Rostedt wrote:
> 
> On Thu, 9 Apr 2009, Neil Horman wrote:
> > 
> > I think this seems like a reasonable idea, as long as the sites that register a
> > hook for the tracepoint don't need to be updated (and it looks like they dont).
> > I do have one nit though: include/trace/skb.h has been emptied and now includes
> > include/trace/skb_event_types.h instead.  For the sake of neatness I'd say we
> > should either not make that change, and just dump the contents of
> > skb_event_types.h into trace/skb.h, or alternatively, remove trace/skb.h
> > entirely, and have files that need trace/skb.h include skb_event_types.h
> > instead.  Do that and it has my ACK.
> 
> Actually that trace/skb.h change is needed. It is not emptied, it still 
> has:
> 
>  #include <linux/skbuff.h>
>  #include <linux/tracepoint.h>
> 
> before the include of include/trace/skb_event_types.h
> 
> For ftrace to do its magic with the TRACE_EVENT macro, the file that 
> contains the TRACE_EVENT can not include tracepoint.h.  ftrace runs the 
> skb_event_types.h through a series of redefines of the TRACE_EVENT to 
> automate the creation of the directory and files in 
> debugfs/tracing/events/*.
> 
> We've already changed most of the files in include/trace/ to this format.
> 
> I hope we can still have your Ack without making your requested update.
> 
> Thanks,
> 
> -- Steve
> 
> 

That strikes me as overly complex and fragile.  One would think that ftrace
could include a first pass to scrub out that include prior to processing.
Having a utility reliant on the format of kernel header files, especially the
placement of includes is begging for somthing to break.

I'll ack it since the change I'm asking for is just cosmetic, and since any
reversions in this area are going to break ftrace in userspace rather than oops
the kernel, but I would really like to see ftrace hardened in such a way that
we don't need to be extra careful of include formatting when adding additional
tracepoints.

Acked-by: Neil Horman <nhorman@tuxdriver.com>

[tip:tracing/core] tracing, net, skb tracepoint: make skb tracepoint use the TRACE_EVENT() macro

[Zhaolei]

Commit-ID:  5cb3d1d9d34ac04bcaa2034139345b2a5fea54c1
Gitweb:     http://git.kernel.org/tip/5cb3d1d9d34ac04bcaa2034139345b2a5fea54c1
Author:     Zhaolei <zhaolei@cn.fujitsu.com>
AuthorDate: Thu, 9 Apr 2009 14:08:18 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 10 Apr 2009 12:57:55 +0200

tracing, net, skb tracepoint: make skb tracepoint use the TRACE_EVENT() macro

TRACE_EVENT is a more generic way to define a tracepoint.
Doing so adds these new capabilities to this tracepoint:

  - zero-copy and per-cpu splice() tracing
  - binary tracing without printf overhead
  - structured logging records exposed under /debug/tracing/events
  - trace events embedded in function tracer output and other plugins
  - user-defined, per tracepoint filter expressions

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: "Steven Rostedt ;" <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <49DD90D2.5020604@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 include/trace/skb.h               |    4 +--
 include/trace/skb_event_types.h   |   38 +++++++++++++++++++++++++++++++++++++
 include/trace/trace_event_types.h |    1 +
 include/trace/trace_events.h      |    1 +
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/include/trace/skb.h b/include/trace/skb.h
index b66206d..d2de717 100644
--- a/include/trace/skb.h
+++ b/include/trace/skb.h
@@ -4,8 +4,6 @@
 #include <linux/skbuff.h>
 #include <linux/tracepoint.h>
 
-DECLARE_TRACE(kfree_skb,
-	TP_PROTO(struct sk_buff *skb, void *location),
-	TP_ARGS(skb, location));
+#include <trace/skb_event_types.h>
 
 #endif
diff --git a/include/trace/skb_event_types.h b/include/trace/skb_event_types.h
new file mode 100644
index 0000000..4a1c504
--- /dev/null
+++ b/include/trace/skb_event_types.h
@@ -0,0 +1,38 @@
+
+/* use <trace/skb.h> instead */
+#ifndef TRACE_EVENT
+# error Do not include this file directly.
+# error Unless you know what you are doing.
+#endif
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM skb
+
+/*
+ * Tracepoint for free an sk_buff:
+ */
+TRACE_EVENT(kfree_skb,
+
+	TP_PROTO(struct sk_buff *skb, void *location),
+
+	TP_ARGS(skb, location),
+
+	TP_STRUCT__entry(
+		__field(	void *,		skbaddr		)
+		__field(	unsigned short,	protocol	)
+		__field(	void *,		location	)
+	),
+
+	TP_fast_assign(
+		__entry->skbaddr = skb;
+		if (skb) {
+			__entry->protocol = ntohs(skb->protocol);
+		}
+		__entry->location = location;
+	),
+
+	TP_printk("skbaddr=%p protocol=%u location=%p",
+		__entry->skbaddr, __entry->protocol, __entry->location)
+);
+
+#undef TRACE_SYSTEM
diff --git a/include/trace/trace_event_types.h b/include/trace/trace_event_types.h
index df56f56..33b6bfc 100644
--- a/include/trace/trace_event_types.h
+++ b/include/trace/trace_event_types.h
@@ -3,3 +3,4 @@
 #include <trace/sched_event_types.h>
 #include <trace/irq_event_types.h>
 #include <trace/lockdep_event_types.h>
+#include <trace/skb_event_types.h>
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index fd13750..0e2aa80 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -3,3 +3,4 @@
 #include <trace/sched.h>
 #include <trace/irq.h>
 #include <trace/lockdep.h>
+#include <trace/skb.h>