2009-10-06 06:01:33

by Tom Zanussi

[permalink] [raw]
Subject: [PATCH 0/3] a few tracing bugfixes

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


2009-10-06 06:01:36

by Tom Zanussi

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

And some minor whitespace cleanup.

Signed-off-by: Tom Zanussi <[email protected]>
---
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

2009-10-06 06:01:52

by Tom Zanussi

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

Add missing BLOCK_IOPOLL_SOFTIRQ entry.

Signed-off-by: Tom Zanussi <[email protected]>
---
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

2009-10-06 06:01:37

by Tom Zanussi

[permalink] [raw]
Subject: [PATCH 3/3] 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 <[email protected]>
---
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

2009-10-06 07:15:57

by Frederic Weisbecker

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

2009/10/6, Tom Zanussi <[email protected]>:
> 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 <[email protected]>
> ---
> 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
>
>

2009-10-06 07:16:36

by Frederic Weisbecker

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

2009/10/6, Tom Zanussi <[email protected]>:
> 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 <[email protected]>

Thanks a lot!

2009-10-06 12:18:00

by Tom Zanussi

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

Commit-ID: d4c3768faaae70cdcffc3a5e296bd99edfa7ddb2
Gitweb: http://git.kernel.org/tip/d4c3768faaae70cdcffc3a5e296bd99edfa7ddb2
Author: Tom Zanussi <[email protected]>
AuthorDate: Tue, 6 Oct 2009 01:00:47 -0500
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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();

2009-10-06 12:18:17

by Tom Zanussi

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

Commit-ID: b934cdd55f2ac38c825f3d46cfa87a1654f1c849
Gitweb: http://git.kernel.org/tip/b934cdd55f2ac38c825f3d46cfa87a1654f1c849
Author: Tom Zanussi <[email protected]>
AuthorDate: Tue, 6 Oct 2009 01:00:48 -0500
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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 },

2009-10-06 12:18:22

by Tom Zanussi

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

Commit-ID: ee949a86b3aef15845ea677aa60231008de62672
Gitweb: http://git.kernel.org/tip/ee949a86b3aef15845ea677aa60231008de62672
Author: Tom Zanussi <[email protected]>
AuthorDate: Tue, 6 Oct 2009 01:00:49 -0500
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;

2009-10-06 13:28:10

by Christoph Hellwig

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

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.

2009-10-06 15:19:40

by Frederic Weisbecker

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

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.

2009-10-06 15:21:45

by Christoph Hellwig

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

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.

2009-10-06 15:23:37

by Frederic Weisbecker

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

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.

2009-10-06 15:24:57

by Christoph Hellwig

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

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.

2009-10-06 15:27:50

by Frederic Weisbecker

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

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.

2009-10-06 21:43:10

by Frederic Weisbecker

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

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.

2009-10-07 01:19:27

by Steven Rostedt

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

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

2009-10-07 04:00:02

by Tom Zanussi

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

On Tue, 2009-10-06 at 09:15 +0200, Frédéric Weisbecker wrote:
> 2009/10/6, Tom Zanussi <[email protected]>:
> > 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 <[email protected]>
> > ---
> > 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
> >
> >

2009-10-11 08:54:34

by Ingo Molnar

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


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

2009-10-11 12:22:13

by Christoph Hellwig

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

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.

2009-10-16 08:48:36

by Ingo Molnar

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


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