Mailing List Archive: Linux: Kernel

[tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS()

Index | Next | Previous | View Threaded

mingo at elte

Nov 26, 2009, 12:16 AM

Post #1 of 12 (233 views)
Permalink

[tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS()

Commit-ID: 091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
Gitweb: http://git.kernel.org/tip/091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
Author: Ingo Molnar <mingo [at] elte>
AuthorDate: Thu, 26 Nov 2009 09:04:55 +0100
Committer: Ingo Molnar <mingo [at] elte>
CommitDate: Thu, 26 Nov 2009 09:04:55 +0100

events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS()

It is not quite obvious at first sight what TRACE_EVENT_TEMPLATE
does: does it define an event as well beyond defining a template?

To clarify this, rename it to DECLARE_EVENT_CLASS, which follows
the various 'DECLARE_*()' idioms we already have in the kernel:

DECLARE_EVENT_CLASS(class)

DEFINE_EVENT(class, event1)
DEFINE_EVENT(class, event2)
DEFINE_EVENT(class, event3)

To complete this logic we should also rename TRACE_EVENT() to:

DEFINE_SINGLE_EVENT(single_event)

... but in a more quiet moment of the kernel cycle.

Cc: Pekka Enberg <penberg [at] cs>
Cc: Steven Rostedt <rostedt [at] goodmis>
Cc: Frederic Weisbecker <fweisbec [at] gmail>
LKML-Reference: <4B0E286A.2000405 [at] cn>
Signed-off-by: Ingo Molnar <mingo [at] elte>
---
include/linux/tracepoint.h | 2 +-
include/trace/define_trace.h | 2 +-
include/trace/events/sched.h | 6 ++--
include/trace/ftrace.h | 46 +++++++++++++++++++++---------------------
4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 7063383..f59604e 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -280,7 +280,7 @@ static inline void tracepoint_synchronize_unregister(void)
* TRACE_EVENT_FN to perform any (un)registration work.
*/

-#define TRACE_EVENT_TEMPLATE(name, proto, args, tstruct, assign, print)
+#define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print)
#define DEFINE_EVENT(template, name, proto, args) \
DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
#define DEFINE_EVENT_PRINT(template, name, proto, args, print) \
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 5d7d855..5acfb1e 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -71,7 +71,7 @@

#undef TRACE_EVENT
#undef TRACE_EVENT_FN
-#undef TRACE_EVENT_TEMPLATE
+#undef DECLARE_EVENT_CLASS
#undef DEFINE_EVENT
#undef DEFINE_EVENT_PRINT
#undef TRACE_HEADER_MULTI_READ
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 238f74b..5ce7950 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_TEMPLATE(sched_wakeup_template,
+DECLARE_EVENT_CLASS(sched_wakeup_template,

TP_PROTO(struct rq *rq, struct task_struct *p, int success),

@@ -197,7 +197,7 @@ TRACE_EVENT(sched_migrate_task,
__entry->orig_cpu, __entry->dest_cpu)
);

-TRACE_EVENT_TEMPLATE(sched_process_template,
+DECLARE_EVENT_CLASS(sched_process_template,

TP_PROTO(struct task_struct *p),

@@ -316,7 +316,7 @@ TRACE_EVENT(sched_signal_send,
* XXX the below sched_stat tracepoints only apply to SCHED_OTHER/BATCH/IDLE
* adding sched_stat support to SCHED_FIFO/RR would be welcome.
*/
-TRACE_EVENT_TEMPLATE(sched_stat_template,
+DECLARE_EVENT_CLASS(sched_stat_template,

TP_PROTO(struct task_struct *tsk, u64 delay),

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index b046177..2c9c073 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -19,17 +19,17 @@
#include <linux/ftrace_event.h>

/*
- * TRACE_EVENT_TEMPLATE can be used to add a generic function
+ * DECLARE_EVENT_CLASS can be used to add a generic function
* handlers for events. That is, if all events have the same
* parameters and just have distinct trace points.
* Each tracepoint can be defined with DEFINE_EVENT and that
- * will map the TRACE_EVENT_TEMPLATE to the tracepoint.
+ * will map the DECLARE_EVENT_CLASS to the tracepoint.
*
* TRACE_EVENT is a one to one mapping between tracepoint and template.
*/
#undef TRACE_EVENT
#define TRACE_EVENT(name, proto, args, tstruct, assign, print) \
- TRACE_EVENT_TEMPLATE(name, \
+ DECLARE_EVENT_CLASS(name, \
PARAMS(proto), \
PARAMS(args), \
PARAMS(tstruct), \
@@ -56,8 +56,8 @@
#undef TP_STRUCT__entry
#define TP_STRUCT__entry(args...) args

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(name, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print) \
struct ftrace_raw_##name { \
struct trace_entry ent; \
tstruct \
@@ -115,8 +115,8 @@
#undef __string
#define __string(item, src) __dynamic_array(char, item, -1)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
struct ftrace_data_offsets_##call { \
tstruct; \
};
@@ -203,8 +203,8 @@
#undef TP_perf_assign
#define TP_perf_assign(args...)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, func, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, func, print) \
static int \
ftrace_format_setup_##call(struct ftrace_event_call *unused, \
struct trace_seq *s) \
@@ -321,8 +321,8 @@ ftrace_format_##name(struct ftrace_event_call *unused, \
ftrace_print_symbols_seq(p, value, symbols); \
})

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
static enum print_line_t \
ftrace_raw_output_id_##call(int event_id, const char *name, \
struct trace_iterator *iter, int flags) \
@@ -428,8 +428,8 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags) \
#undef __string
#define __string(item, src) __dynamic_array(char, item, -1)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, func, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, func, print) \
static int \
ftrace_define_fields_##call(struct ftrace_event_call *event_call) \
{ \
@@ -480,8 +480,8 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call) \
#undef __string
#define __string(item, src) __dynamic_array(char, item, strlen(src) + 1)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
static inline int ftrace_get_offsets_##call( \
struct ftrace_data_offsets_##call *__data_offsets, proto) \
{ \
@@ -521,8 +521,8 @@ static inline int ftrace_get_offsets_##call( \
*
*/

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print)
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)

#undef DEFINE_EVENT
#define DEFINE_EVENT(template, name, proto, args) \
@@ -681,8 +681,8 @@ static void ftrace_profile_disable_##name(struct ftrace_event_call *unused)\
#define __assign_str(dst, src) \
strcpy(__get_str(dst), src);

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
\
static void ftrace_raw_event_id_##call(struct ftrace_event_call *event_call, \
proto) \
@@ -764,8 +764,8 @@ static int ftrace_raw_init_event_##call(struct ftrace_event_call *unused)\

#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print)
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)

#undef DEFINE_EVENT
#define DEFINE_EVENT(template, call, proto, args) \
@@ -885,8 +885,8 @@ __attribute__((section("_ftrace_events"))) event_##call = { \
#undef __perf_count
#define __perf_count(c) __count = (c)

-#undef TRACE_EVENT_TEMPLATE
-#define TRACE_EVENT_TEMPLATE(call, proto, args, tstruct, assign, print) \
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
static void \
ftrace_profile_templ_##call(struct ftrace_event_call *event_call, \
proto) \
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 12:33 AM

Post #2 of 12 (219 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, 2009-11-26 at 08:16 +0000, tip-bot for Ingo Molnar wrote:
> Commit-ID: 091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
> Gitweb: http://git.kernel.org/tip/091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
> Author: Ingo Molnar <mingo [at] elte>
> AuthorDate: Thu, 26 Nov 2009 09:04:55 +0100
> Committer: Ingo Molnar <mingo [at] elte>
> CommitDate: Thu, 26 Nov 2009 09:04:55 +0100
>
> events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS()
>
> It is not quite obvious at first sight what TRACE_EVENT_TEMPLATE
> does: does it define an event as well beyond defining a template?
>
> To clarify this, rename it to DECLARE_EVENT_CLASS, which follows
> the various 'DECLARE_*()' idioms we already have in the kernel:
>
> DECLARE_EVENT_CLASS(class)
>
> DEFINE_EVENT(class, event1)
> DEFINE_EVENT(class, event2)
> DEFINE_EVENT(class, event3)
>
> To complete this logic we should also rename TRACE_EVENT() to:
>
> DEFINE_SINGLE_EVENT(single_event)
>
> ... but in a more quiet moment of the kernel cycle.

I would like to hear what others think about this change before we go
ahead and implement it. A lot of developers have just learned about
TRACE_EVENT and now it just disappeared. Well, not really, but in the
sense of ' find linux.git -name '*.[ch]' | xargs grep TRACE_EVENT' it no
longer exists.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 26, 2009, 12:40 AM

Post #3 of 12 (213 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

* Steven Rostedt <rostedt [at] goodmis> wrote:

> On Thu, 2009-11-26 at 08:16 +0000, tip-bot for Ingo Molnar wrote:
> > Commit-ID: 091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
> > Gitweb: http://git.kernel.org/tip/091ad3658e3c76c5fb05f65bfb64a0246f8f31b5
> > Author: Ingo Molnar <mingo [at] elte>
> > AuthorDate: Thu, 26 Nov 2009 09:04:55 +0100
> > Committer: Ingo Molnar <mingo [at] elte>
> > CommitDate: Thu, 26 Nov 2009 09:04:55 +0100
> >
> > events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS()
> >
> > It is not quite obvious at first sight what TRACE_EVENT_TEMPLATE
> > does: does it define an event as well beyond defining a template?
> >
> > To clarify this, rename it to DECLARE_EVENT_CLASS, which follows
> > the various 'DECLARE_*()' idioms we already have in the kernel:
> >
> > DECLARE_EVENT_CLASS(class)
> >
> > DEFINE_EVENT(class, event1)
> > DEFINE_EVENT(class, event2)
> > DEFINE_EVENT(class, event3)
> >
> > To complete this logic we should also rename TRACE_EVENT() to:
> >
> > DEFINE_SINGLE_EVENT(single_event)
> >
> > ... but in a more quiet moment of the kernel cycle.
>
>
> I would like to hear what others think about this change before we go
> ahead and implement it.

You mean TRACE_EVENT() -> DEFINE_SINGLE_EVENT()? Sure, we want todo it
in a more quiet moment of the kernel cycle, not now.

(TRACE_EVENT_TEMPLATE OTOH has existed for just a few days so it's not a
problem.)

> A lot of developers have just learned about TRACE_EVENT and now it
> just disappeared. Well, not really, but in the sense of ' find
> linux.git -name '*.[ch]' | xargs grep TRACE_EVENT' it no longer
> exists.

A second problem with the TRACE_EVENT name is that it's not just for
tracing - we dont necessarily 'trace' events here. We can use the event
callbacks to collect pure counts:

| aldebaran> perf stat -e sched:sched_wakeup ./hackbench 10
| Time: 0.093
|
| Performance counter stats for './hackbench 10':
|
| 15481 sched:sched_wakeup
|
| 0.107390574 seconds time elapsed

etc.

A third problem is that the name 'TRACE_EVENT' does not tell us what is
being done. Do we declare it? Do we also define it?

DEFINE_SINGLE_EVENT() solves all these problems:

- It's obvious what it does

- It suggests users of it that there's another non-single-event
facility, gently nudging them towards the use of the more efficient
DEFINE_EVENT_CLASS() + DEFINE_EVENT() method.

- It fits nicely into the rest of the naming scheme.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 6:45 AM

Post #4 of 12 (218 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, 2009-11-26 at 09:40 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt [at] goodmis> wrote:
>

> > I would like to hear what others think about this change before we go
> > ahead and implement it.
>
> You mean TRACE_EVENT() -> DEFINE_SINGLE_EVENT()? Sure, we want todo it
> in a more quiet moment of the kernel cycle, not now.
>
> (TRACE_EVENT_TEMPLATE OTOH has existed for just a few days so it's not a
> problem.)

Yes the template name is new, I'm not talking about that on
particularly.

>
> > A lot of developers have just learned about TRACE_EVENT and now it
> > just disappeared. Well, not really, but in the sense of ' find
> > linux.git -name '*.[ch]' | xargs grep TRACE_EVENT' it no longer
> > exists.
>
> A second problem with the TRACE_EVENT name is that it's not just for
> tracing - we dont necessarily 'trace' events here. We can use the event
> callbacks to collect pure counts:

Then we might as well rename the "trace_*" all over the kernel.

>
> | aldebaran> perf stat -e sched:sched_wakeup ./hackbench 10
> | Time: 0.093
> |
> | Performance counter stats for './hackbench 10':
> |
> | 15481 sched:sched_wakeup
> |
> | 0.107390574 seconds time elapsed
>
> etc.

Right, because it hooked into a trace_point.

>
> A third problem is that the name 'TRACE_EVENT' does not tell us what is
> being done. Do we declare it? Do we also define it?

That's exactly the point. It does both. I actually tried to avoid the
"DEFINE/DECLARE" because it becomes confusing to what it does. The
TRACE_EVENT macros are obviously unique in the kernel. There are
"DECLARE_*" and "DEFINE_*" all over the kernel. And they have an obvious
meaning. DECLARE_* is used to set up a declaration for a header.
DEFINE_* creates the instance. But TRACE_EVENT will default declare
event, but when CREATE_TRACE_POINTS is set, it defines the instances. Oh
we should rename that to CREATE_EVENTS?

>
> DEFINE_SINGLE_EVENT() solves all these problems:
>
> - It's obvious what it does
>
> - It suggests users of it that there's another non-single-event
> facility, gently nudging them towards the use of the more efficient
> DEFINE_EVENT_CLASS() + DEFINE_EVENT() method.
>
> - It fits nicely into the rest of the naming scheme.

Like I said earlier, I'm not really attached to the name. Except that
there's already a lot of documentation (I've given tutorials about it)
using the TRACE_EVENT name. But who am I to decide?

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

fweisbec at gmail

Nov 26, 2009, 9:55 AM

Post #5 of 12 (217 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, Nov 26, 2009 at 09:45:30AM -0500, Steven Rostedt wrote:
> On Thu, 2009-11-26 at 09:40 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt [at] goodmis> wrote:
> >
>
> > > I would like to hear what others think about this change before we go
> > > ahead and implement it.
> >
> > You mean TRACE_EVENT() -> DEFINE_SINGLE_EVENT()? Sure, we want todo it
> > in a more quiet moment of the kernel cycle, not now.
> >
> > (TRACE_EVENT_TEMPLATE OTOH has existed for just a few days so it's not a
> > problem.)
>
> Yes the template name is new, I'm not talking about that on
> particularly.

I personally don't mind much about the name, especially
between class and template. Both make equally sense to me.

But DECLARE sounds like a misnomer here (like DEFINE_EVENT
somehow) as TRACE_EVENT, DEFINE_EVENT and TRACE_EVENT_TEMPLATE
all behave either as a declaration or a definition, depending
on the CREATE_TRACE_POINT macro.

Also, considering the arising question of notifiers and TRACE_EVENT
that are starting to collide in that we have two event callbacks
subsystems that could be gathered in one, I guess TRACE_EVENT will
become too general in the future.

If we consider improving the TRACE_EVENT to support tracing (like
it does already) but also blocking notifiers, atomic notifiers, etc...
by migrating the notifier code to TRACE_EVENT,
then the name should probably be reconsidered as a more general thing.

KERNEL_EVENT ? NOTIFY_EVENT ?

And then the CPP callbacks we are currently using for tracing should
probably be renamed as they won't concern the notifier callbacks.

TP_printk could be renamed as TRACE_print, TP_fast_assing could be
TRACE_fast_assign, etc...

> >
> > > A lot of developers have just learned about TRACE_EVENT and now it
> > > just disappeared. Well, not really, but in the sense of ' find
> > > linux.git -name '*.[ch]' | xargs grep TRACE_EVENT' it no longer
> > > exists.
> >
> > A second problem with the TRACE_EVENT name is that it's not just for
> > tracing - we dont necessarily 'trace' events here. We can use the event
> > callbacks to collect pure counts:
>
> Then we might as well rename the "trace_*" all over the kernel.

I think this should be kept. Although if notifier goes migrated in
TRACE_EVENT, having notify_event() would gather the two meanings of
trace_* and notify_*

> > DEFINE_SINGLE_EVENT() solves all these problems:
> >
> > - It's obvious what it does
> >
> > - It suggests users of it that there's another non-single-event
> > facility, gently nudging them towards the use of the more efficient
> > DEFINE_EVENT_CLASS() + DEFINE_EVENT() method.
> >
> > - It fits nicely into the rest of the naming scheme.
>
> Like I said earlier, I'm not really attached to the name. Except that
> there's already a lot of documentation (I've given tutorials about it)
> using the TRACE_EVENT name. But who am I to decide?

Not that I like much DEFINE_SINGLE_EVENT(), because DEFINE is ambiguous
and SINGLE too (single can indeed be interpreted as something that doesn't
need a class, but is also confusing as it suggests that DEFINE_EVENT defines
several events in once), but I think a tutorial shouldn't paralyze a
subsystem progression.

Why not having BUILD_EVENT_CLASS(), BUILD_EVENT_FROM_CLASS(), and BUILD_EVENT() ?

That said, TRACE_EVENT() can still remain as an alias.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 26, 2009, 10:12 AM

Post #6 of 12 (219 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

* Frederic Weisbecker <fweisbec [at] gmail> wrote:

> On Thu, Nov 26, 2009 at 09:45:30AM -0500, Steven Rostedt wrote:
> > On Thu, 2009-11-26 at 09:40 +0100, Ingo Molnar wrote:
> > > * Steven Rostedt <rostedt [at] goodmis> wrote:
> > >
> >
> > > > I would like to hear what others think about this change before we go
> > > > ahead and implement it.
> > >
> > > You mean TRACE_EVENT() -> DEFINE_SINGLE_EVENT()? Sure, we want todo it
> > > in a more quiet moment of the kernel cycle, not now.
> > >
> > > (TRACE_EVENT_TEMPLATE OTOH has existed for just a few days so it's not a
> > > problem.)
> >
> > Yes the template name is new, I'm not talking about that on
> > particularly.
>
> I personally don't mind much about the name, especially between class
> and template. Both make equally sense to me.
>
> But DECLARE sounds like a misnomer here (like DEFINE_EVENT somehow) as
> TRACE_EVENT, DEFINE_EVENT and TRACE_EVENT_TEMPLATE all behave either
> as a declaration or a definition, depending on the CREATE_TRACE_POINT
> macro.

DECLARE_EVENT_CLASS() doesnt really define an event visible to the user
yet though. It defines functions internally (to be used by the real
definition of the event) - but not visible externally really.

So the real 'definition' of an event happens with DEFINE_EVENT() - in
the logical model of this.

So the logical model is clear:

DECLARE_EVENT_CLASS(class);

DEFINE_EVENT(class, event1);
DEFINE_EVENT(class, event2);
DEFINE_EVENT(class, event3);
...

# later:
# DEFINE_STANDALONE_EVENT(event)

And the logical model is what matters: that's what developers will use.
They'll use these constructs based on the logical model, nobody sane
will look into the CPP magic ;-)

And yes, we occasionally have to revisit our naming choices - especially
when mistakes/misnomers become apparent.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 11:12 AM

Post #7 of 12 (218 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

(added Christoph since he was the one to recommend the template
creation)

On Thu, 2009-11-26 at 19:12 +0100, Ingo Molnar wrote:
> * Frederic Weisbecker <fweisbec [at] gmail> wrote:

> DECLARE_EVENT_CLASS() doesnt really define an event visible to the user
> yet though. It defines functions internally (to be used by the real
> definition of the event) - but not visible externally really.
>
> So the real 'definition' of an event happens with DEFINE_EVENT() - in
> the logical model of this.
>
> So the logical model is clear:
>
> DECLARE_EVENT_CLASS(class);
>
> DEFINE_EVENT(class, event1);
> DEFINE_EVENT(class, event2);
> DEFINE_EVENT(class, event3);
> ...
>
> # later:
> # DEFINE_STANDALONE_EVENT(event)

I think that name sounds even uglier than DEFINE_SINGLE_EVENT :-/

I'm fine with the DECLARE_EVENT_CLASS and DEFINE_EVENT, but I'm unsure
what to rename TRACE_EVENT as. I know its still pretty new, but it's
being used quite a bit. So it should take some extra thought.

I guess DEFINE_EVENT_CLASS is probably not good, although this would be
the combination of DECLARE_EVENT_CLASS and DEFINE_EVENT which it
actually is.

DECLARE_DEFINE_EVENT? *naw*

DEFINE_DECLARED_EVENT?

Or we could go with DECLARE_EVENT(), DECLARE_EVENT_CLASS() and
DEFINE_EVENT_CLASS_INSTANCE()?

>
> And the logical model is what matters: that's what developers will use.
> They'll use these constructs based on the logical model, nobody sane
> will look into the CPP magic ;-)
>
> And yes, we occasionally have to revisit our naming choices - especially
> when mistakes/misnomers become apparent.

Agreed, but lets discuss it before we commit it to a non-rebase branch.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 26, 2009, 11:20 AM

Post #8 of 12 (216 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

* Steven Rostedt <rostedt [at] goodmis> wrote:

> (added Christoph since he was the one to recommend the template
> creation)
>
> On Thu, 2009-11-26 at 19:12 +0100, Ingo Molnar wrote:
> > * Frederic Weisbecker <fweisbec [at] gmail> wrote:
>
> > DECLARE_EVENT_CLASS() doesnt really define an event visible to the user
> > yet though. It defines functions internally (to be used by the real
> > definition of the event) - but not visible externally really.
> >
> > So the real 'definition' of an event happens with DEFINE_EVENT() - in
> > the logical model of this.
> >
> > So the logical model is clear:
> >
> > DECLARE_EVENT_CLASS(class);
> >
> > DEFINE_EVENT(class, event1);
> > DEFINE_EVENT(class, event2);
> > DEFINE_EVENT(class, event3);
> > ...
> >
> > # later:
> > # DEFINE_STANDALONE_EVENT(event)
>
> I think that name sounds even uglier than DEFINE_SINGLE_EVENT :-/
>
> I'm fine with the DECLARE_EVENT_CLASS and DEFINE_EVENT, but I'm unsure
> what to rename TRACE_EVENT as. I know its still pretty new, but it's
> being used quite a bit. So it should take some extra thought.
>
> I guess DEFINE_EVENT_CLASS is probably not good, although this would
> be the combination of DECLARE_EVENT_CLASS and DEFINE_EVENT which it
> actually is.
>
> DECLARE_DEFINE_EVENT? *naw*
>
> DEFINE_DECLARED_EVENT?
>
> Or we could go with DECLARE_EVENT(), DECLARE_EVENT_CLASS() and
> DEFINE_EVENT_CLASS_INSTANCE()?

I think the most common one should be the shortest, and the most common
one will be DEFINE_EVENT() - that's short enough already IMO.

I think we generally want to encourage the creation of classes of
events, not myriads of standalone events, each with their own call
signature, record format and printouts.

In that sense making the TRACE_EVENT() one longer would achieve that
goal of discouraging its over-use: DEFINE_SINGLE_EVENT() tells the
developer that it's an event of it's kind.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 11:44 AM

Post #9 of 12 (218 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, 2009-11-26 at 20:20 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt [at] goodmis> wrote:

> > DECLARE_DEFINE_EVENT? *naw*
> >
> > DEFINE_DECLARED_EVENT?
> >
> > Or we could go with DECLARE_EVENT(), DECLARE_EVENT_CLASS() and
> > DEFINE_EVENT_CLASS_INSTANCE()?
>
> I think the most common one should be the shortest, and the most common
> one will be DEFINE_EVENT() - that's short enough already IMO.

The above were ideas for replacing TRACE_EVENT, not the current
DEFINE_EVENT.

>
> I think we generally want to encourage the creation of classes of
> events, not myriads of standalone events, each with their own call
> signature, record format and printouts.
>
> In that sense making the TRACE_EVENT() one longer would achieve that
> goal of discouraging its over-use: DEFINE_SINGLE_EVENT() tells the
> developer that it's an event of it's kind.

But I do agree with Frederic that this can be a little confusing, since
it makes it sound like DEFINE_EVENT is for multiple events.

What about saying exactly what it does?

DECLARE_AND_DEFINE_EVENT()

Come to think of it, since current TRACE_EVENT is now just:

#define TRACE_EVENT() \
TRACE_EVENT_TEMPLATE() \
DEFINE_EVENT

This may make the most sense. I haven't tried it, but I believe that you
could even base other events off of the TRACE_EVENT. That is:

TRACE_EVENT(x, ...);

DEFINE_EVENT(x, y, ...);

And y would use x as its class.

So going back to your scheme of DECLARE_EVENT_CLASS(), it may make sense
to have DECLARE_AND_DEFINE_EVENT().

DECLARE_EVENT_CLASS(class, ...);
DEFINE_EVENT(class, foo, ...);

DECLARE_AND_DEFINE_EVENT(bar, ...);

DEFINE_EVENT(bar, zoo, ...);

May work.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 11:47 AM

Post #10 of 12 (217 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, 2009-11-26 at 14:44 -0500, Steven Rostedt wrote:

> Come to think of it, since current TRACE_EVENT is now just:
>
> #define TRACE_EVENT() \
> TRACE_EVENT_TEMPLATE() \
> DEFINE_EVENT
>
> This may make the most sense. I haven't tried it, but I believe that you
> could even base other events off of the TRACE_EVENT. That is:
>
> TRACE_EVENT(x, ...);
>
> DEFINE_EVENT(x, y, ...);
>
> And y would use x as its class.
>
> So going back to your scheme of DECLARE_EVENT_CLASS(), it may make sense
> to have DECLARE_AND_DEFINE_EVENT().
>
>
> DECLARE_EVENT_CLASS(class, ...);
> DEFINE_EVENT(class, foo, ...);
>
> DECLARE_AND_DEFINE_EVENT(bar, ...);

Perhaps being the most descriptive to what it does:

DECLARE_CLASS_AND_DEFINE_EVENT() ?

-- Steve

>
> DEFINE_EVENT(bar, zoo, ...);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

fweisbec at gmail

Nov 26, 2009, 3:13 PM

Post #11 of 12 (206 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Thu, Nov 26, 2009 at 02:44:27PM -0500, Steven Rostedt wrote:
> On Thu, 2009-11-26 at 20:20 +0100, Ingo Molnar wrote:
> > I think we generally want to encourage the creation of classes of
> > events, not myriads of standalone events, each with their own call
> > signature, record format and printouts.
> >
> > In that sense making the TRACE_EVENT() one longer would achieve that
> > goal of discouraging its over-use: DEFINE_SINGLE_EVENT() tells the
> > developer that it's an event of it's kind.
>
> But I do agree with Frederic that this can be a little confusing, since
> it makes it sound like DEFINE_EVENT is for multiple events.
>
> What about saying exactly what it does?
>
> DECLARE_AND_DEFINE_EVENT()

It tells so much that it is confusing :)

>
> Come to think of it, since current TRACE_EVENT is now just:
>
> #define TRACE_EVENT() \
> TRACE_EVENT_TEMPLATE() \
> DEFINE_EVENT
>
> This may make the most sense. I haven't tried it, but I believe that you
> could even base other events off of the TRACE_EVENT. That is:
>
> TRACE_EVENT(x, ...);
>
> DEFINE_EVENT(x, y, ...);
>
> And y would use x as its class.
>
> So going back to your scheme of DECLARE_EVENT_CLASS(), it may make sense
> to have DECLARE_AND_DEFINE_EVENT().
>
>
> DECLARE_EVENT_CLASS(class, ...);
> DEFINE_EVENT(class, foo, ...);
>
> DECLARE_AND_DEFINE_EVENT(bar, ...);

Yep, or DEFINE_EVENT_NOCLASS.

> DEFINE_EVENT(bar, zoo, ...);
>
>
> May work.
>
> -- Steve
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Nov 26, 2009, 7:52 PM

Post #12 of 12 (205 views)
Permalink

Re: [tip:perf/core] events: Rename TRACE_EVENT_TEMPLATE() to DECLARE_EVENT_CLASS() [In reply to]

On Fri, 2009-11-27 at 00:13 +0100, Frederic Weisbecker wrote:

> >
> > But I do agree with Frederic that this can be a little confusing, since
> > it makes it sound like DEFINE_EVENT is for multiple events.
> >
> > What about saying exactly what it does?
> >
> > DECLARE_AND_DEFINE_EVENT()
>
>
> It tells so much that it is confusing :)

Information overload huh? ;-)

>
>
> >
> > Come to think of it, since current TRACE_EVENT is now just:
> >
> > #define TRACE_EVENT() \
> > TRACE_EVENT_TEMPLATE() \
> > DEFINE_EVENT
> >
> > This may make the most sense. I haven't tried it, but I believe that you
> > could even base other events off of the TRACE_EVENT. That is:
> >
> > TRACE_EVENT(x, ...);
> >
> > DEFINE_EVENT(x, y, ...);
> >
> > And y would use x as its class.
> >
> > So going back to your scheme of DECLARE_EVENT_CLASS(), it may make sense
> > to have DECLARE_AND_DEFINE_EVENT().
> >
> >
> > DECLARE_EVENT_CLASS(class, ...);
> > DEFINE_EVENT(class, foo, ...);
> >
> > DECLARE_AND_DEFINE_EVENT(bar, ...);
>
>
>
> Yep, or DEFINE_EVENT_NOCLASS.

Well it may not be upper class, but I wouldn't say it has no class ;-)

But seriously, that is more misleading. It is a class. Remember, that
TRACE_EVENT is both a class and a define. With the new names for
template trace_event is:

#define TRACE_EVENT(name, ...) \
DECLARE_EVENT_CLASS(name, ...); \
DEFINE_EVENT(name, name, ...);

So TRACE_EVENT really is a DECLARE_CLASS_AND_DEFINE_EVENT(name, ...);

-- Steve

>
>
>
> > DEFINE_EVENT(bar, zoo, ...);
> >
> >
> > May work.
> >
> > -- Steve
> >
> >
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads