| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07701693397@SHSMSX103.ccr.corp.intel.com>
Date: Tue, 6 Jan 2015 16:39:20 +0000
From: "Liang, Kan" <kan.liang@...el.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: "jolsa@...hat.com" <jolsa@...hat.com>,
"namhyung@...nel.org" <namhyung@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"ak@...ux.intel.com" <ak@...ux.intel.com>
Subject: RE: [PATCH V6 1/1] perf tool: perf diff support for different
binaries
> Em Tue, Dec 02, 2014 at 10:39:18AM -0500, kan.liang@...el.com escreveu:
> > From: Kan Liang <kan.liang@...el.com>
> >
> > Currently, the perf diff only works with same binaries. That's because
> > it compares the symbol start address. It doesn't work if the perf.data
> > comes from different binaries. This patch matches the function names.
>
> Sorry Kan, I saw these patches, its just that it the above statement looked
> completely strange to me, i.e. of course it should look at the dso name,
> then at the symbol name, comparing via addresses makes no sense to me,
> so I kept leaving this patch to look after processing other patches and doing
> other things, then the year end holidays, etc.
>
> So now I'm looking at old code, from 2009:
>
> commit 604c5c92972dcb4b98be34775452d09a5d4ec248
> Author: Arnaldo Carvalho de Melo <acme@...hat.com>
> Date: Wed Dec 16 14:09:53 2009 -0200
>
> perf diff: Change the default sort order to "dso,symbol"
>
> This is a more intuitive / more meaningful default:
>
> $ perf diff | head -8
> 9.02% +1.00% libc-2.10.1.so [.] _IO_vfprintf_internal
> 2.91% -1.00% [kernel] [k] __kmalloc
> 2.85% -1.00% [kernel] [k] ext4_htree_store_dirent
> 1.99% -1.00% [kernel] [k] _atomic_dec_and_lock
> 2.44% [kernel]
> $
>
> Suggested-by: Ingo Molnar <mingo@...e.hu>
>
> and I see:
>
> static void perf_session__match_hists(struct perf_session *old_session,
> struct perf_session *new_session) {
> struct rb_node *nd;
>
> perf_session__resort_by_name(old_session);
>
> for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
> struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
> pos->pair = perf_session__find_hist_entry_by_name(old_session,
> pos);
> }
> }
>
> Ok, it will process all samples, store everything in hist_entries, etc, then
> traverse all entries in the most recent perf.data file and look for a pair, _by
> name_:
>
> static struct hist_entry *
> perf_session__find_hist_entry_by_name(struct perf_session *self,
> struct hist_entry *he) {
> struct rb_node *n = self->hists.rb_node;
>
> while (n) {
> struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
> int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
>
> if (cmp > 0)
> n = n->rb_left;
> else if (cmp < 0)
> n = n->rb_right;
> else {
> cmp = strcmp(he->sym->name, iter->sym->name);
> if (cmp > 0)
> n = n->rb_left;
> else if (cmp < 0)
> n = n->rb_right;
> else
> return iter;
> }
> }
>
> return NULL;
> }
>
> See? Resort the old session by "dso,symbol_name", then go on pairing
> them.
>
> So at some point this got broken :-\
>
> I.e. this is not changing an existing correct behaviour, but fixing a bug, i.e.
> the changeset comment confused me :-\
>
> Now I'm trying to figure out _when_ this got broken, what was the
> reasoning for that code to have changed in a way that made it not match
> by dso_name, symbol_name.
>
It looks it got broken just after two weeks later in 2009.
perf_session__find_hist_entry_by_name was replaced by hist_entry__cmp.
After that it doesn't compares the name any more.
commit 9c443dfdd31eddea6cbe6ee0ca469fbcc4e1dc3b
Author: Arnaldo Carvalho de Melo <acme@...hat.com>
Date: Mon Dec 28 22:48:36 2009 -0200
perf diff: Fix support for all --sort combinations
@@ -122,29 +112,28 @@ static void perf_session__resort_by_name(struct perf_session *self)
self->hists = tmp;
}
+static void perf_session__set_hist_entries_positions(struct perf_session *self)
+{
+ perf_session__output_resort(self, self->events_stats.total);
+ perf_session__resort_hist_entries(self);
+}
+
static struct hist_entry *
-perf_session__find_hist_entry_by_name(struct perf_session *self,
- struct hist_entry *he)
+perf_session__find_hist_entry(struct perf_session *self,
+ struct hist_entry *he)
{
struct rb_node *n = self->hists.rb_node;
while (n) {
struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
- int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
+ int64_t cmp = hist_entry__cmp(he, iter);
- if (cmp > 0)
+ if (cmp < 0)
n = n->rb_left;
- else if (cmp < 0)
+ else if (cmp > 0)
n = n->rb_right;
- else {
- cmp = strcmp(he->sym->name, iter->sym->name);
- if (cmp > 0)
- n = n->rb_left;
- else if (cmp < 0)
- n = n->rb_right;
- else
- return iter;
- }
+ else
+ return iter;
}
return NULL;
@@ -155,11 +144,9 @@ static void perf_session__match_hists(struct perf_session *old_session,
{
struct rb_node *nd;
- perf_session__resort_by_name(old_session);
-
for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
- pos->pair = perf_session__find_hist_entry_by_name(old_session, pos);
+ pos->pair = perf_session__find_hist_entry(old_session, pos);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists