[RFC patch 0/5] Trace event fixes and cleanups

[RFC patch 0/5] Trace event fixes and cleanups

[Mathieu Desnoyers]

Hi,

I noticed a 2 bugs in TRACE_EVENT() users (unassigned fields), along with 3
inconsistencies in the Ftrace comments and macros, which should be considered as
cleanups.

I made sure to only include trivial patches that do not change anything
ABI-wise.

(tested against -tip)

Comments are welcome,

Thanks,

Mathieu

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

[RFC patch 1/5] trace event block fix unassigned field

[Mathieu Desnoyers]

[-- Attachment #1: trace-event-block-fix-unassigned-field.patch --]
[-- Type: text/plain, Size: 1153 bytes --]

The "error" field in block_bio_complete is not assigned, leaving the memory area
uninitialized (keeping garbage data). Initialize it to 0.

We should eventually remove this field when we find out if blktrace can live
without it.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Jeff Moyer <jmoyer@redhat.com>
CC: Jens Axboe <jens.axboe@oracle.com>
CC: Li Zefan <lizf@cn.fujitsu.com>
CC: Alan.Brunelle@hp.com
---
 include/trace/events/block.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6-lttng/include/trace/events/block.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/events/block.h
+++ linux-2.6-lttng/include/trace/events/block.h
@@ -228,6 +228,7 @@ TRACE_EVENT(block_bio_complete,
 		__entry->dev		= bio->bi_bdev->bd_dev;
 		__entry->sector		= bio->bi_sector;
 		__entry->nr_sector	= bio->bi_size >> 9;
+		__entry->error		= 0;
 		blk_fill_rwbs(__entry->rwbs, bio->bi_rw, bio->bi_size);
 	),
 

Re: [RFC patch 1/5] trace event block fix unassigned field

[Jeff Moyer]

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> The "error" field in block_bio_complete is not assigned, leaving the memory area
> uninitialized (keeping garbage data). Initialize it to 0.
>
> We should eventually remove this field when we find out if blktrace can live
> without it.

Well, I'm fairly sure blkparse has the ability to print this field out,
so we should probably just fill it in properly.  Something like the
following untested patch should do.

Cheers,
Jeff

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..f4d83f2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -630,7 +630,7 @@ static void dec_pending(struct dm_io *io, int error)
 			queue_io(md, bio);
 		} else {
 			/* done with normal IO or empty flush */
-			trace_block_bio_complete(md->queue, bio);
+			trace_block_bio_complete(md->queue, bio, io_error);
 			bio_endio(bio, io_error);
 		}
 	}
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index d8ce278..8990a62 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -212,7 +212,7 @@ TRACE_EVENT(block_bio_bounce,
  */
 TRACE_EVENT(block_bio_complete,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio),
+	TP_PROTO(struct request_queue *q, struct bio *bio, int error),
 
 	TP_ARGS(q, bio),
 
@@ -228,6 +228,7 @@ TRACE_EVENT(block_bio_complete,
 		__entry->dev		= bio->bi_bdev->bd_dev;
 		__entry->sector		= bio->bi_sector;
 		__entry->nr_sector	= bio->bi_size >> 9;
+		__entry->error		= error;
 		blk_fill_rwbs(__entry->rwbs, bio->bi_rw, bio->bi_size);
 	),
 

Re: [RFC patch 1/5] trace event block fix unassigned field

[Mathieu Desnoyers]

* Jeff Moyer (jmoyer@redhat.com) wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
> > The "error" field in block_bio_complete is not assigned, leaving the memory area
> > uninitialized (keeping garbage data). Initialize it to 0.
> >
> > We should eventually remove this field when we find out if blktrace can live
> > without it.
> 
> Well, I'm fairly sure blkparse has the ability to print this field out,
> so we should probably just fill it in properly.  Something like the
> following untested patch should do.

I updated your patch slightly (documentation and build fix). It should be fine
now. Thanks!

Mathieu


trace event block fix unassigned field

The "error" field in block_bio_complete is not assigned, leaving the memory area
uninitialized (keeping garbage data). Pass an additional tracepoint argument to
this event to initialize this field.

From: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Jens Axboe <jens.axboe@oracle.com>
CC: Li Zefan <lizf@cn.fujitsu.com>
CC: Alan.Brunelle@hp.com
---
 drivers/md/dm.c              |    2 +-
 include/trace/events/block.h |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6-lttng/include/trace/events/block.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/events/block.h
+++ linux-2.6-lttng/include/trace/events/block.h
@@ -206,15 +206,16 @@ TRACE_EVENT(block_bio_bounce,
  * block_bio_complete - completed all work on the block operation
  * @q: queue holding the block operation
  * @bio: block operation completed
+ * @error: io error value
  *
  * This tracepoint indicates there is no further work to do on this
  * block IO operation @bio.
  */
 TRACE_EVENT(block_bio_complete,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio),
+	TP_PROTO(struct request_queue *q, struct bio *bio, int error),
 
-	TP_ARGS(q, bio),
+	TP_ARGS(q, bio, error),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev		)
@@ -228,6 +229,7 @@ TRACE_EVENT(block_bio_complete,
 		__entry->dev		= bio->bi_bdev->bd_dev;
 		__entry->sector		= bio->bi_sector;
 		__entry->nr_sector	= bio->bi_size >> 9;
+		__entry->error		= error;
 		blk_fill_rwbs(__entry->rwbs, bio->bi_rw, bio->bi_size);
 	),
 
Index: linux-2.6-lttng/drivers/md/dm.c
===================================================================
--- linux-2.6-lttng.orig/drivers/md/dm.c
+++ linux-2.6-lttng/drivers/md/dm.c
@@ -659,7 +659,7 @@ static void dec_pending(struct dm_io *io
 			free_io(md, io);
 
 			if (io_error != DM_ENDIO_REQUEUE) {
-				trace_block_bio_complete(md->queue, bio);
+				trace_block_bio_complete(md->queue, bio, io_error);
 
 				bio_endio(bio, io_error);
 			}


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

Re: [RFC patch 1/5] trace event block fix unassigned field

[Jeff Moyer]

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> * Jeff Moyer (jmoyer@redhat.com) wrote:
>> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
>> 
>> > The "error" field in block_bio_complete is not assigned, leaving the memory area
>> > uninitialized (keeping garbage data). Initialize it to 0.
>> >
>> > We should eventually remove this field when we find out if blktrace can live
>> > without it.
>> 
>> Well, I'm fairly sure blkparse has the ability to print this field out,
>> so we should probably just fill it in properly.  Something like the
>> following untested patch should do.
>
> I updated your patch slightly (documentation and build fix). It should be fine
> now. Thanks!

Thanks a lot!

Cheers,
Jeff

> trace event block fix unassigned field
>
> The "error" field in block_bio_complete is not assigned, leaving the memory area
> uninitialized (keeping garbage data). Pass an additional tracepoint argument to
> this event to initialize this field.
>
> From: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Jens Axboe <jens.axboe@oracle.com>
> CC: Li Zefan <lizf@cn.fujitsu.com>
> CC: Alan.Brunelle@hp.com
> ---
>  drivers/md/dm.c              |    2 +-
>  include/trace/events/block.h |    6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> Index: linux-2.6-lttng/include/trace/events/block.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/trace/events/block.h
> +++ linux-2.6-lttng/include/trace/events/block.h
> @@ -206,15 +206,16 @@ TRACE_EVENT(block_bio_bounce,
>   * block_bio_complete - completed all work on the block operation
>   * @q: queue holding the block operation
>   * @bio: block operation completed
> + * @error: io error value
>   *
>   * This tracepoint indicates there is no further work to do on this
>   * block IO operation @bio.
>   */
>  TRACE_EVENT(block_bio_complete,
>  
> -	TP_PROTO(struct request_queue *q, struct bio *bio),
> +	TP_PROTO(struct request_queue *q, struct bio *bio, int error),
>  
> -	TP_ARGS(q, bio),
> +	TP_ARGS(q, bio, error),
>  
>  	TP_STRUCT__entry(
>  		__field( dev_t,		dev		)
> @@ -228,6 +229,7 @@ TRACE_EVENT(block_bio_complete,
>  		__entry->dev		= bio->bi_bdev->bd_dev;
>  		__entry->sector		= bio->bi_sector;
>  		__entry->nr_sector	= bio->bi_size >> 9;
> +		__entry->error		= error;
>  		blk_fill_rwbs(__entry->rwbs, bio->bi_rw, bio->bi_size);
>  	),
>  
> Index: linux-2.6-lttng/drivers/md/dm.c
> ===================================================================
> --- linux-2.6-lttng.orig/drivers/md/dm.c
> +++ linux-2.6-lttng/drivers/md/dm.c
> @@ -659,7 +659,7 @@ static void dec_pending(struct dm_io *io
>  			free_io(md, io);
>  
>  			if (io_error != DM_ENDIO_REQUEUE) {
> -				trace_block_bio_complete(md->queue, bio);
> +				trace_block_bio_complete(md->queue, bio, io_error);
>  
>  				bio_endio(bio, io_error);
>  			}

[RFC patch 2/5] trace event skb fix unassigned field

[Mathieu Desnoyers]

[-- Attachment #1: trace-event-skb-fix-unassigned-field.patch --]
[-- Type: text/plain, Size: 985 bytes --]

The field "protocol" in event kfree_skb is left unassigned if skb is NULL,
leaving its trace output as garbage. Assign the value to 0 when skb is NULL
instead.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 include/trace/events/skb.h |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: linux-2.6-lttng/include/trace/events/skb.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/events/skb.h
+++ linux-2.6-lttng/include/trace/events/skb.h
@@ -25,9 +25,7 @@ TRACE_EVENT(kfree_skb,
 
 	TP_fast_assign(
 		__entry->skbaddr = skb;
-		if (skb) {
-			__entry->protocol = ntohs(skb->protocol);
-		}
+		__entry->protocol = skb ? ntohs(skb->protocol) : 0;
 		__entry->location = location;
 	),
 

[RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

[-- Attachment #1: ftrace-trace-event-add-missing-semicolumn.patch --]
[-- Type: text/plain, Size: 1084 bytes --]

Add a missing semicolumn at the end of a ftrace definition.

We currently are not seeing any impact of this missing semicolumn because extra
semicolumns appear all over the place in the code generated from TRACE_EVENT
within ftrace stages.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/ftrace.h
+++ linux-2.6-lttng/include/trace/ftrace.h
@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args)	\
 	static struct ftrace_event_call	__used		\
-	__attribute__((__aligned__(4))) event_##name
+	__attribute__((__aligned__(4))) event_##name;
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> Add a missing semicolumn at the end of a ftrace definition.
> 
> We currently are not seeing any impact of this missing semicolumn because extra
> semicolumns appear all over the place in the code generated from TRACE_EVENT
> within ftrace stages.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/trace/ftrace.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6-lttng/include/trace/ftrace.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/trace/ftrace.h
> +++ linux-2.6-lttng/include/trace/ftrace.h
> @@ -69,7 +69,7 @@
>  #undef DEFINE_EVENT
>  #define DEFINE_EVENT(template, name, proto, args)	\
>  	static struct ftrace_event_call	__used		\
> -	__attribute__((__aligned__(4))) event_##name
> +	__attribute__((__aligned__(4))) event_##name;

But DEFINE_EVENT() calls are supposed to be ";" terminated, no?

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> > Add a missing semicolumn at the end of a ftrace definition.
> > 
> > We currently are not seeing any impact of this missing semicolumn because extra
> > semicolumns appear all over the place in the code generated from TRACE_EVENT
> > within ftrace stages.
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > CC: Steven Rostedt <rostedt@goodmis.org>
> > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > CC: Ingo Molnar <mingo@elte.hu>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  include/trace/ftrace.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-2.6-lttng/include/trace/ftrace.h
> > ===================================================================
> > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > +++ linux-2.6-lttng/include/trace/ftrace.h
> > @@ -69,7 +69,7 @@
> >  #undef DEFINE_EVENT
> >  #define DEFINE_EVENT(template, name, proto, args)	\
> >  	static struct ftrace_event_call	__used		\
> > -	__attribute__((__aligned__(4))) event_##name
> > +	__attribute__((__aligned__(4))) event_##name;
> 
> But DEFINE_EVENT() calls are supposed to be ";" terminated, no?

Currently yes, but if you look at the preprocessor output currently generated by
the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all
over the place. I have a patch later in my queue that proposes removal of these
extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for
later because it removes the extra ";" at the end of each TRACE_EVENT()
instance (and thus is more intrusive code-wise).

Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
semicolumns. We can get away without consistency currently exactly because the
current scheme adds many useless semicolumns between each TRACE_EVENT().

Thanks,

Mathieu

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

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> > > Add a missing semicolumn at the end of a ftrace definition.
> > > 
> > > We currently are not seeing any impact of this missing semicolumn because extra
> > > semicolumns appear all over the place in the code generated from TRACE_EVENT
> > > within ftrace stages.
> > > 
> > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > CC: Steven Rostedt <rostedt@goodmis.org>
> > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > CC: Ingo Molnar <mingo@elte.hu>
> > > CC: Thomas Gleixner <tglx@linutronix.de>
> > > ---
> > >  include/trace/ftrace.h |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6-lttng/include/trace/ftrace.h
> > > ===================================================================
> > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > @@ -69,7 +69,7 @@
> > >  #undef DEFINE_EVENT
> > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > >  	static struct ftrace_event_call	__used		\
> > > -	__attribute__((__aligned__(4))) event_##name
> > > +	__attribute__((__aligned__(4))) event_##name;
> > 
> > But DEFINE_EVENT() calls are supposed to be ";" terminated, no?
> 
> Currently yes, but if you look at the preprocessor output currently generated by
> the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all
> over the place. I have a patch later in my queue that proposes removal of these
> extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for
> later because it removes the extra ";" at the end of each TRACE_EVENT()
> instance (and thus is more intrusive code-wise).
> 
> Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> semicolumns. We can get away without consistency currently exactly because the
> current scheme adds many useless semicolumns between each TRACE_EVENT().

Are you sure you want to put so much time on this?

This will require a massive change for the sole win of removing double ";"
in generated code. This won't optimize much the build, and it will make the things
not so much more readable for very rare people who dare to have interest into the
TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
code resides more in the lack of indentation and newlines than in double
semicolons that we barely notice.

Hm?

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> > > > Add a missing semicolumn at the end of a ftrace definition.
> > > > 
> > > > We currently are not seeing any impact of this missing semicolumn because extra
> > > > semicolumns appear all over the place in the code generated from TRACE_EVENT
> > > > within ftrace stages.
> > > > 
> > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > CC: Steven Rostedt <rostedt@goodmis.org>
> > > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > > CC: Ingo Molnar <mingo@elte.hu>
> > > > CC: Thomas Gleixner <tglx@linutronix.de>
> > > > ---
> > > >  include/trace/ftrace.h |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6-lttng/include/trace/ftrace.h
> > > > ===================================================================
> > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > > @@ -69,7 +69,7 @@
> > > >  #undef DEFINE_EVENT
> > > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > > >  	static struct ftrace_event_call	__used		\
> > > > -	__attribute__((__aligned__(4))) event_##name
> > > > +	__attribute__((__aligned__(4))) event_##name;
> > > 
> > > But DEFINE_EVENT() calls are supposed to be ";" terminated, no?
> > 
> > Currently yes, but if you look at the preprocessor output currently generated by
> > the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all
> > over the place. I have a patch later in my queue that proposes removal of these
> > extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for
> > later because it removes the extra ";" at the end of each TRACE_EVENT()
> > instance (and thus is more intrusive code-wise).
> > 
> > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> > semicolumns. We can get away without consistency currently exactly because the
> > current scheme adds many useless semicolumns between each TRACE_EVENT().
> 
> Are you sure you want to put so much time on this?

We are about to spend more time arguing about it that the time it takes cleaning
it up. But here we go.

> 
> This will require a massive change for the sole win of removing double ";"
> in generated code. This won't optimize much the build, and it will make the things
> not so much more readable for very rare people who dare to have interest into the
> TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
> code resides more in the lack of indentation and newlines than in double
> semicolons that we barely notice.

Building:

make kernel/sched.o V=1

taking the gcc invokation, changing it to:

gcc -Wp,-MD,kernel/.sched.o.d  -nostdinc -isystem /usr/local/lib/gcc/x86_64-unknown-linux-gnu/4.5.1/include -I/home/compudj/git/linux-tip/arch/x86/include -Iinclude  -include include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -march=core2 -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -Wframe-larger-than=1024 -fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(sched)"  -D"KBUILD_MODNAME=KBUILD_STR(sched)"  -E -o kernel/sched.pp kernel/sched.c

An exerpt of the output, fed through "indent" for readability:

static inline __attribute__ ((always_inline))
     void
# 275 "include/trace/events/sched.h"
       check_trace_callback_type_sched_process_fork
# 252 "include/trace/events/sched.h"
      
       (void (*cb)
        (void *__data, struct task_struct * parent,
         struct task_struct * child))
{
}
# 275 "include/trace/events/sched.h"
 ;






# 305 "include/trace/events/sched.h"
 ;


As we can notice, a few extra ";" are added between each "entity" created by the
ftrace trace event phase. This works only as long as we declare
semicolumn-separated C structure elements, functions, and statements, because
the compiler just skips the extra semicolumns, but forbids creation of arrays of
events, which need to be comma-separated.

These extra semicolumns we see here are simply polluting the compiler input, and
I don't see any reason why we should leave them there.

Thanks,

Mathieu

> 
> Hm?

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

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Tue, Jan 04, 2011 at 09:35:41PM -0500, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:
> > > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > > On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> > > > > Add a missing semicolumn at the end of a ftrace definition.
> > > > > 
> > > > > We currently are not seeing any impact of this missing semicolumn because extra
> > > > > semicolumns appear all over the place in the code generated from TRACE_EVENT
> > > > > within ftrace stages.
> > > > > 
> > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > CC: Steven Rostedt <rostedt@goodmis.org>
> > > > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > > > CC: Ingo Molnar <mingo@elte.hu>
> > > > > CC: Thomas Gleixner <tglx@linutronix.de>
> > > > > ---
> > > > >  include/trace/ftrace.h |    2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > Index: linux-2.6-lttng/include/trace/ftrace.h
> > > > > ===================================================================
> > > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > > > @@ -69,7 +69,7 @@
> > > > >  #undef DEFINE_EVENT
> > > > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > > > >  	static struct ftrace_event_call	__used		\
> > > > > -	__attribute__((__aligned__(4))) event_##name
> > > > > +	__attribute__((__aligned__(4))) event_##name;
> > > > 
> > > > But DEFINE_EVENT() calls are supposed to be ";" terminated, no?
> > > 
> > > Currently yes, but if you look at the preprocessor output currently generated by
> > > the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all
> > > over the place. I have a patch later in my queue that proposes removal of these
> > > extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for
> > > later because it removes the extra ";" at the end of each TRACE_EVENT()
> > > instance (and thus is more intrusive code-wise).
> > > 
> > > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> > > semicolumns. We can get away without consistency currently exactly because the
> > > current scheme adds many useless semicolumns between each TRACE_EVENT().
> > 
> > Are you sure you want to put so much time on this?
> 
> We are about to spend more time arguing about it that the time it takes cleaning
> it up. But here we go.

Hardly, as the real cleanup requires dozens of patches to clean every trace events.

But dropping the requirement of a ";" after the macro calls still sounds sensible.
 
> > 
> > This will require a massive change for the sole win of removing double ";"
> > in generated code. This won't optimize much the build, and it will make the things
> > not so much more readable for very rare people who dare to have interest into the
> > TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
> > code resides more in the lack of indentation and newlines than in double
> > semicolons that we barely notice.
> 
> Building:
> 
> make kernel/sched.o V=1
> 
> taking the gcc invokation, changing it to:
> 
> gcc -Wp,-MD,kernel/.sched.o.d  -nostdinc -isystem /usr/local/lib/gcc/x86_64-unknown-linux-gnu/4.5.1/include -I/home/compudj/git/linux-tip/arch/x86/include -Iinclude  -include include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -march=core2 -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -Wframe-larger-than=1024 -fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(sched)"  -D"KBUILD_MODNAME=KBUILD_STR(sched)"  -E -o kernel/sched.pp kernel/sched.c
> 
> An exerpt of the output, fed through "indent" for readability:
> 
> static inline __attribute__ ((always_inline))
>      void
> # 275 "include/trace/events/sched.h"
>        check_trace_callback_type_sched_process_fork
> # 252 "include/trace/events/sched.h"
>       
>        (void (*cb)
>         (void *__data, struct task_struct * parent,
>          struct task_struct * child))
> {
> }
> # 275 "include/trace/events/sched.h"
>  ;
> 
> 
> 
> 
> 
> 
> # 305 "include/trace/events/sched.h"
>  ;
> 
> 
> As we can notice, a few extra ";" are added between each "entity" created by the
> ftrace trace event phase. This works only as long as we declare
> semicolumn-separated C structure elements, functions, and statements, because
> the compiler just skips the extra semicolumns, but forbids creation of arrays of
> events, which need to be comma-separated.

So what are these arrays of events you have in mind?

> These extra semicolumns we see here are simply polluting the compiler input, and
> I don't see any reason why we should leave them there.

Sure they are useless, I just wonder if that alone justifies such a massive change, although
actually I don't care much :)

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Tue, Jan 04, 2011 at 09:35:41PM -0500, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:
> > > > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > > > On Tue, Jan 04, 2011 at 06:16:32PM -0500, Mathieu Desnoyers wrote:
> > > > > > Add a missing semicolumn at the end of a ftrace definition.
> > > > > > 
> > > > > > We currently are not seeing any impact of this missing semicolumn because extra
> > > > > > semicolumns appear all over the place in the code generated from TRACE_EVENT
> > > > > > within ftrace stages.
> > > > > > 
> > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > > CC: Steven Rostedt <rostedt@goodmis.org>
> > > > > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > > > > CC: Ingo Molnar <mingo@elte.hu>
> > > > > > CC: Thomas Gleixner <tglx@linutronix.de>
> > > > > > ---
> > > > > >  include/trace/ftrace.h |    2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > Index: linux-2.6-lttng/include/trace/ftrace.h
> > > > > > ===================================================================
> > > > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > > > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > > > > @@ -69,7 +69,7 @@
> > > > > >  #undef DEFINE_EVENT
> > > > > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > > > > >  	static struct ftrace_event_call	__used		\
> > > > > > -	__attribute__((__aligned__(4))) event_##name
> > > > > > +	__attribute__((__aligned__(4))) event_##name;
> > > > > 
> > > > > But DEFINE_EVENT() calls are supposed to be ";" terminated, no?
> > > > 
> > > > Currently yes, but if you look at the preprocessor output currently generated by
> > > > the current TRACE_EVENT()/DEFINE_EVENT() scheme, there are useless ";" added all
> > > > over the place. I have a patch later in my queue that proposes removal of these
> > > > extra ";" as a cleanup of the TRACE_EVENT() semantic, but I'm keeping it for
> > > > later because it removes the extra ";" at the end of each TRACE_EVENT()
> > > > instance (and thus is more intrusive code-wise).
> > > > 
> > > > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> > > > semicolumns. We can get away without consistency currently exactly because the
> > > > current scheme adds many useless semicolumns between each TRACE_EVENT().
> > > 
> > > Are you sure you want to put so much time on this?
> > 
> > We are about to spend more time arguing about it that the time it takes cleaning
> > it up. But here we go.
> 
> Hardly, as the real cleanup requires dozens of patches to clean every trace
> events.

I know, but it's hardly a large change in terms of lines of code, not
complexity.

> 
> But dropping the requirement of a ";" after the macro calls still sounds
> sensible.

Glad to hear that :) More information below,

>  
> > > 
> > > This will require a massive change for the sole win of removing double ";"
> > > in generated code. This won't optimize much the build, and it will make the things
> > > not so much more readable for very rare people who dare to have interest into the
> > > TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
> > > code resides more in the lack of indentation and newlines than in double
> > > semicolons that we barely notice.
> > 
> > Building:
> > 
> > make kernel/sched.o V=1
> > 
> > taking the gcc invokation, changing it to:
> > 
> > gcc -Wp,-MD,kernel/.sched.o.d  -nostdinc -isystem /usr/local/lib/gcc/x86_64-unknown-linux-gnu/4.5.1/include -I/home/compudj/git/linux-tip/arch/x86/include -Iinclude  -include include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -march=core2 -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -Wframe-larger-than=1024 -fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(sched)"  -D"KBUILD_MODNAME=KBUILD_STR(sched)"  -E -o kernel/sched.pp kernel/sched.c
> > 
> > An exerpt of the output, fed through "indent" for readability:
> > 
> > static inline __attribute__ ((always_inline))
> >      void
> > # 275 "include/trace/events/sched.h"
> >        check_trace_callback_type_sched_process_fork
> > # 252 "include/trace/events/sched.h"
> >       
> >        (void (*cb)
> >         (void *__data, struct task_struct * parent,
> >          struct task_struct * child))
> > {
> > }
> > # 275 "include/trace/events/sched.h"
> >  ;
> > 
> > 
> > 
> > 
> > 
> > 
> > # 305 "include/trace/events/sched.h"
> >  ;
> > 
> > 
> > As we can notice, a few extra ";" are added between each "entity" created by the
> > ftrace trace event phase. This works only as long as we declare
> > semicolumn-separated C structure elements, functions, and statements, because
> > the compiler just skips the extra semicolumns, but forbids creation of arrays of
> > events, which need to be comma-separated.
> 
> So what are these arrays of events you have in mind?

Currently, Ftrace is creating a "ftrace_define_fields_##call" function for each
event to define the event fields, which consumes space. It also has the
".fields" list head to keep a dynamically generated list of event fields.

By allowing creation of arrays of events, we can do the following: turn the
dynamically created list of event fields into a first-level const array listing
event fields. We can use ARRAY_SIZE() on this array to know its size,
statically. Then, in a following trace event stage, we can create another const
array containing tuples of (pointers to the event-specific arrays, array size).

So we get all the same information Ftrace currently gets with much less code
overall, much less read-write data, and less dynamic initialization code.

The following relevant code snippets does the trick. It's extracted from my
LTTng git tree. Feel free to use any variation of it if you want.

Thanks,

Mathieu

/*
 * Stage 1 of the trace events.
 *
 * Create event field type metadata section.
 * Each event produce an array of fields.
 */

#include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */

/* Named field types must be defined in lttng-types.h */

#undef __field
#define __field(_type, _item)                                   \
        { .name = #_item, .type = { .atype = atype_integer, .name = #_type} },

[... same for __array, __dynamic_array, __string, ...]

#undef TP_STRUCT__entry
#define TP_STRUCT__entry(args...) args  /* Only one used in this phase */

#undef DECLARE_EVENT_CLASS
#define DECLARE_EVENT_CLASS(_name, _proto, _args, _tstruct, _assign, _print) \
        static const struct lttng_event_field __event_fields___##_name[] = { \
                _tstruct                                                     \
        };

#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)


/*
 * Stage 2 of the trace events.
 *
 * Create an array of events.
 */

/* Named field types must be defined in lttng-types.h */

#include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */

#undef DEFINE_EVENT
#define DEFINE_EVENT(_template, _name, _proto, _args)                          \
                {                                                              \
                        .fields = __event_fields___##_template,                \
                        .name = #_name,                                        \
                        .nr_fields = ARRAY_SIZE(__event_fields___##_template), \
                },

#define TP_ID1(_token, _system) _token##_system
#define TP_ID(_token, _system)  TP_ID1(_token, _system)

static const struct lttng_event_desc TP_ID(__event_desc___, TRACE_SYSTEM)[] = {
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
};

#undef TP_ID1
#undef TP_ID

> 
> > These extra semicolumns we see here are simply polluting the compiler input, and
> > I don't see any reason why we should leave them there.
> 
> Sure they are useless, I just wonder if that alone justifies such a massive change, although
> actually I don't care much :)

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

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Wed, Jan 05, 2011 at 08:52:42AM -0500, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > Hardly, as the real cleanup requires dozens of patches to clean every trace
> > events.
> 
> I know, but it's hardly a large change in terms of lines of code, not
> complexity.

Given the big coverage of such change, requiring routing pieces to relevant
maintainers, care about conflicts, etc... As a reason I'd expect more than just
a semicolon in files.i that almost nobody looks at, except few tracing people
when things break. 

I guess the win is more hiding in the Lttng tree that needs this API requirement
change? It's perfectly fine. But you should come up with stronger arguments to
convince people that the change improves the mainline tracing. The same problem
happened with your __assign() changes lately.

That said I personally don't oppose about the change.


> > So what are these arrays of events you have in mind?
> 
> Currently, Ftrace is creating a "ftrace_define_fields_##call" function for each
> event to define the event fields, which consumes space. It also has the
> ".fields" list head to keep a dynamically generated list of event fields.
> 
> By allowing creation of arrays of events, we can do the following: turn the
> dynamically created list of event fields into a first-level const array listing
> event fields. We can use ARRAY_SIZE() on this array to know its size,
> statically. Then, in a following trace event stage, we can create another const
> array containing tuples of (pointers to the event-specific arrays, array size).
> 
> So we get all the same information Ftrace currently gets with much less code
> overall, much less read-write data, and less dynamic initialization code.
> 
> The following relevant code snippets does the trick. It's extracted from my
> LTTng git tree. Feel free to use any variation of it if you want.
> 
> Thanks,
> 
> Mathieu
> 
> /*
>  * Stage 1 of the trace events.
>  *
>  * Create event field type metadata section.
>  * Each event produce an array of fields.
>  */
> 
> #include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */
> 
> /* Named field types must be defined in lttng-types.h */
> 
> #undef __field
> #define __field(_type, _item)                                   \
>         { .name = #_item, .type = { .atype = atype_integer, .name = #_type} },
> 
> [... same for __array, __dynamic_array, __string, ...]
> 
> #undef TP_STRUCT__entry
> #define TP_STRUCT__entry(args...) args  /* Only one used in this phase */
> 
> #undef DECLARE_EVENT_CLASS
> #define DECLARE_EVENT_CLASS(_name, _proto, _args, _tstruct, _assign, _print) \
>         static const struct lttng_event_field __event_fields___##_name[] = { \
>                 _tstruct                                                     \
>         };
> 
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> 
> 
> /*
>  * Stage 2 of the trace events.
>  *
>  * Create an array of events.
>  */
> 
> /* Named field types must be defined in lttng-types.h */
> 
> #include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */
> 
> #undef DEFINE_EVENT
> #define DEFINE_EVENT(_template, _name, _proto, _args)                          \
>                 {                                                              \
>                         .fields = __event_fields___##_template,                \
>                         .name = #_name,                                        \
>                         .nr_fields = ARRAY_SIZE(__event_fields___##_template), \
>                 },
> 
> #define TP_ID1(_token, _system) _token##_system
> #define TP_ID(_token, _system)  TP_ID1(_token, _system)
> 
> static const struct lttng_event_desc TP_ID(__event_desc___, TRACE_SYSTEM)[] = {
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> };
> 
> #undef TP_ID1
> #undef TP_ID

Looks good!

I might be missing corner things but it seems this would reduce the code
footprint (one function less) and turn more rw into ro datas.

So it seems to be a very valuable reason to change the semicolon requirement
all over the place.

If you come up with this feature along the massive semicolon requirement change,
we will probably happily apply the whole.

But coming with only the semicolon change is more like an empty shell.

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
[...]
> Looks good!
> 
> I might be missing corner things but it seems this would reduce the code
> footprint (one function less) and turn more rw into ro datas.
> 
> So it seems to be a very valuable reason to change the semicolon requirement
> all over the place.
> 
> If you come up with this feature along the massive semicolon requirement
> change, we will probably happily apply the whole.
> 
> But coming with only the semicolon change is more like an empty shell.

My proposal here is to incrementally improve the tracing code, starting by
cleaning up what is already there. I cannot do this if you keep asking me for
larger changes to both Ftrace and Perf before any of the prerequisite cleanups
can make their way in.

In this thread, I demonstrated that the TRACE_EVENT cleanup I proposed opens a
lot of code/data size reduction cleanups for Ftrace and Perf. But let's get the
cleanup in there first (it does not break the current way Ftrace and Perf are
working), and once all the code-base has moved to the semicolumn-less semantic,
then we can start improving Ftrace and Perf.

As we say in French, "il ne faut pas mettre la charrue devant les boeufs"
(roughly: don't put the cart before the horse)

Thanks,

Mathieu

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

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Wed, Jan 05, 2011 at 02:56:12PM -0500, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> [...]
> > Looks good!
> > 
> > I might be missing corner things but it seems this would reduce the code
> > footprint (one function less) and turn more rw into ro datas.
> > 
> > So it seems to be a very valuable reason to change the semicolon requirement
> > all over the place.
> > 
> > If you come up with this feature along the massive semicolon requirement
> > change, we will probably happily apply the whole.
> > 
> > But coming with only the semicolon change is more like an empty shell.
> 
> My proposal here is to incrementally improve the tracing code, starting by
> cleaning up what is already there. I cannot do this if you keep asking me for
> larger changes to both Ftrace and Perf before any of the prerequisite cleanups
> can make their way in.
> 
> In this thread, I demonstrated that the TRACE_EVENT cleanup I proposed opens a
> lot of code/data size reduction cleanups for Ftrace and Perf. But let's get the
> cleanup in there first (it does not break the current way Ftrace and Perf are
> working), and once all the code-base has moved to the semicolumn-less semantic,
> then we can start improving Ftrace and Perf.

Don't be suprised of my reaction. The way the things were presented was:

1) A patch with an meaningless changelog, absolutely no idea why that new
semicolon is useful for.

2) Me asking you why

3) You explaining me it's actually part of a much bigger cleanup for
a goal almost completely useless

4) Me has a hard time considering the big cleanup useful as is. Expressing
my worries.

5) You detailing much more the semicolon symptoms, and finally dropping an
evasive reason to do this cleanup, collapsed in the obsure notion of "arrays
of event"..

6) Me still puzzled by the big cleanup just for the sake of unnoticeable
files.i pollution cleanup. But I try to give some chance by asking to
expand the "arrays of event" idea.

7) You explaining me something that looks like a meaningful, useful reason for
the big cleanup, but as a possibility. You propose me that I reuse the Lttng code that
you pick as an example, not suggesting you're planing to handle that part.

8) Me: ok for the big cleanup then, if you plan to also handle the useful change
you detailed, because otherwise it's meaningless.

See? That meaningful reason came so late in the discussion, and was proposed
so much under the angle of the possibility rather than something you plan
that I just thought you would just drop that empty shell and run away.

Hence my worries.

But if you plan to do the thing incrementally, this is also perfectly ok.

And you know what, may be you won't eventually have the time to complete
with the useful part. And may be someone else will handle that part.
Whatever, it's ok. We don't always finish everything.
But at least fill your changelogs with the useful longterm goal so that we
know we are going somewhere with this.


> As we say in French, "il ne faut pas mettre la charrue devant les boeufs"
> (roughly: don't put the cart before the horse)

As we say in French, "oui mais il a fallu te tirer les vers du nez"
(roughly: right, but still I had to worm it out of you)

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Steven Rostedt]

On Thu, 2011-01-06 at 00:40 +0100, Frederic Weisbecker wrote:
> On Wed, Jan 05, 2011 at 02:56:12PM -0500, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > [...]
> > > Looks good!
> > > 
> > > I might be missing corner things but it seems this would reduce the code
> > > footprint (one function less) and turn more rw into ro datas.
> > > 
> > > So it seems to be a very valuable reason to change the semicolon requirement
> > > all over the place.
> > > 
> > > If you come up with this feature along the massive semicolon requirement
> > > change, we will probably happily apply the whole.
> > > 
> > > But coming with only the semicolon change is more like an empty shell.
> > 
> > My proposal here is to incrementally improve the tracing code, starting by
> > cleaning up what is already there. I cannot do this if you keep asking me for
> > larger changes to both Ftrace and Perf before any of the prerequisite cleanups
> > can make their way in.
> > 
> > In this thread, I demonstrated that the TRACE_EVENT cleanup I proposed opens a
> > lot of code/data size reduction cleanups for Ftrace and Perf. But let's get the
> > cleanup in there first (it does not break the current way Ftrace and Perf are
> > working), and once all the code-base has moved to the semicolumn-less semantic,
> > then we can start improving Ftrace and Perf.
> 
> Don't be suprised of my reaction. The way the things were presented was:
> 
> 1) A patch with an meaningless changelog, absolutely no idea why that new
> semicolon is useful for.

Exactly... From the original change log:

"Add a missing semicolumn at the end of a ftrace definition.

We currently are not seeing any impact of this missing semicolumn because extra
semicolumns appear all over the place in the code generated from TRACE_EVENT
within ftrace stages."

Basically you state: We add this semicolon because ftrace already pushes
out lots of semicolons, so why not add more.

This is a totally useless changelog, and worthy of a nak because it has
no basis.

Yes, if there is a reason for doing this then state it.


> > As we say in French, "il ne faut pas mettre la charrue devant les boeufs"
> > (roughly: don't put the cart before the horse)
> 
> As we say in French, "oui mais il a fallu te tirer les vers du nez"
> (roughly: right, but still I had to worm it out of you)

As we say in English: "If it ain't broke, don't fix it!"

(roughly: Si ce n'est pas cassé, ne le correctif)

-- Steve

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Wed, Jan 05, 2011 at 02:56:12PM -0500, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > [...]
> > > Looks good!
> > > 
> > > I might be missing corner things but it seems this would reduce the code
> > > footprint (one function less) and turn more rw into ro datas.
> > > 
> > > So it seems to be a very valuable reason to change the semicolon requirement
> > > all over the place.
> > > 
> > > If you come up with this feature along the massive semicolon requirement
> > > change, we will probably happily apply the whole.
> > > 
> > > But coming with only the semicolon change is more like an empty shell.
> > 
> > My proposal here is to incrementally improve the tracing code, starting by
> > cleaning up what is already there. I cannot do this if you keep asking me for
> > larger changes to both Ftrace and Perf before any of the prerequisite cleanups
> > can make their way in.
> > 
> > In this thread, I demonstrated that the TRACE_EVENT cleanup I proposed opens a
> > lot of code/data size reduction cleanups for Ftrace and Perf. But let's get the
> > cleanup in there first (it does not break the current way Ftrace and Perf are
> > working), and once all the code-base has moved to the semicolumn-less semantic,
> > then we can start improving Ftrace and Perf.
> 
> Don't be suprised of my reaction. The way the things were presented was:
> 
> 1) A patch with an meaningless changelog, absolutely no idea why that new
> semicolon is useful for.
> 
> 2) Me asking you why
> 
> 3) You explaining me it's actually part of a much bigger cleanup for
> a goal almost completely useless
> 
> 4) Me has a hard time considering the big cleanup useful as is. Expressing
> my worries.
> 
> 5) You detailing much more the semicolon symptoms, and finally dropping an
> evasive reason to do this cleanup, collapsed in the obsure notion of "arrays
> of event"..
> 
> 6) Me still puzzled by the big cleanup just for the sake of unnoticeable
> files.i pollution cleanup. But I try to give some chance by asking to
> expand the "arrays of event" idea.
> 
> 7) You explaining me something that looks like a meaningful, useful reason for
> the big cleanup, but as a possibility. You propose me that I reuse the Lttng code that
> you pick as an example, not suggesting you're planing to handle that part.
> 
> 8) Me: ok for the big cleanup then, if you plan to also handle the useful change
> you detailed, because otherwise it's meaningless.
> 
> See? That meaningful reason came so late in the discussion, and was proposed
> so much under the angle of the possibility rather than something you plan
> that I just thought you would just drop that empty shell and run away.
> 
> Hence my worries.
> 
> But if you plan to do the thing incrementally, this is also perfectly ok.
> 
> And you know what, may be you won't eventually have the time to complete
> with the useful part. And may be someone else will handle that part.
> Whatever, it's ok. We don't always finish everything.
> But at least fill your changelogs with the useful longterm goal so that we
> know we are going somewhere with this.

Fair enough. I clearly see that for you it is tremendousy important to
understand what is the advantage of merging one of my patches, so I'll try to
better outline my motivation in the future. I seem to have misunderstood you
initially, thinking that you wanted me to push a large monster-patchset
- or nothing, which made me reluctant to tell you more about my plans. But if we
agree on working on these things incrementally, then I think we'll automatically
start benefiting from each other's work.

> > As we say in French, "il ne faut pas mettre la charrue devant les boeufs"
> > (roughly: don't put the cart before the horse)
> 
> As we say in French, "oui mais il a fallu te tirer les vers du nez"
> (roughly: right, but still I had to worm it out of you)

Thanks for coping with my attitude here. I might indeed not have been as
straightforward as I should. I'll work on that. Knowing that you are open about
incremental improvements, and that it's not a "all or nothing" policy, makes me
much more inclined to describe my motivation.

Mathieu

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

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Valdis.Kletnieks]

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

On Wed, 05 Jan 2011 03:08:02 +0100, Frederic Weisbecker said:
> On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:

> > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > > @@ -69,7 +69,7 @@
> > > >  #undef DEFINE_EVENT
> > > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > > >  	static struct ftrace_event_call	__used		\
> > > > -	__attribute__((__aligned__(4))) event_##name
> > > > +	__attribute__((__aligned__(4))) event_##name;

> > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> > semicolumns. We can get away without consistency currently exactly because the
> > current scheme adds many useless semicolumns between each TRACE_EVENT().

> Are you sure you want to put so much time on this?

> This will require a massive change for the sole win of removing double ";"
> in generated code. This won't optimize much the build, and it will make the things
> not so much more readable for very rare people who dare to have interest into the
> TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
> code resides more in the lack of indentation and newlines than in double
> semicolons that we barely notice.

Can DEFINE_EVENT ever be sensibly used in a context where the additional ; will
cause an issue (for instance, a hypothetical array initialization like:

static struct events[] = {DEFINE_EVENT(..), DEFINE_EVENT(...) }

or other places we usually do the 'do { X } while (0)' trick to make the code legal?


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Tue, Jan 04, 2011 at 10:01:33PM -0500, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 05 Jan 2011 03:08:02 +0100, Frederic Weisbecker said:
> > On Tue, Jan 04, 2011 at 07:18:37PM -0500, Mathieu Desnoyers wrote:
> 
> > > > > --- linux-2.6-lttng.orig/include/trace/ftrace.h
> > > > > +++ linux-2.6-lttng/include/trace/ftrace.h
> > > > > @@ -69,7 +69,7 @@
> > > > >  #undef DEFINE_EVENT
> > > > >  #define DEFINE_EVENT(template, name, proto, args)	\
> > > > >  	static struct ftrace_event_call	__used		\
> > > > > -	__attribute__((__aligned__(4))) event_##name
> > > > > +	__attribute__((__aligned__(4))) event_##name;
> 
> > > Adding this semicolumn here ensures that all Ftrace macros are consistent wrt
> > > semicolumns. We can get away without consistency currently exactly because the
> > > current scheme adds many useless semicolumns between each TRACE_EVENT().
> 
> > Are you sure you want to put so much time on this?
> 
> > This will require a massive change for the sole win of removing double ";"
> > in generated code. This won't optimize much the build, and it will make the things
> > not so much more readable for very rare people who dare to have interest into the
> > TRACE_EVENT generated code. That notwithstanding the obfuscation of that generated
> > code resides more in the lack of indentation and newlines than in double
> > semicolons that we barely notice.
> 
> Can DEFINE_EVENT ever be sensibly used in a context where the additional ; will
> cause an issue (for instance, a hypothetical array initialization like:
> 
> static struct events[] = {DEFINE_EVENT(..), DEFINE_EVENT(...) }

You can't do the above as DEFINE_EVENT() do more than just creating a structure.
It can define functions and so.

Plus it doesn't behave the same whether CREATE_TRACE_POINTS is defined or not:
it can either define or declare the functions and structures.
 
> or other places we usually do the 'do { X } while (0)' trick to make the code legal?

I just can't figure out a sane case.

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Valdis.Kletnieks]

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

On Wed, 05 Jan 2011 04:10:18 +0100, Frederic Weisbecker said:
> On Tue, Jan 04, 2011 at 10:01:33PM -0500, Valdis.Kletnieks@vt.edu wrote:
> > Can DEFINE_EVENT ever be sensibly used in a context where the additional ; will
> > cause an issue (for instance, a hypothetical array initialization like:
> >
> > static struct events[] = {DEFINE_EVENT(..), DEFINE_EVENT(...) }

> You can't do the above as DEFINE_EVENT() do more than just creating a structure.
> It can define functions and so.
>
> Plus it doesn't behave the same whether CREATE_TRACE_POINTS is defined or not:
> it can either define or declare the functions and structures.
>
> > or other places we usually do the 'do { X } while (0)' trick to make the code legal?
>
> I just can't figure out a sane case.

OK..  I was wondering if there was a corner case where we had to resolve the
one versus two semicolon issue in a specific way to guarantee syntactic
correctness, but it looks like this one gets to fight it out on taste/style
grounds...


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Mathieu Desnoyers]

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

* Valdis.Kletnieks@vt.edu (Valdis.Kletnieks@vt.edu) wrote:
> On Wed, 05 Jan 2011 04:10:18 +0100, Frederic Weisbecker said:
> > On Tue, Jan 04, 2011 at 10:01:33PM -0500, Valdis.Kletnieks@vt.edu wrote:
> > > Can DEFINE_EVENT ever be sensibly used in a context where the additional ; will
> > > cause an issue (for instance, a hypothetical array initialization like:
> > >
> > > static struct events[] = {DEFINE_EVENT(..), DEFINE_EVENT(...) }
> 
> > You can't do the above as DEFINE_EVENT() do more than just creating a structure.
> > It can define functions and so.
> >
> > Plus it doesn't behave the same whether CREATE_TRACE_POINTS is defined or not:
> > it can either define or declare the functions and structures.
> >
> > > or other places we usually do the 'do { X } while (0)' trick to make the code legal?
> >
> > I just can't figure out a sane case.
> 
> OK..  I was wondering if there was a corner case where we had to resolve the
> one versus two semicolon issue in a specific way to guarantee syntactic
> correctness, but it looks like this one gets to fight it out on taste/style
> grounds...

As I pointed out in my reply to Frederic, the creation of an array of events is
the exact use-case I have in mind. It allows us to shrink the code, remove
dynamic initialization code and to shrink read-write data size significantly
over the current Ftrace trace event scheme.

Thanks,

Mathieu


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[RFC patch 4/5] tracepoint trace event add missing comma

[Mathieu Desnoyers]

[-- Attachment #1: tracepoint-trace-event-add-missing-comma.patch --]
[-- Type: text/plain, Size: 910 bytes --]

Impact: documentation cleanup.

Add missing comma within the TRACE_EVENT() example in tracepoint.h.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/tracepoint.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/include/linux/tracepoint.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/tracepoint.h
+++ linux-2.6-lttng/include/linux/tracepoint.h
@@ -312,7 +312,7 @@ static inline void tracepoint_update_pro
  *		memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
  *		__entry->next_pid	= next->pid;
  *		__entry->next_prio	= next->prio;
- *	)
+ *	),
  *
  *	*
  *	* Formatted output of a trace record via TP_printk().

[RFC patch 5/5] trace event sched: remove TP_perf_assign

[Mathieu Desnoyers]

[-- Attachment #1: trace-event-sched-remove-perf-count.patch --]
[-- Type: text/plain, Size: 2128 bytes --]

The TP_perf_assign part of 2 scheduler TRACE_EVENT are not used and don't act as
TRACE_EVENT fields per se.

CC: Steven Rostedt <rostedt@goodmis.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Mike Galbraith <efault@gmx.de>
CC: Paul Mackerras <paulus@samba.org>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 include/trace/events/sched.h |    6 ------
 include/trace/ftrace.h       |    6 ------
 2 files changed, 12 deletions(-)

Index: linux-2.6-lttng/include/trace/events/sched.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/events/sched.h
+++ linux-2.6-lttng/include/trace/events/sched.h
@@ -294,9 +294,6 @@ DECLARE_EVENT_CLASS(sched_stat_template,
 		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
 		__entry->pid	= tsk->pid;
 		__entry->delay	= delay;
-	)
-	TP_perf_assign(
-		__perf_count(delay);
 	),
 
 	TP_printk("comm=%s pid=%d delay=%Lu [ns]",
@@ -351,9 +348,6 @@ TRACE_EVENT(sched_stat_runtime,
 		__entry->pid		= tsk->pid;
 		__entry->runtime	= runtime;
 		__entry->vruntime	= vruntime;
-	)
-	TP_perf_assign(
-		__perf_count(runtime);
 	),
 
 	TP_printk("comm=%s pid=%d runtime=%Lu [ns] vruntime=%Lu [ns]",
Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/ftrace.h
+++ linux-2.6-lttng/include/trace/ftrace.h
@@ -481,9 +481,6 @@ static inline notrace int ftrace_get_off
 #undef TP_fast_assign
 #define TP_fast_assign(args...) args
 
-#undef TP_perf_assign
-#define TP_perf_assign(args...)
-
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\
 									\
@@ -680,9 +677,6 @@ __attribute__((section("_ftrace_events")
 #undef __perf_addr
 #define __perf_addr(a) __addr = (a)
 
-#undef __perf_count
-#define __perf_count(c) __count = (c)
-
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\
 static notrace void							\

Re: [RFC patch 5/5] trace event sched: remove TP_perf_assign

[Peter Zijlstra]

On Tue, 2011-01-04 at 18:16 -0500, Mathieu Desnoyers wrote:
> plain text document attachment (trace-event-sched-remove-perf-count.patch)
> The TP_perf_assign part of 2 scheduler TRACE_EVENT are not used and don't act as
> TRACE_EVENT fields per se.

Hrm,. they should be used, include/trace/ftrace.h uses __perf_count().
If it doesn't actually work them something broke.

So NAK on removing them.

Re: [RFC patch 5/5] trace event sched: remove TP_perf_assign

[Mathieu Desnoyers]

* Peter Zijlstra (a.p.zijlstra@chello.nl) wrote:
> On Tue, 2011-01-04 at 18:16 -0500, Mathieu Desnoyers wrote:
> > plain text document attachment (trace-event-sched-remove-perf-count.patch)
> > The TP_perf_assign part of 2 scheduler TRACE_EVENT are not used and don't act as
> > TRACE_EVENT fields per se.
> 
> Hrm,. they should be used, include/trace/ftrace.h uses __perf_count().
> If it doesn't actually work them something broke.
> 
> So NAK on removing them.

Good point, a fresh look at perf's use of __perf_count() shows that the __count
and __addr local variables are acutally used. I'm therefore dropping this patch.

Thanks,

Mathieu

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