| 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: <20150225153006.GD18705@kernel.org>
Date: Wed, 25 Feb 2015 12:30:06 -0300
From: "acme@...nel.org" <acme@...nel.org>
To: "Liang, Kan" <kan.liang@...el.com>
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 V7 1/1] perf tool:perf diff support for different binaries
Em Wed, Feb 25, 2015 at 03:14:06PM +0000, Liang, Kan escreveu:
> Hi Arnaldo,
>
> Could you please review the patch?
> I've already updated the patch description to try to address your concern.
> Please let me know if you have any questions.
Just out of time, sorry, will get to it eventually.
- Arnaldo
> Thanks,
> Kan
>
> >
> > 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 symbol names.
> >
> > Actually, perf diff once intended to compare the symbol names.
> > The commit as below can look for a pair by name.
> > 604c5c92972d (perf diff: Change the default sort order to "dso,symbol")
> > However, at that time, perf diff used a global list of dsos. That means the
> > binaries which has same name can only be loaded once. That's a problem
> > for different binaries compare. 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.
> >
> > Then the commit as below used IP to replace symbol name.
> > 9c443dfdd31e ("perf diff: Fix support for all --sort combinations") From that
> > time, perf diff starts to compare the symbol address.
> >
> > The global dsos is discarded from a patch in 2010.
> > a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance
> > from host") However, at that time, perf diff already compared by address.
> > So perf diff cannot work for different binaries as well.
> >
> > This patch actually rolls back the perf diff to original design. The document
> > is also changed, so everybody knows the original design is to compare the
> > symbol names.
> >
> > Here is an examples.
> > The only difference between example_v1.c and example_v2.c is the
> > location of f2 and f3. There is no change in behavior, but the previous perf
> > diff display the wrong differential profile.
> >
> > example_v1.c
> > noinline void f3(void)
> > {
> > volatile int i;
> > for (i = 0; i < 10000;) {
> >
> > if(i%2)
> > i++;
> > else
> > i++;
> > }
> > }
> >
> > noinline void f2(void)
> > {
> > volatile int a = 100, b, c;
> > for (b = 0; b < 10000; b++)
> > c = a * b;
> >
> > }
> >
> > noinline void f1(void)
> > {
> > f2();
> > f3();
> > }
> >
> > int main()
> > {
> > int i;
> > for (i = 0; i < 100000; i++)
> > f1();
> > }
> >
> > example_v2.c
> > noinline void f2(void)
> > {
> > volatile int a = 100, b, c;
> > for (b = 0; b < 10000; b++)
> > c = a * b;
> > }
> >
> > noinline void f3(void)
> > {
> > volatile int i;
> > for (i = 0; i < 10000;) {
> > if(i%2)
> > i++;
> > else
> > i++;
> > }
> > }
> >
> > noinline void f1(void)
> > {
> > f2();
> > f3();
> > }
> >
> > int main()
> > {
> > int i;
> > for (i = 0; i < 100000; i++)
> > f1();
> > }
> >
> > [lk@...alhost perf_diff]$ gcc example_v1.c -o example [lk@...alhost
> > perf_diff]$ perf record -o example_v1.data ./example [ perf record:
> > Woken up 4 times to write data ] [ perf record: Captured and wrote 0.813
> > MB example_v1.data (~35522
> > samples) ]
> >
> > [lk@...alhost perf_diff]$ gcc example_v2.c -o example [lk@...alhost
> > perf_diff]$ perf record -o example_v2.data ./example [ perf record:
> > Woken up 4 times to write data ] [ perf record: Captured and wrote 0.824
> > MB example_v2.data (~36015
> > samples) ]
> >
> > Old perf diff result.
> > [lk@...alhost perf_diff]$ perf diff example_v1.data example_v2.data
> > Event 'cycles'
> > Baseline Delta Shared Object Symbol
> > ........ ....... ................ ...............................
> >
> > [kernel.vmlinux] [k] __perf_event_task_sched_out
> > 0.00% [kernel.vmlinux] [k] apic_timer_interrupt
> > [kernel.vmlinux] [k] idle_cpu
> > [kernel.vmlinux] [k] intel_pstate_timer_func
> > [kernel.vmlinux] [k] native_read_msr_safe
> > 0.00% [kernel.vmlinux] [k] native_read_tsc
> > 0.00% [kernel.vmlinux] [k] native_write_msr_safe
> > [kernel.vmlinux] [k] ntp_tick_length
> > 0.00% [kernel.vmlinux] [k] rb_erase
> > 0.00% [kernel.vmlinux] [k] tick_sched_timer
> > 0.00% [kernel.vmlinux] [k] unmap_single_vma
> > 0.00% [kernel.vmlinux] [k] update_wall_time
> > 0.00% example [.] f1
> > 46.24% example [.] f2
> > 53.71% -7.55% example [.] f3
> > +53.81% example [.] f3
> > 0.02% example [.] main
> >
> > New perf diff result.
> > [lk@...alhost perf_diff]$ perf diff example_v1.data example_v2.data
> > [kernel.vmlinux] [k] __perf_event_task_sched_out
> > 0.00% [kernel.vmlinux] [k] apic_timer_interrupt
> > [kernel.vmlinux] [k] idle_cpu
> > [kernel.vmlinux] [k] intel_pstate_timer_func
> > [kernel.vmlinux] [k] native_read_msr_safe
> > 0.00% [kernel.vmlinux] [k] native_read_tsc
> > 0.00% [kernel.vmlinux] [k] native_write_msr_safe
> > [kernel.vmlinux] [k] ntp_tick_length
> > 0.00% [kernel.vmlinux] [k] rb_erase
> > 0.00% [kernel.vmlinux] [k] tick_sched_timer
> > 0.00% [kernel.vmlinux] [k] unmap_single_vma
> > 0.00% [kernel.vmlinux] [k] update_wall_time
> > 0.00% example [.] f1
> > 46.24% -0.08% example [.] f2
> > 53.71% +0.11% example [.] f3
> > 0.02% example [.] main
> >
> > Signed-off-by: Kan Liang <kan.liang@...el.com>
> > Acked-by: Namhyung Kim <namhyung@...nel.org>
> > Acked-by: Jiri Olsa <jolsa@...nel.org>
> >
> > ---
> >
> > Changes since V4:
> > - Seperate from old patch set
> > - Add an example in changelog
> > - Update man page
> >
> > Changes since V5:
> > - Correct patch style
> >
> > Changes since V6:
> > - Update description
> >
> > tools/perf/Documentation/perf-diff.txt | 5 +++++
> > tools/perf/util/sort.c | 9 +++++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/tools/perf/Documentation/perf-diff.txt
> > b/tools/perf/Documentation/perf-diff.txt
> > index e463caa..5182661 100644
> > --- a/tools/perf/Documentation/perf-diff.txt
> > +++ b/tools/perf/Documentation/perf-diff.txt
> > @@ -20,6 +20,11 @@ If no parameters are passed it will assume
> > perf.data.old and perf.data.
> > The differential profile is displayed only for events matching both
> > specified perf.data files.
> >
> > +If no parameters are passed the samples will be sorted by dso and symbol.
> > +As the perf.data files could come from different binaries, the symbols
> > +addresses could vary. So perf diff is based on the comparison of the
> > +files and symbols name.
> > +
> > OPTIONS
> > -------
> > -D::
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c index
> > 7a39c1e..4593f36 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -1463,6 +1463,15 @@ int sort_dimension__add(const char *tok)
> > sort__has_parent = 1;
> > } else if (sd->entry == &sort_sym) {
> > sort__has_sym = 1;
> > + /*
> > + * perf diff displays the performance difference
> > amongst
> > + * two or more perf.data files. Those files could
> > come
> > + * from different binaries. So we should not
> > compare
> > + * their ips, but the name of symbol.
> > + */
> > + if (sort__mode == SORT_MODE__DIFF)
> > + sd->entry->se_collapse = sort__sym_sort;
> > +
> > } else if (sd->entry == &sort_dso) {
> > sort__has_dso = 1;
> > }
> > --
> > 1.7.11.7
--
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