[PATCH 0/3] a few tracing bugfixes

[PATCH 0/3] a few tracing bugfixes

[Tom Zanussi]

Some fixes for a few things I noticed recently.

Tom Zanussi (3):
  perf trace: Remove unused code in builtin-trace.c
  perf trace: Update eval_flag() flags array to match interrupt.h
  tracing/syscalls: Use long for syscall ret format and field
    definitions

 kernel/trace/trace_syscalls.c       |    4 ++--
 tools/perf/builtin-trace.c          |    6 ------
 tools/perf/util/trace-event-parse.c |    9 +++++----
 3 files changed, 7 insertions(+), 12 deletions(-)

[PATCH 1/3] perf trace: Remove unused code in builtin-trace.c

[Tom Zanussi]

And some minor whitespace cleanup.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
---
 tools/perf/builtin-trace.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 2f93888..5d4c84d 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -191,10 +191,6 @@ remap:
 more:
 	event = (event_t *)(buf + head);
 
-	size = event->header.size;
-	if (!size)
-		size = 8;
-
 	if (head + event->header.size >= page_size * mmap_window) {
 		unsigned long shift = page_size * (head / page_size);
 		int res;
@@ -209,7 +205,6 @@ more:
 
 	size = event->header.size;
 
-
 	if (!size || process_event(event, offset, head) < 0) {
 
 		/*
@@ -262,7 +257,6 @@ int cmd_trace(int argc, const char **argv, const char *prefix __used)
 			usage_with_options(annotate_usage, options);
 	}
 
-
 	setup_pager();
 
 	return __cmd_trace();
-- 
1.6.4.GIT

[PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Tom Zanussi]

Add missing BLOCK_IOPOLL_SOFTIRQ entry.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
---
 tools/perf/util/trace-event-parse.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
index f6a8437..55b41b9 100644
--- a/tools/perf/util/trace-event-parse.c
+++ b/tools/perf/util/trace-event-parse.c
@@ -1968,10 +1968,11 @@ static const struct flag flags[] = {
 	{ "NET_TX_SOFTIRQ", 2 },
 	{ "NET_RX_SOFTIRQ", 3 },
 	{ "BLOCK_SOFTIRQ", 4 },
-	{ "TASKLET_SOFTIRQ", 5 },
-	{ "SCHED_SOFTIRQ", 6 },
-	{ "HRTIMER_SOFTIRQ", 7 },
-	{ "RCU_SOFTIRQ", 8 },
+	{ "BLOCK_IOPOLL_SOFTIRQ", 5 },
+	{ "TASKLET_SOFTIRQ", 6 },
+	{ "SCHED_SOFTIRQ", 7 },
+	{ "HRTIMER_SOFTIRQ", 8 },
+	{ "RCU_SOFTIRQ", 9 },
 
 	{ "HRTIMER_NORESTART", 0 },
 	{ "HRTIMER_RESTART", 1 },
-- 
1.6.4.GIT

[PATCH 3/3] tracing/syscalls: Use long for syscall ret format and field definitions

[Tom Zanussi]

The syscall event definitions use long for the syscall exit ret value,
but unsigned long for the same thing in the format and field
definitions.  Change them all to long.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
---
 kernel/trace/trace_syscalls.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 9fbce6c..527e17e 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -166,7 +166,7 @@ int syscall_exit_format(struct ftrace_event_call *call, struct trace_seq *s)
 			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
 			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
 			       SYSCALL_FIELD(int, nr),
-			       SYSCALL_FIELD(unsigned long, ret));
+			       SYSCALL_FIELD(long, ret));
 	if (!ret)
 		return 0;
 
@@ -212,7 +212,7 @@ int syscall_exit_define_fields(struct ftrace_event_call *call)
 	if (ret)
 		return ret;
 
-	ret = trace_define_field(call, SYSCALL_FIELD(unsigned long, ret), 0,
+	ret = trace_define_field(call, SYSCALL_FIELD(long, ret), 0,
 				 FILTER_OTHER);
 
 	return ret;
-- 
1.6.4.GIT

Re: [PATCH 3/3] tracing/syscalls: Use long for syscall ret format and field definitions

[Frédéric Weisbecker]

2009/10/6, Tom Zanussi <tzanussi@gmail.com>:
> The syscall event definitions use long for the syscall exit ret value,
> but unsigned long for the same thing in the format and field
> definitions.  Change them all to long.


Thanks.

We may perhaps also want to change struct syscall_trace_exit
according to that?
That won't change the code behaviour, just the logic while
reviewing.

It's just a neat, this patch itself is fine.


>
> Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> ---
>  kernel/trace/trace_syscalls.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 9fbce6c..527e17e 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -166,7 +166,7 @@ int syscall_exit_format(struct ftrace_event_call *call,
> struct trace_seq *s)
>  			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
>  			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
>  			       SYSCALL_FIELD(int, nr),
> -			       SYSCALL_FIELD(unsigned long, ret));
> +			       SYSCALL_FIELD(long, ret));
>  	if (!ret)
>  		return 0;
>
> @@ -212,7 +212,7 @@ int syscall_exit_define_fields(struct ftrace_event_call
> *call)
>  	if (ret)
>  		return ret;
>
> -	ret = trace_define_field(call, SYSCALL_FIELD(unsigned long, ret), 0,
> +	ret = trace_define_field(call, SYSCALL_FIELD(long, ret), 0,
>  				 FILTER_OTHER);
>
>  	return ret;
> --
> 1.6.4.GIT
>
>

Re: [PATCH 0/3] a few tracing bugfixes

[Frédéric Weisbecker]

2009/10/6, Tom Zanussi <tzanussi@gmail.com>:
> Some fixes for a few things I noticed recently.
>
> Tom Zanussi (3):
>   perf trace: Remove unused code in builtin-trace.c
>   perf trace: Update eval_flag() flags array to match interrupt.h
>   tracing/syscalls: Use long for syscall ret format and field
>     definitions
>
>  kernel/trace/trace_syscalls.c       |    4 ++--
>  tools/perf/builtin-trace.c          |    6 ------
>  tools/perf/util/trace-event-parse.c |    9 +++++----
>  3 files changed, 7 insertions(+), 12 deletions(-)

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Thanks a lot!

[tip:perf/urgent] perf trace: Remove unused code in builtin-trace.c

[tip-bot for Tom Zanussi]

Commit-ID:  d4c3768faaae70cdcffc3a5e296bd99edfa7ddb2
Gitweb:     http://git.kernel.org/tip/d4c3768faaae70cdcffc3a5e296bd99edfa7ddb2
Author:     Tom Zanussi <tzanussi@gmail.com>
AuthorDate: Tue, 6 Oct 2009 01:00:47 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 6 Oct 2009 12:02:33 +0200

perf trace: Remove unused code in builtin-trace.c

And some minor whitespace cleanup.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: rostedt@goodmis.org
Cc: lizf@cn.fujitsu.com
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
LKML-Reference: <1254808849-7829-2-git-send-email-tzanussi@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/builtin-trace.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index e9d256e..0c5e4f7 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -219,10 +219,6 @@ remap:
 more:
 	event = (event_t *)(buf + head);
 
-	size = event->header.size;
-	if (!size)
-		size = 8;
-
 	if (head + event->header.size >= page_size * mmap_window) {
 		unsigned long shift = page_size * (head / page_size);
 		int res;
@@ -237,7 +233,6 @@ more:
 
 	size = event->header.size;
 
-
 	if (!size || process_event(event, offset, head) < 0) {
 
 		/*
@@ -290,7 +285,6 @@ int cmd_trace(int argc, const char **argv, const char *prefix __used)
 			usage_with_options(annotate_usage, options);
 	}
 
-
 	setup_pager();
 
 	return __cmd_trace();

[tip:perf/urgent] perf trace: Update eval_flag() flags array to match interrupt.h

[tip-bot for Tom Zanussi]

Commit-ID:  b934cdd55f2ac38c825f3d46cfa87a1654f1c849
Gitweb:     http://git.kernel.org/tip/b934cdd55f2ac38c825f3d46cfa87a1654f1c849
Author:     Tom Zanussi <tzanussi@gmail.com>
AuthorDate: Tue, 6 Oct 2009 01:00:48 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 6 Oct 2009 12:02:34 +0200

perf trace: Update eval_flag() flags array to match interrupt.h

Add missing BLOCK_IOPOLL_SOFTIRQ entry.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: rostedt@goodmis.org
Cc: lizf@cn.fujitsu.com
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
LKML-Reference: <1254808849-7829-3-git-send-email-tzanussi@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/util/trace-event-parse.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
index f6a8437..55b41b9 100644
--- a/tools/perf/util/trace-event-parse.c
+++ b/tools/perf/util/trace-event-parse.c
@@ -1968,10 +1968,11 @@ static const struct flag flags[] = {
 	{ "NET_TX_SOFTIRQ", 2 },
 	{ "NET_RX_SOFTIRQ", 3 },
 	{ "BLOCK_SOFTIRQ", 4 },
-	{ "TASKLET_SOFTIRQ", 5 },
-	{ "SCHED_SOFTIRQ", 6 },
-	{ "HRTIMER_SOFTIRQ", 7 },
-	{ "RCU_SOFTIRQ", 8 },
+	{ "BLOCK_IOPOLL_SOFTIRQ", 5 },
+	{ "TASKLET_SOFTIRQ", 6 },
+	{ "SCHED_SOFTIRQ", 7 },
+	{ "HRTIMER_SOFTIRQ", 8 },
+	{ "RCU_SOFTIRQ", 9 },
 
 	{ "HRTIMER_NORESTART", 0 },
 	{ "HRTIMER_RESTART", 1 },

[tip:perf/urgent] tracing/syscalls: Use long for syscall ret format and field definitions

[tip-bot for Tom Zanussi]

Commit-ID:  ee949a86b3aef15845ea677aa60231008de62672
Gitweb:     http://git.kernel.org/tip/ee949a86b3aef15845ea677aa60231008de62672
Author:     Tom Zanussi <tzanussi@gmail.com>
AuthorDate: Tue, 6 Oct 2009 01:00:49 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 6 Oct 2009 12:02:34 +0200

tracing/syscalls: Use long for syscall ret format and field definitions

The syscall event definitions use long for the syscall exit ret
value, but unsigned long for the same thing in the format and field
definitions.  Change them all to long.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: rostedt@goodmis.org
Cc: lizf@cn.fujitsu.com
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
LKML-Reference: <1254808849-7829-4-git-send-email-tzanussi@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/trace/trace_syscalls.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 9fbce6c..527e17e 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -166,7 +166,7 @@ int syscall_exit_format(struct ftrace_event_call *call, struct trace_seq *s)
 			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
 			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
 			       SYSCALL_FIELD(int, nr),
-			       SYSCALL_FIELD(unsigned long, ret));
+			       SYSCALL_FIELD(long, ret));
 	if (!ret)
 		return 0;
 
@@ -212,7 +212,7 @@ int syscall_exit_define_fields(struct ftrace_event_call *call)
 	if (ret)
 		return ret;
 
-	ret = trace_define_field(call, SYSCALL_FIELD(unsigned long, ret), 0,
+	ret = trace_define_field(call, SYSCALL_FIELD(long, ret), 0,
 				 FILTER_OTHER);
 
 	return ret;

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Christoph Hellwig]

On Tue, Oct 06, 2009 at 01:00:48AM -0500, Tom Zanussi wrote:
> Add missing BLOCK_IOPOLL_SOFTIRQ entry.

Nothing against your patch which is good for the short term, but the
existance of this code is a major design flaw.  The trace events are
supposed to be self describing and having them duplicated in the trace
tool means either we're not using that self-description or it's not
good enough to be used.

We'll soon add a lot more flags (e.g. my xfs tracing patch if I can ever
get the formal maintainer to review it will add various), and it was
also made pretty clear that perf while packaged with the kernel should
work independent of the actual kernel version, e.g. one perf binary
should work with older host kernels too which might have very different
flag values.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Frederic Weisbecker]

On Tue, Oct 06, 2009 at 09:27:32AM -0400, Christoph Hellwig wrote:
> On Tue, Oct 06, 2009 at 01:00:48AM -0500, Tom Zanussi wrote:
> > Add missing BLOCK_IOPOLL_SOFTIRQ entry.
> 
> Nothing against your patch which is good for the short term, but the
> existance of this code is a major design flaw.  The trace events are
> supposed to be self describing and having them duplicated in the trace
> tool means either we're not using that self-description or it's not
> good enough to be used.
> 
> We'll soon add a lot more flags (e.g. my xfs tracing patch if I can ever
> get the formal maintainer to review it will add various), and it was
> also made pretty clear that perf while packaged with the kernel should
> work independent of the actual kernel version, e.g. one perf binary
> should work with older host kernels too which might have very different
> flag values.
> 


Yeah. We may want to do that by including trace/events/irq.h
and then use the show_softirq_name() macro defined there.

The rest of the header can be wrapped through no-op macros and
stub includes.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Christoph Hellwig]

On Tue, Oct 06, 2009 at 05:19:00PM +0200, Frederic Weisbecker wrote:
> Yeah. We may want to do that by including trace/events/irq.h
> and then use the show_softirq_name() macro defined there.
> 
> The rest of the header can be wrapped through no-op macros and
> stub includes.

No, not at all.  Performance tracing tools really should not be
dependent on the kernel source.  This kind of creep is exactly what I
feared from putting the perf source in the kernel tree.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Frederic Weisbecker]

On Tue, Oct 06, 2009 at 11:21:06AM -0400, Christoph Hellwig wrote:
> On Tue, Oct 06, 2009 at 05:19:00PM +0200, Frederic Weisbecker wrote:
> > Yeah. We may want to do that by including trace/events/irq.h
> > and then use the show_softirq_name() macro defined there.
> > 
> > The rest of the header can be wrapped through no-op macros and
> > stub includes.
> 
> No, not at all.  Performance tracing tools really should not be
> dependent on the kernel source.  This kind of creep is exactly what I
> feared from putting the perf source in the kernel tree.
> 

I see...
Then the only solution I can imagine is to export a debugfs file
in the tracing directory that provides this name resolution.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Christoph Hellwig]

On Tue, Oct 06, 2009 at 05:22:58PM +0200, Frederic Weisbecker wrote:
> I see...
> Then the only solution I can imagine is to export a debugfs file
> in the tracing directory that provides this name resolution.

The format files kinda contain that information, just not in a very
useful format.  I think the general problem is that the formats exported
aren't descriptive enough, and fields/flags are just one symptom of
that.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Frederic Weisbecker]

On Tue, Oct 06, 2009 at 05:22:58PM +0200, Frederic Weisbecker wrote:
> On Tue, Oct 06, 2009 at 11:21:06AM -0400, Christoph Hellwig wrote:
> > On Tue, Oct 06, 2009 at 05:19:00PM +0200, Frederic Weisbecker wrote:
> > > Yeah. We may want to do that by including trace/events/irq.h
> > > and then use the show_softirq_name() macro defined there.
> > > 
> > > The rest of the header can be wrapped through no-op macros and
> > > stub includes.
> > 
> > No, not at all.  Performance tracing tools really should not be
> > dependent on the kernel source.  This kind of creep is exactly what I
> > feared from putting the perf source in the kernel tree.
> > 
> 
> I see...
> Then the only solution I can imagine is to export a debugfs file
> in the tracing directory that provides this name resolution.
> 

I mean, not something that we would check from an offline client,
but something we could include in the traces info while recording the
traces.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Frederic Weisbecker]

On Tue, Oct 06, 2009 at 11:24:19AM -0400, Christoph Hellwig wrote:
> On Tue, Oct 06, 2009 at 05:22:58PM +0200, Frederic Weisbecker wrote:
> > I see...
> > Then the only solution I can imagine is to export a debugfs file
> > in the tracing directory that provides this name resolution.
> 
> The format files kinda contain that information, just not in a very
> useful format.  I think the general problem is that the formats exported
> aren't descriptive enough, and fields/flags are just one symptom of
> that.


Well, Steve has posted a whole rewrite of the format files some months ago,
I don't know what's the status of this work. One of the problems is that we
know have tools that support the current format.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Steven Rostedt]

On Tue, 2009-10-06 at 23:42 +0200, Frederic Weisbecker wrote:
> On Tue, Oct 06, 2009 at 11:24:19AM -0400, Christoph Hellwig wrote:
> > On Tue, Oct 06, 2009 at 05:22:58PM +0200, Frederic Weisbecker wrote:
> > > I see...
> > > Then the only solution I can imagine is to export a debugfs file
> > > in the tracing directory that provides this name resolution.
> > 
> > The format files kinda contain that information, just not in a very
> > useful format.  I think the general problem is that the formats exported
> > aren't descriptive enough, and fields/flags are just one symptom of
> > that.
> 
> 
> Well, Steve has posted a whole rewrite of the format files some months ago,
> I don't know what's the status of this work. One of the problems is that we
> know have tools that support the current format.
> 

Yeah, but that new format was just begging for a NAK. No one wants a new
format layout. They want C code. But unfortunately, when we use enums
for flags and such, the macros have no idea how to handle it. I may try
to work out a way to let those same macros be able to export the value
with the enum name.

I'll try to figure out something.

-- Steve

Re: [PATCH 3/3] tracing/syscalls: Use long for syscall ret format and field definitions

[Tom Zanussi]

On Tue, 2009-10-06 at 09:15 +0200, Frédéric Weisbecker wrote:
> 2009/10/6, Tom Zanussi <tzanussi@gmail.com>:
> > The syscall event definitions use long for the syscall exit ret value,
> > but unsigned long for the same thing in the format and field
> > definitions.  Change them all to long.
> 
> 
> Thanks.
> 
> We may perhaps also want to change struct syscall_trace_exit
> according to that?
> That won't change the code behaviour, just the logic while
> reviewing.
> 
> It's just a neat, this patch itself is fine.
> 

Yeah, that should have been part of it too.  I'll submit another patch
later doing that.

Tom

> 
> >
> > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > ---
> >  kernel/trace/trace_syscalls.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> > index 9fbce6c..527e17e 100644
> > --- a/kernel/trace/trace_syscalls.c
> > +++ b/kernel/trace/trace_syscalls.c
> > @@ -166,7 +166,7 @@ int syscall_exit_format(struct ftrace_event_call *call,
> > struct trace_seq *s)
> >  			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> >  			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
> >  			       SYSCALL_FIELD(int, nr),
> > -			       SYSCALL_FIELD(unsigned long, ret));
> > +			       SYSCALL_FIELD(long, ret));
> >  	if (!ret)
> >  		return 0;
> >
> > @@ -212,7 +212,7 @@ int syscall_exit_define_fields(struct ftrace_event_call
> > *call)
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = trace_define_field(call, SYSCALL_FIELD(unsigned long, ret), 0,
> > +	ret = trace_define_field(call, SYSCALL_FIELD(long, ret), 0,
> >  				 FILTER_OTHER);
> >
> >  	return ret;
> > --
> > 1.6.4.GIT
> >
> >

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Oct 06, 2009 at 05:19:00PM +0200, Frederic Weisbecker wrote:
> > Yeah. We may want to do that by including trace/events/irq.h
> > and then use the show_softirq_name() macro defined there.
> > 
> > The rest of the header can be wrapped through no-op macros and
> > stub includes.
> 
> No, not at all.  Performance tracing tools really should not be 
> dependent on the kernel source.  This kind of creep is exactly what I 
> feared from putting the perf source in the kernel tree.

And you were full of it back then and you are full of it now as well.

Of course tools/perf/ can be dependent on the kernel source, as long as 
it's all exposed cleanly. Runtime exposure of information is better of 
course in many cases, but there's a balance to be stricken.

We already have deep and good dependencies between kernel code and 
tools/perf: for example we use the kernel's list.h and lib/rbtree.c in 
perf and those facilities are God-sent over user-space crap that for 
example Glist is.

I tend to agree that softirq names might make sense to expose runtime as 
well, but that is totally independent of your _idiotic_ argument that 
this issue somehow talks against perf being part of the kernel source.

Really, give up that argument already - or if not, please engage in an 
open, honest exchange about it. These drip-drip attacks you are doing, 
without actually having the balls to argue your technical position are 
somewhat annoying.

	Ingo

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Christoph Hellwig]

On Sun, Oct 11, 2009 at 10:53:45AM +0200, Ingo Molnar wrote:
> And you were full of it back then and you are full of it now as well.

Beeing nice today, eh? :)

> Of course tools/perf/ can be dependent on the kernel source, as long as 
> it's all exposed cleanly. Runtime exposure of information is better of 
> course in many cases, but there's a balance to be stricken.
> 
> We already have deep and good dependencies between kernel code and 
> tools/perf: for example we use the kernel's list.h and lib/rbtree.c in 
> perf and those facilities are God-sent over user-space crap that for 
> example Glist is.

Re-using code is no problem at all.  If you look at typical lowlevel
userspace code written by kernel developers you'll notice that they
usually use the kernel data structures, too.  And yeah, glib is
quite horrible.

The problem is run-time depdency on the kernel it was built with.  It's
not practival or desirable to have one perf binary per kernel version.

> I tend to agree that softirq names might make sense to expose runtime as 
> well, but that is totally independent of your _idiotic_ argument that 
> this issue somehow talks against perf being part of the kernel source.

It is directly related.  If you ship perf as part of the kernel source
these kinds of thing slip in easily, just because people don't think
about it enough.  If you have a separate source tree it's much more
clear that you can't depend on internal implementation details.

Re: [PATCH 2/3] perf trace: Update eval_flag() flags array to match interrupt.h

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Oct 11, 2009 at 10:53:45AM +0200, Ingo Molnar wrote:
> > And you were full of it back then and you are full of it now as well.
> 
> Beeing nice today, eh? :)

Yeah, being reciprocal to you ;-)

> > Of course tools/perf/ can be dependent on the kernel source, as long 
> > as it's all exposed cleanly. Runtime exposure of information is 
> > better of course in many cases, but there's a balance to be 
> > stricken.
> > 
> > We already have deep and good dependencies between kernel code and 
> > tools/perf: for example we use the kernel's list.h and lib/rbtree.c 
> > in perf and those facilities are God-sent over user-space crap that 
> > for example Glist is.
> 
> Re-using code is no problem at all.  If you look at typical lowlevel 
> userspace code written by kernel developers you'll notice that they 
> usually use the kernel data structures, too.  And yeah, glib is quite 
> horrible.
> 
> The problem is run-time depdency on the kernel it was built with.  
> It's not practival or desirable to have one perf binary per kernel 
> version.

But we release a new version of perf per kernel release. So we have a 
perf binary per kernel release _already_.

Yes, it's good to avoid coupling but it's not nearly as much of a 
problem as you make it out to be. You seem to argue in favor of some 
sort of inflexible, rigid cage for instrumentation apps and that's 
simply the wrong mindset. That kind of rigidity and isolation kept Linux 
instrumentation poor for a decade to begin with.

> > I tend to agree that softirq names might make sense to expose 
> > runtime as well, but that is totally independent of your _idiotic_ 
> > argument that this issue somehow talks against perf being part of 
> > the kernel source.
> 
> It is directly related.  If you ship perf as part of the kernel source 
> these kinds of thing slip in easily, just because people don't think 
> about it enough.  If you have a separate source tree it's much more 
> clear that you can't depend on internal implementation details.

Exactly where is the problem? You say things like 'bad' and 'mess', 
without actually articulating a single practical disadvantage.

Sure if you go back to an old kernel with new instrumentation binary you 
might get the wrong softirq names. It doesnt really matter in practice: 
'perf' can be built in any past kernel and can be used there.

Fact is, using information from the kernel source has its clear 
advantages. There's lots of details we dont want to guarantee in the ABI 
sense yet. Amongst them are the softirq names. They get changed all the 
time and softirqs might even go away entirely. It's good for tools to 
have _some_ notion about them - but to guarantee it until eternity? No 
way is that an unconditionally good thing.

	Ingo