TRACE_EVENT is more unify and generic for define a tracepoint.
It also add ftrace support for this tracepoint.
Signed-off-by: Zhao Lei <[email protected]>
---
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
* Zhaolei <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Apr 09, 2009 at 08:40:41AM +0200, Ingo Molnar wrote:
>
> * Zhaolei <[email protected]> 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 <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
Commit-ID: 5cb3d1d9d34ac04bcaa2034139345b2a5fea54c1
Gitweb: http://git.kernel.org/tip/5cb3d1d9d34ac04bcaa2034139345b2a5fea54c1
Author: Zhaolei <[email protected]>
AuthorDate: Thu, 9 Apr 2009 14:08:18 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Neil Horman <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: "Steven Rostedt ;" <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Tom Zanussi <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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>