Trace event fixes and cleanups

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.

Quoted 5 lines · Mathieu Desnoyers

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.

Quoted 8 lines · * Jeff Moyer ([email protected]) wrote:

Quoted 5 lines · Mathieu Desnoyers

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.

Quoted 3 lines

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!

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 <[email protected]>

Signed-off-by Jeff Moyer <[email protected]>

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Jens Axboe <[email protected]>

CC Li Zefan <[email protected]>

CC [email protected]

---
 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
===================================================================

include/trace/events/block.h +4 -2

@@ -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
===================================================================

drivers/md/dm.c +1 -1

@@ -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);
 			}

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

Quoted 15 lines · Mathieu Desnoyers

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/trace/ftrace.h line 72

Quoted 10 lines

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +1 -1

@@ -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?

Quoted 16 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 15 lines · Mathieu Desnoyers

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

include/trace/ftrace.h line 72

Quoted 10 lines

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +1 -1

@@ -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().

Quoted 37 lines · Mathieu Desnoyers

Quoted 16 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 15 lines · Mathieu Desnoyers

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

include/trace/ftrace.h line 72

Quoted 10 lines

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +1 -1

@@ -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).

Quoted 3 lines

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?

Quoted 38 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 37 lines · Mathieu Desnoyers

Quoted 16 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 15 lines · Mathieu Desnoyers

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

include/trace/ftrace.h line 72

Quoted 10 lines

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +1 -1

@@ -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).

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 43 lines · Mathieu Desnoyers

Quoted 42 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 40 lines · Mathieu Desnoyers

Quoted 16 lines · * Frederic Weisbecker ([email protected]) wrote:

Quoted 15 lines · Mathieu Desnoyers

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 <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

include/trace/ftrace.h line 72

Quoted 10 lines

Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +1 -1

@@ -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().

Quoted 4 lines

Quoted 1 line

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.

Quoted 42 lines

Quoted 8 lines

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"
;

Quoted 5 lines

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?

Quoted 3 lines

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 :)

Quoted 2 lines · * Frederic Weisbecker ([email protected]) 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.

Quoted 1 line

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.

* Frederic Weisbecker ([email protected]) wrote:
[...]

Quoted 5 lines

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.

Quoted 7 lines

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)

Quoted 20 lines · Mathieu Desnoyers

* Frederic Weisbecker ([email protected]) wrote:
[...]

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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)

Quoted 20 lines · Mathieu Desnoyers

* Frederic Weisbecker ([email protected]) wrote:
[...]

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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)

Quoted 4 lines · Mathieu Desnoyers

Quoted 4 lines

Quoted 4 lines

include/trace/ftrace.h +0 -0

@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT

Quoted 8 lines

Quoted 4 lines

include/trace/ftrace.h line 72

Quoted 4 lines

include/trace/ftrace.h +1 -1

 #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.

Quoted 15 lines · Frederic Weisbecker

Quoted 4 lines · Mathieu Desnoyers

Quoted 4 lines

Quoted 4 lines

include/trace/ftrace.h +0 -0

@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT

Quoted 8 lines

Quoted 4 lines

include/trace/ftrace.h line 72

Quoted 4 lines

include/trace/ftrace.h +1 -1

 #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().

Quoted 8 lines

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?

Quoted 4 lines · On Tue, Jan 04, 2011 at 10:01:33PM -0500, [email protected] 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.

Quoted 1 line

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.

Quoted 12 lines · Frederic Weisbecker

Quoted 4 lines · On Tue, Jan 04, 2011 at 10:01:33PM -0500, [email protected] 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.

Quoted 3 lines

Quoted 1 line

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...

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.

Quoted 3 lines · Mathieu Desnoyers

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.