[patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Arjan van de Ven]

On Mon, 21 Sep 2009 17:20:31 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2009-09-21 at 20:00 +0200, Arjan van de Ven wrote:
> > On Mon, 21 Sep 2009 12:46:07 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Mon, 2009-09-21 at 15:28 +0200, Arjan van de Ven wrote:
> 
> > > This patch compiled for me;
> > > 
> > 
> > your magic skills exceed mine ;-)
> > 
> > mind sending me a signoff for this ?
> 
> Sure,
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


>From e2c647ae2d3ddc25b804f0419956caf40d89c606 Mon Sep 17 00:00:00 2001
From: Steven Rostedt <rostedt@goodmis.org>
Date: Mon, 21 Sep 2009 20:14:53 +0200
Subject: [PATCH] trace: introduce TRACE_EVENT_ABI

Some trace events are suitable to form a stable userspace ABI;
this patch introduces infrastructure to mark them as such,
and marks the first few tracepoints this way

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/tracepoint.h   |    4 ++++
 include/trace/events/power.h |    6 +++---
 include/trace/events/sched.h |    4 ++--
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 63a3f7a..4cb454f 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -280,6 +280,10 @@ static inline void tracepoint_synchronize_unregister(void)
  * TRACE_EVENT_FN to perform any (un)registration work.
  */
 
+#define TRACE_EVENT_ABI(name, proto, args, tstruct, assign, print)	\
+	TRACE_EVENT(name, PARAMS(proto), PARAMS(args),			\
+		    PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
 #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
 #define TRACE_EVENT_FN(name, proto, args, struct,		\
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index ea6d579..7f0e86e 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -18,7 +18,7 @@ enum {
 
 
 
-TRACE_EVENT(power_start,
+TRACE_EVENT_ABI(power_start,
 
 	TP_PROTO(unsigned int type, unsigned int state),
 
@@ -37,7 +37,7 @@ TRACE_EVENT(power_start,
 	TP_printk("type=%lu state=%lu", (unsigned long)__entry->type, (unsigned long)__entry->state)
 );
 
-TRACE_EVENT(power_end,
+TRACE_EVENT_ABI(power_end,
 
 	TP_PROTO(int dummy),
 
@@ -56,7 +56,7 @@ TRACE_EVENT(power_end,
 );
 
 
-TRACE_EVENT(power_frequency,
+TRACE_EVENT_ABI(power_frequency,
 
 	TP_PROTO(unsigned int type, unsigned int state),
 
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 4069c43..93a3e17 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -83,7 +83,7 @@ TRACE_EVENT(sched_wait_task,
  * (NOTE: the 'rq' argument is not used by generic trace events,
  *        but used by the latency tracer plugin. )
  */
-TRACE_EVENT(sched_wakeup,
+TRACE_EVENT_ABI(sched_wakeup,
 
 	TP_PROTO(struct rq *rq, struct task_struct *p, int success),
 
@@ -149,7 +149,7 @@ TRACE_EVENT(sched_wakeup_new,
  * (NOTE: the 'rq' argument is not used by generic trace events,
  *        but used by the latency tracer plugin. )
  */
-TRACE_EVENT(sched_switch,
+TRACE_EVENT_ABI(sched_switch,
 
 	TP_PROTO(struct rq *rq, struct task_struct *prev,
 		 struct task_struct *next),
-- 
1.6.0.6



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Frédéric Weisbecker]

2009/9/23, Arjan van de Ven <arjan@infradead.org>:
> From e2c647ae2d3ddc25b804f0419956caf40d89c606 Mon Sep 17 00:00:00 2001
> From: Steven Rostedt <rostedt@goodmis.org>
> Date: Mon, 21 Sep 2009 20:14:53 +0200
> Subject: [PATCH] trace: introduce TRACE_EVENT_ABI
>
> Some trace events are suitable to form a stable userspace ABI;
> this patch introduces infrastructure to mark them as such,
> and marks the first few tracepoints this way
>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  include/linux/tracepoint.h   |    4 ++++
>  include/trace/events/power.h |    6 +++---
>  include/trace/events/sched.h |    4 ++--
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 63a3f7a..4cb454f 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -280,6 +280,10 @@ static inline void tracepoint_synchronize_unregister(void)
>  * TRACE_EVENT_FN to perform any (un)registration work.
>  */
>
> +#define TRACE_EVENT_ABI(name, proto, args, tstruct, assign, print)     \
> +       TRACE_EVENT(name, PARAMS(proto), PARAMS(args),                  \
> +                   PARAMS(tstruct), PARAMS(assign), PARAMS(print))
> +


You may probably need to undefine it from trace/define_trace.h
once every cpp magic have been done (ie: once trace/ftrace.h
have been included.)

Because some files include several tracepoint/trace event headers,
then linux/tracepoint.h gets included several times and eventually
TRACE_EVENT_ABI() would be redefined (and then crash).

That's why we have:

#ifdef CONFIG_EVENT_TRACING
#include <trace/ftrace.h>
#endif

#undef TRACE_EVENT
#undef TRACE_EVENT_FN
#undef TRACE_HEADER_MULTI_READ

in define_trace.h: to allow such re-inclusion of tracepoint.h
and avoid redefinitions of TRACE_EVENT and TRACE_EVENT_FN.

So you just need to add:

 #undef TRACE_EVENT
 #undef TRACE_EVENT_FN
+#undef TRACE_EVENT_ABI
 #undef TRACE_HEADER_MULTI_READ


The problem is visible if you build napi that has such
multiple tracepoint headers included in a single C file.

Thanks.

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Christoph Hellwig]

I'm not sure we can support any as an ABI yet.  The text format seems to
volatile in general - not just the output of the individual trace events
but also the common file format, and for the binary format we need to
figure a good way to tag the output yet.   Also when we define one as
one ABI we should make very clear what that means, e.g. does it have to stay
exactly as is?  Or can we add new fields but not remove old one?

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Steven Rostedt]

On Wed, 2009-09-23 at 08:43 -0400, Christoph Hellwig wrote:
> I'm not sure we can support any as an ABI yet.  The text format seems to
> volatile in general - not just the output of the individual trace events
> but also the common file format, and for the binary format we need to
> figure a good way to tag the output yet.   Also when we define one as
> one ABI we should make very clear what that means, e.g. does it have to stay
> exactly as is?  Or can we add new fields but not remove old one?

I had this discussion with people in Portland. We seem to agree that
this should just lock the old fields in, but you can add new ones at the
end.

-- Steve

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Christoph Hellwig]

On Wed, Sep 23, 2009 at 10:03:52AM -0400, Steven Rostedt wrote:
> I had this discussion with people in Portland. We seem to agree that
> this should just lock the old fields in, but you can add new ones at the
> end.

Please document this very clearly.

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 2009-09-23 at 08:43 -0400, Christoph Hellwig wrote:

> > I'm not sure we can support any as an ABI yet.  The text format 
> > seems to volatile in general - not just the output of the individual 
> > trace events but also the common file format, and for the binary 
> > format we need to figure a good way to tag the output yet.  Also 
> > when we define one as one ABI we should make very clear what that 
> > means, e.g. does it have to stay exactly as is?  Or can we add new 
> > fields but not remove old one?
> 
> I had this discussion with people in Portland. We seem to agree that 
> this should just lock the old fields in, but you can add new ones at 
> the end.

Yeah, that's the sanest approach i was thinking about when i suggested 
TRACE_EVENT_ABI() to Arjan and Peter.

The raw record is opaque, comes with a length field and goes into the 
ring-buffer so it's nicely extensible. Existing bits shouldnt change.

User-space that relies on a record can define a structure of that and 
copy that over from the ring-buffer - and ignore any new bits.

What i'd also like to see is the use of typical ABI-safe type fields in 
the trace definitions themselves: u8, u32, u64, etc. 'long' is obviously 
not good. Could we do some automation for that perhaps? I.e. emit a 
warning (boot time or so) if TRACE_EVENT_ABI() is used with unsafe type 
fields.

( Endianness is another detail, if perf.data is shipped to a
  different-endian system. Best is probably to define a new perf.data
  attribute extension with the endianness of the generator system
  included. That way the perf.data parser can convert endianness if it 
  wants/needs to. )

	Ingo

Re: [patch] introduce TRACE_EVENT_ABI (was Re: TRACE_EVENT_ABI ?)

[Mathieu Desnoyers]

* Ingo Molnar (mingo@elte.hu) wrote:
> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 2009-09-23 at 08:43 -0400, Christoph Hellwig wrote:
> 
> > > I'm not sure we can support any as an ABI yet.  The text format 
> > > seems to volatile in general - not just the output of the individual 
> > > trace events but also the common file format, and for the binary 
> > > format we need to figure a good way to tag the output yet.  Also 
> > > when we define one as one ABI we should make very clear what that 
> > > means, e.g. does it have to stay exactly as is?  Or can we add new 
> > > fields but not remove old one?
> > 
> > I had this discussion with people in Portland. We seem to agree that 
> > this should just lock the old fields in, but you can add new ones at 
> > the end.
> 
> Yeah, that's the sanest approach i was thinking about when i suggested 
> TRACE_EVENT_ABI() to Arjan and Peter.
> 
> The raw record is opaque, comes with a length field and goes into the 
> ring-buffer so it's nicely extensible. Existing bits shouldnt change.
> 
> User-space that relies on a record can define a structure of that and 
> copy that over from the ring-buffer - and ignore any new bits.
> 
> What i'd also like to see is the use of typical ABI-safe type fields in 
> the trace definitions themselves: u8, u32, u64, etc. 'long' is obviously 
> not good. Could we do some automation for that perhaps? I.e. emit a 
> warning (boot time or so) if TRACE_EVENT_ABI() is used with unsafe type 
> fields.
> 
> ( Endianness is another detail, if perf.data is shipped to a
>   different-endian system. Best is probably to define a new perf.data
>   attribute extension with the endianness of the generator system
>   included. That way the perf.data parser can convert endianness if it 
>   wants/needs to. )
> 

You can ship sizeof(long) along with the endianness information to
access "long" typed variables from the parser too. If we export this
information, then I don't see how exporting a 'long' variable would be
obviously not good. It would become as portable as u32 or u64, yet more
efficient when only the space for a 'long' record is required. Same
applies to pointers, obviously.

All we need is a small "accessor" library to deal with the type size and
endianness issues.

Mathieu

> 	Ingo

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68