* [PATCH 1/2] perf tools: Compare dso's also when comparing symbols
@ 2013-10-15 2:01 Namhyung Kim
2013-10-15 2:01 ` [RFC/PATCH 2/2] perf tools: Sort dso using pointers Namhyung Kim
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Namhyung Kim @ 2013-10-15 2:01 UTC (permalink / raw
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
Jiri Olsa, David Ahern
From: Namhyung Kim <namhyung.kim@lge.com>
Linus reported that sometimes 'perf report -s symbol' exits without
any message on TUI. David and Jiri found that it's because it failed
to add a hist entry due to an invalid symbol length.
It turns out that sorting by symbol (address) was broken since it only
compares symbol addresses. The symbol address is a relative address
within a dso thus just checking its address can result in merging
unrelated symbols together. Fix it by checking dso before comparing
symbol address.
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/sort.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 32c56377e008..1f9821db9e77 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -182,9 +182,19 @@ static int64_t _sort__sym_cmp(struct symbol *sym_l, struct symbol *sym_r)
static int64_t
sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
{
+ int64_t ret;
+
if (!left->ms.sym && !right->ms.sym)
return right->level - left->level;
+ /*
+ * comparing symbol address alone is not enough since it's a
+ * relative address within a dso.
+ */
+ ret = sort__dso_cmp(left, right);
+ if (ret != 0)
+ return ret;
+
return _sort__sym_cmp(left->ms.sym, right->ms.sym);
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC/PATCH 2/2] perf tools: Sort dso using pointers
2013-10-15 2:01 [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Namhyung Kim
@ 2013-10-15 2:01 ` Namhyung Kim
2013-10-15 6:08 ` [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Ingo Molnar
2013-10-23 7:54 ` [tip:perf/core] perf tools: Compare dso' s " tip-bot for Namhyung Kim
2 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2013-10-15 2:01 UTC (permalink / raw
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
Jiri Olsa, David Ahern
From: Namhyung Kim <namhyung.kim@lge.com>
The dso's in a perf session are maintained machine-wide so same dso is
shared by hist entries. Thus just checking pointer should be enough
to comparing dso's.
It may change behavior of 'perf report -s dso' when there're two or
more dso's that have same basename - the dso's which have same name
used to merged to one unless -v option was given. But I don't think
it's a big problem. ;)
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/sort.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1f9821db9e77..70658a0834e3 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -114,20 +114,8 @@ static int64_t _sort__dso_cmp(struct map *map_l, struct map *map_r)
{
struct dso *dso_l = map_l ? map_l->dso : NULL;
struct dso *dso_r = map_r ? map_r->dso : NULL;
- const char *dso_name_l, *dso_name_r;
- if (!dso_l || !dso_r)
- return cmp_null(dso_l, dso_r);
-
- if (verbose) {
- dso_name_l = dso_l->long_name;
- dso_name_r = dso_r->long_name;
- } else {
- dso_name_l = dso_l->short_name;
- dso_name_r = dso_r->short_name;
- }
-
- return strcmp(dso_name_l, dso_name_r);
+ return (int64_t)(dso_l - dso_r);
}
static int64_t
--
1.7.11.7
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] perf tools: Compare dso's also when comparing symbols
2013-10-15 2:01 [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Namhyung Kim
2013-10-15 2:01 ` [RFC/PATCH 2/2] perf tools: Sort dso using pointers Namhyung Kim
@ 2013-10-15 6:08 ` Ingo Molnar
2013-10-15 7:12 ` Namhyung Kim
2013-10-23 7:54 ` [tip:perf/core] perf tools: Compare dso' s " tip-bot for Namhyung Kim
2 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2013-10-15 6:08 UTC (permalink / raw
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
Namhyung Kim, LKML, Jiri Olsa, David Ahern
* Namhyung Kim <namhyung@kernel.org> wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
>
> Linus reported that sometimes 'perf report -s symbol' exits without any
> message on TUI. David and Jiri found that it's because it failed to add
> a hist entry due to an invalid symbol length.
Btw., 'exit without any messages' is something that should be fixed
separately as well I suspect.
> static int64_t
> sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
> {
> + int64_t ret;
> +
Btw., this file should go back to u64/s64 like the kernel and most of perf
does. (A few int64_t uses slipped into other places as well, I suspect
they should be fixed.)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] perf tools: Compare dso's also when comparing symbols
2013-10-15 6:08 ` [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Ingo Molnar
@ 2013-10-15 7:12 ` Namhyung Kim
0 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2013-10-15 7:12 UTC (permalink / raw
To: Ingo Molnar
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
Namhyung Kim, LKML, Jiri Olsa, David Ahern
On Tue, 15 Oct 2013 08:08:29 +0200, Ingo Molnar wrote:
> * Namhyung Kim <namhyung@kernel.org> wrote:
>
>> From: Namhyung Kim <namhyung.kim@lge.com>
>>
>> Linus reported that sometimes 'perf report -s symbol' exits without any
>> message on TUI. David and Jiri found that it's because it failed to add
>> a hist entry due to an invalid symbol length.
>
> Btw., 'exit without any messages' is something that should be fixed
> separately as well I suspect.
Sure. I believe acme is gonna work on it. :)
>
>> static int64_t
>> sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
>> {
>> + int64_t ret;
>> +
>
> Btw., this file should go back to u64/s64 like the kernel and most of perf
> does. (A few int64_t uses slipped into other places as well, I suspect
> they should be fixed.)
Okay, will send a separate patch for the fix.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:perf/core] perf tools: Compare dso' s also when comparing symbols
2013-10-15 2:01 [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Namhyung Kim
2013-10-15 2:01 ` [RFC/PATCH 2/2] perf tools: Sort dso using pointers Namhyung Kim
2013-10-15 6:08 ` [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Ingo Molnar
@ 2013-10-23 7:54 ` tip-bot for Namhyung Kim
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-10-23 7:54 UTC (permalink / raw
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, torvalds,
namhyung.kim, namhyung, jolsa, dsahern, tglx
Commit-ID: 09600e0f9ebb06235b852a646a3644b7d4a71aca
Gitweb: http://git.kernel.org/tip/09600e0f9ebb06235b852a646a3644b7d4a71aca
Author: Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Tue, 15 Oct 2013 11:01:56 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 21 Oct 2013 17:33:23 -0300
perf tools: Compare dso's also when comparing symbols
Linus reported that sometimes 'perf report -s symbol' exits without any
message on TUI. David and Jiri found that it's because it failed to add
a hist entry due to an invalid symbol length.
It turns out that sorting by symbol (address) was broken since it only
compares symbol addresses. The symbol address is a relative address
within a dso thus just checking its address can result in merging
unrelated symbols together. Fix it by checking dso before comparing
symbol address.
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1381802517-18812-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/sort.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 32c5637..1f9821d 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -182,9 +182,19 @@ static int64_t _sort__sym_cmp(struct symbol *sym_l, struct symbol *sym_r)
static int64_t
sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
{
+ int64_t ret;
+
if (!left->ms.sym && !right->ms.sym)
return right->level - left->level;
+ /*
+ * comparing symbol address alone is not enough since it's a
+ * relative address within a dso.
+ */
+ ret = sort__dso_cmp(left, right);
+ if (ret != 0)
+ return ret;
+
return _sort__sym_cmp(left->ms.sym, right->ms.sym);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-23 7:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-15 2:01 [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Namhyung Kim
2013-10-15 2:01 ` [RFC/PATCH 2/2] perf tools: Sort dso using pointers Namhyung Kim
2013-10-15 6:08 ` [PATCH 1/2] perf tools: Compare dso's also when comparing symbols Ingo Molnar
2013-10-15 7:12 ` Namhyung Kim
2013-10-23 7:54 ` [tip:perf/core] perf tools: Compare dso' s " tip-bot for Namhyung Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.