linux-kernel - Re: [PATCH v3 0/5] perf report: Show inline stack

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]

Message-ID: <7369636.07Z93mMh8K@agathebauer>
Date:   Thu, 02 Mar 2017 22:42:00 +0100
From:   Milian Wolff <milian.wolff@...b.com>
To:     "Jin, Yao" <yao.jin@...ux.intel.com>
Cc:     acme@...nel.org, jolsa@...nel.org, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com
Subject: Re: [PATCH v3 0/5] perf report: Show inline stack

On Dienstag, 21. Februar 2017 01:28:17 CET Jin, Yao wrote:
> Hi,
> 
> Any comments for this patch series?

Sorry for the delay. I just tested it again.

Overall, this is a clear improvement, so I'm all for getting this 
functionality in.

But from a usability point of view, I still have the some of the issues that I 
have raised in the past:

a) --inline should be a boolean setting that enables inline resolution on 
demand

b) the other callgraph settings and formatting should be used for inlined 
frames, i.e.

- instead of `perf report --inline-name`
  it should be: `perf report --inline -g function`
  and since `-g function` is the default, it would be the same as:
  `perf report --inline`

- instead of `perf report --inline-line -g address`
  it should be: `perf report --inline -g address`

Again: As a user of `perf report`, I do not care whether a frame is an inlined 
one or a non-inlined one - both should be grouped and displayed the same way. 

I.e. this is unusable (imo):

~~~~~~~~~~~~~~~~~~~~~~~~~
perf report --inline-line --stdio

    99.81%    35.99%  cpp-inlining  cpp-inlining      [.] main
            |          
            |--63.82%--main
                       |
                       ---/home/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/main.cpp:39 (inline)
                          /usr/include/c++/6.3.1/complex:664 (inline)
            |          |          
            |          |--63.19%--hypot
            |          |          |          
            |          |           --58.04%--__hypot_finite
            |          |          
            |           --0.62%--cabs
~~~~~~~~~~~~~~~~~~~~~~~~~

Dito for this:

~~~~~~~~~~~~~~~~~~~~~~~~~
perf report --stdio --inline-name -g address --stdio

    99.81%    35.99%  cpp-inlining  cpp-inlining      [.] main
            |          
            |--63.82%--main complex:655
                       |
                       ---main (inline)
                          std::norm<double> (inline)
~~~~~~~~~~~~~~~~~~~~~~~~~

But, again, even with these gripes, I think it's a very useful feature and I 
would like to see it integrated upstream. From my POV, you can add

Tested-by: Milian Wolff <milian.wolff@...b.com>

to all patches in this series.

Many thanks!

-- 
Milian Wolff | milian.wolff@...b.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5903 bytes)