linux-kernel - Re: [PATCHSET 0/3] perf diff: Introduce delta-abs compute method

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Feb 2017 23:26:16 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Jiri Olsa <jolsa@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Minchan Kim <minchan@...nel.org>,
        Taeung Song <treeze.taeung@...il.com>
Subject: Re: [PATCHSET 0/3] perf diff: Introduce delta-abs compute method

Hi Arnaldo,

On Mon, Feb 06, 2017 at 09:51:49AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 06, 2017 at 04:20:34PM +0900, Namhyung Kim escreveu:
> > Hello,
> > 
> > This patchset adds 'delta-abs' compute method to -c/--compute option.
> > The 'delta-abs' is same as 'delta' but shows entries with bigger
> > absolute delta first instead of sorting numerically.  This is only
> > useful together with -o option.
> > 
> > Below is default output (-c delta):
> > 
> >   $ perf diff -o 1 -c delta | grep -v ^# | head
> >     42.22%   +4.97%  [kernel.kallsyms]  [k] cfb_imageblit
> >      0.62%   +1.23%  [kernel.kallsyms]  [k] mutex_lock
> >              +1.15%  [kernel.kallsyms]  [k] copy_user_generic_string
> >      2.40%   +0.95%  [kernel.kallsyms]  [k] bit_putcs
> >      0.31%   +0.79%  [kernel.kallsyms]  [k] link_path_walk
> >              +0.64%  [kernel.kallsyms]  [k] kmem_cache_alloc
> >      0.00%   +0.57%  [kernel.kallsyms]  [k] __rcu_read_unlock
> >              +0.45%  [kernel.kallsyms]  [k] alloc_set_pte
> >      0.16%   +0.45%  [kernel.kallsyms]  [k] menu_select
> >              +0.41%  ld-2.24.so         [.] do_lookup_x
> > 
> > Now with 'delta-abs' it shows entries have bigger delta value either
> > positive or negative.
> > 
> >   $ perf diff -o 1 -c delta-abs | grep -v ^# | head
> >     42.22%   +4.97%  [kernel.kallsyms]  [k] cfb_imageblit
> >     12.72%   -3.01%  [kernel.kallsyms]  [k] intel_idle
> >      9.72%   -1.31%  [unknown]          [.] 0x0000000000411343
> >      0.62%   +1.23%  [kernel.kallsyms]  [k] mutex_lock
> >              +1.15%  [kernel.kallsyms]  [k] copy_user_generic_string
> >      2.40%   +0.95%  [kernel.kallsyms]  [k] bit_putcs
> >      0.31%   +0.79%  [kernel.kallsyms]  [k] link_path_walk
> >      1.35%   -0.71%  [kernel.kallsyms]  [k] smp_call_function_single
> >              +0.64%  [kernel.kallsyms]  [k] kmem_cache_alloc
> >      0.00%   +0.57%  [kernel.kallsyms]  [k] __rcu_read_unlock
> > 
> > The patch 2 and 3 are to add config options to control the default
> > behavior of perf diff command.  I think that it's worth consider
> > changing the default to use 'delta-abs' method since users want to see
> > where the difference occurs actually (either positive or negative) IMHO.
> 
> I agree on having the default changed to 'delta-abs', Ingo?

Good.  Also, as I said in the changelog, it needs to change default
value of -o option to 1 in order to make the 'delta-abs' effective.

> 
> Namhyung, and perhaps we should have a single letter option to do that
> '| grep -v ^#' bit :-) and perhaps we also should have, for all tools
> the equivalent of that "| head", that git log has:
> 
> [acme@...et linux]$ git log --oneline -5
> d7cb3a507d23 Merge tag 'perf-core-for-mingo-4.11-20170201' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core
> 5443624bedd0 perf/x86/intel/pt: Add format strings for PTWRITE and power event tracing
> b05d1093987a perf ftrace: Add ftrace.tracer config option
> 43d41deb71fe perf tools: Create for_each_event macro for tracepoints iteration
> a26305363d4b perf test: Add libbpf pinning test
> [acme@...et linux]$
> 
> That '-5' to show just the first 5 lines worth of output.
> 
> With all that we would have:
> 
>   perf diff -o 1 -q10
> 
> As the equivalent to "perf diff -o 1 -c delta-abs | grep -v ^# | head".

The -q/--quiet looks ok since it corresponds to -v/--verbose option.
But I'm not sure about the number option.

In case of git, it'll stop processing commits after the given number
of them, so it will reduce significant processing time IMHO.  However,
in perf, we need to process whole data anyway and sort at the final
stage, and then stop displaying entries after the given number.

Maybe it's just a shortcut of piping to the head command.  Then I
don't feel the strong desire to have it as we have pager, TUI and GUI
already.

Thanks,
Namhyung


> 
> Ah, adding Paul McKenney to the CC list, he may have something to add
> here.
> 
> - Arnaldo
>  
> > The code is avaiable at 'perf/diff-delta-abs-v1' branch in
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > 
> > Thanks,
> > Namhyung
> > 
> > 
> > Namhyung Kim (3):
> >   perf diff: Add 'delta-abs' compute method
> >   perf diff: Add diff.order config option
> >   perf diff: Add diff.compute config option
> > 
> >  tools/perf/Documentation/perf-config.txt | 12 +++++
> >  tools/perf/Documentation/perf-diff.txt   | 15 +++++--
> >  tools/perf/builtin-diff.c                | 76 ++++++++++++++++++++++++++++++--
> >  3 files changed, 97 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.11.0