LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Liang, Kan" <kan.liang@intel.com>
To: "'Arnaldo Carvalho de Melo'" <acme@kernel.org>
Cc: "jolsa@redhat.com" <jolsa@redhat.com>,
	"namhyung@kernel.org" <namhyung@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH V6 1/1] perf tool: perf diff support for different binaries
Date: Mon, 26 Jan 2015 14:15:01 +0000	[thread overview]
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077016A5F6F@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20150106145356.GA10675@kernel.org>



> 
> Em Tue, Dec 02, 2014 at 10:39:18AM -0500, kan.liang@intel.com escreveu:
> > From: Kan Liang <kan.liang@intel.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@redhat.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@elte.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 :-\
> 


Hi Arnaldo,

I tried the 2009 version perf diff. The code still only work for the same binaries.

Although it compared by name, it used a global list of dsos. 
That means the binaries which has same name can only be loaded once. It's OK
for same binaries. But for different binaries, it's a problem. For example, we have
an old binary and an updated binary. They very likely have same name and most
of the function. So only dsos from old binary will be loaded. When processing the
data from updated binary, perf still use the symbol information from old binary. 
That's wrong for different binaries.

Then about two weeks later, the patch as below to use IP to replace symbol name.

author	Arnaldo Carvalho de Melo <acme@redhat.com>	2009-12-29 00:48:36 (GMT)
committer	Ingo Molnar <mingo@elte.hu>	2009-12-30 11:00:00 (GMT)
commit	9c443dfdd31eddea6cbe6ee0ca469fbcc4e1dc3b (patch)
tree	6e217aaf89b941fbbc7b905046d5350392d642db /tools/perf
parent	cdbae31408cf39372402076cf2e189ec693daa71 (diff)
perf diff: Fix support for all --sort combinations


The global dsos is discarded from a patch in 2010. Since the previous patch has already
use the IP to compare. So it still doesn't work for different binaries

author	Zhang, Yanmin <yanmin_zhang@linux.intel.com>	2010-04-19 05:32:50 (GMT)
committer	Avi Kivity <avi@redhat.com>	2010-04-19 09:37:24 (GMT)
commit	a1645ce12adb6c9cc9e19d7695466204e3f017fe (patch)
tree	5d31aaaf534997e6e9cebc07f38eca35f76986cf /tools/perf
parent	ff9d07a0e7ce756a183e7c2e483aec452ee6b574 (diff)
perf: 'perf kvm' tool for monitoring guest performance from host


So I think perf diff never works for different binaries.  My patch should not be a bug fix,
but a feature enhancement as I mentioned in the description. 

What do you think?

Thanks,
Kan


> 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.
> 
> Also please, when providing code to be built to reproduce your results,
> make it self contained,  i.e. in the case below, I had to replace "noinline"
> with "__attribute__((noinline))".
> 
> - Arnaldo
> 


      parent reply	other threads:[~2015-01-26 14:15 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-02 15:39 kan.liang
2015-01-05 16:04 ` Liang, Kan
2015-01-06 14:53 ` Arnaldo Carvalho de Melo
2015-01-06 16:16   ` Arnaldo Carvalho de Melo
2015-01-12 18:06     ` Liang, Kan
2015-01-06 16:39   ` Liang, Kan
2015-01-26 14:15   ` Liang, Kan [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=37D7C6CF3E00A74B8858931C1DB2F077016A5F6F@SHSMSX103.ccr.corp.intel.com \
    --to=kan.liang@intel.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namhyung@kernel.org \
    --subject='RE: [PATCH V6 1/1] perf tool: perf diff support for different binaries' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).