2010-01-22 16:35:27

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: [PATCH 1/2] perf symbols: Fix inverted logic for showing kallsyms as the source of symbols

From: Arnaldo Carvalho de Melo <[email protected]>

Only if we parsed /proc/kallsyms (or a copy found in the buildid cache)
we should set the dso long name to "[kernel.kallsyms]".

Reported-by: Mike Galbraith <[email protected]>
Cc: Frédéric Weisbecker <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/symbol.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 6f30fe1..1270cf8 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1671,7 +1671,7 @@ do_kallsyms:
out_try_fixup:
if (err > 0) {
out_fixup:
- if (kallsyms_filename == NULL)
+ if (kallsyms_filename != NULL)
dso__set_long_name(self, strdup("[kernel.kallsyms]"));
map__fixup_start(map);
map__fixup_end(map);
--
1.6.2.5


2010-01-22 16:35:20

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: [PATCH 2/2] perf symbols: Use the right variable to check for kallsyms in the cache

From: Arnaldo Carvalho de Melo <[email protected]>

Probably this wasn't noticed when testing this on my parisc machine
because I must have copied manually to its cache the vmlinux file used
in the x86_64 machine, now that I tried looking on a x86-32 machine with
a fresh cache, kernel symbols weren't being resolved even with the right
kallsyms copy on its cache, duh.

Cc: Frédéric Weisbecker <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/symbol.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 1270cf8..f1f609d 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1650,12 +1650,12 @@ static int dso__load_kernel_sym(struct dso *self, struct map *map,
getenv("HOME"), sbuild_id) == -1)
return -1;

+ kallsyms_filename = kallsyms_allocated_filename;
+
if (access(kallsyms_filename, F_OK)) {
free(kallsyms_allocated_filename);
return -1;
}
-
- kallsyms_filename = kallsyms_allocated_filename;
} else {
/*
* Last resort, if we don't have a build-id and couldn't find
--
1.6.2.5

2010-01-22 16:43:33

by Mike Galbraith

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf symbols: Fix inverted logic for showing kallsyms as the source of symbols

On Fri, 2010-01-22 at 14:35 -0200, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <[email protected]>
>
> Only if we parsed /proc/kallsyms (or a copy found in the buildid cache)
> we should set the dso long name to "[kernel.kallsyms]".

Yup, works just as well as my busted one :)

-Mike

2010-01-22 16:51:12

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: perf regression testing. was Re: [PATCH 1/2] perf symbols: Fix inverted logic for showing kallsyms as the source of symbols

Em Fri, Jan 22, 2010 at 05:43:28PM +0100, Mike Galbraith escreveu:
> On Fri, 2010-01-22 at 14:35 -0200, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <[email protected]>
> >
> > Only if we parsed /proc/kallsyms (or a copy found in the buildid cache)
> > we should set the dso long name to "[kernel.kallsyms]".
>
> Yup, works just as well as my busted one :)

yeah :-)

I'll soon start working on 'perf regtest', to start accumulating
regression tests a la 'perf bench', as these silly bugs are worrying
me...

Some will require more than one machine, and of different archs, so that
we can properly test 'perf archive' and the buildid machinery, etc. Some
need to compare what we get from kallsyms with what we get from vmlinux
of same buildid, etc.

We'll use this when developing new tools or changing the core code, and
a subset of it can also be used by users to sanity test their
environment.

If not I'll have to continue relying on you as my regression test ;-)

- Arnaldo

2010-01-23 07:05:50

by Ingo Molnar

[permalink] [raw]
Subject: Re: perf regression testing. was Re: [PATCH 1/2] perf symbols: Fix inverted logic for showing kallsyms as the source of symbols


* Arnaldo Carvalho de Melo <[email protected]> wrote:

> Em Fri, Jan 22, 2010 at 05:43:28PM +0100, Mike Galbraith escreveu:
> > On Fri, 2010-01-22 at 14:35 -0200, Arnaldo Carvalho de Melo wrote:
> > > From: Arnaldo Carvalho de Melo <[email protected]>
> > >
> > > Only if we parsed /proc/kallsyms (or a copy found in the buildid cache)
> > > we should set the dso long name to "[kernel.kallsyms]".
> >
> > Yup, works just as well as my busted one :)
>
> yeah :-)
>
> I'll soon start working on 'perf regtest', to start accumulating regression
> tests a la 'perf bench', as these silly bugs are worrying me...

Excellent, it's much needed!

The best example i know about is KVM: they have built an impressive regression
test suite and add testcases for new regressions all the time.

The key is to have something simple to run, which developers (and
maintainers) can run after every new perf commit or so.

Ingo

2010-01-23 15:53:16

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: perf regression testing. was Re: [PATCH 1/2] perf symbols: Fix inverted logic for showing kallsyms as the source of symbols

Em Sat, Jan 23, 2010 at 08:04:51AM +0100, Ingo Molnar escreveu:
>
> * Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> > Em Fri, Jan 22, 2010 at 05:43:28PM +0100, Mike Galbraith escreveu:
> > > On Fri, 2010-01-22 at 14:35 -0200, Arnaldo Carvalho de Melo wrote:
> > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > >
> > > > Only if we parsed /proc/kallsyms (or a copy found in the buildid cache)
> > > > we should set the dso long name to "[kernel.kallsyms]".
> > >
> > > Yup, works just as well as my busted one :)
> >
> > yeah :-)
> >
> > I'll soon start working on 'perf regtest', to start accumulating regression
> > tests a la 'perf bench', as these silly bugs are worrying me...
>
> Excellent, it's much needed!
>
> The best example i know about is KVM: they have built an impressive regression
> test suite and add testcases for new regressions all the time.
>
> The key is to have something simple to run, which developers (and
> maintainers) can run after every new perf commit or so.

Yes, that is the idea, I'm finishing the first test (/proc/kallsyms
versus what we get from a auto-located vmlinux symtab) and will submit
it soon:

[acme@doppio linux-2.6-tip]$ perf regtest
1: vmlinux symtab matches kallsyms...
Segmentation fault
[acme@doppio linux-2.6-tip]$

Not quite working yet, tho :o)

- Arnaldo