linux-kernel - Re: [PATCH] perf report: Add option to collapse undesired parts of call graph

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: <871u7p3bjb.fsf@sejong.aot.lge.com>
Date:	Wed, 26 Jun 2013 10:28:56 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Greg Price <price@....EDU>
Cc:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...hat.com>,
	David Ahern <dsahern@...il.com>
Subject: Re: [PATCH] perf report: Add option to collapse undesired parts of call graph

Hi Greg,

On Sat, 22 Jun 2013 23:17:20 -0400, Greg Price wrote:
> For example, in an application with an expensive function
> implemented with deeply nested recursive calls, the default
> call-graph presentation is dominated by the different callchains
> within that function.  By treating the function as a black box,
> we can collect the callchains leading into the function and
> compactly identify what to blame for expensive calls.
>
> For example, in this report the callers of garbage_collect() are

s/callers/callees/ ?

And it'd be better it shows more lines after garbage_collect so that one
can see its callers also to understand what it does more clearly.


> scattered across the tree:
> $ perf report -d ruby 2>- | grep -m10 ^[^#]*[a-z]
>     22.03%     ruby  [.] gc_mark
>                --- gc_mark
>                   |--59.40%-- mark_keyvalue
>                   |          st_foreach
>                   |          gc_mark_children
>                   |          |--99.75%-- rb_gc_mark
>                   |          |          rb_vm_mark
>                   |          |          gc_mark_children
>                   |          |          gc_marks
>                   |          |          |--99.00%-- garbage_collect
>
> If we make garbage_collect() a black box, its callers are coalesced:

Again, s/callers/callees/ ?


> $ perf report --blackbox garbage_collect -d ruby 2>- | grep -m10 ^[^#]*[a-z]
>     72.92%     ruby  [.] garbage_collect
>                --- garbage_collect
>                    vm_xmalloc
>                   |--47.08%-- ruby_xmalloc
>                   |          st_insert2
>                   |          rb_hash_aset
>                   |          |--98.45%-- features_index_add
>                   |          |          rb_provide_feature
>                   |          |          rb_require_safe
>                   |          |          vm_call_method
>
> Cc: Arnaldo Carvalho de Melo <acme@...stprotocols.net>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Jiri Olsa <jolsa@...hat.com>
> Cc: David Ahern <dsahern@...il.com>
> Signed-off-by: Greg Price <price@....edu>
> ---
>
> On Fri, Jan 11, 2013 at 02:27:36AM -0300, Arnaldo Carvalho de Melo wrote:
>> Looks like an interesting feature, will review this soon,
>
> Rebased on top of v3.10-rc7, please take a look at your convenience.
>
>
>  tools/perf/builtin-report.c | 19 ++++++++++++++++---
>  tools/perf/builtin-top.c    |  3 +--
>  tools/perf/util/machine.c   | 26 +++++++++++++++++---------
>  tools/perf/util/machine.h   |  9 ++++++++-
>  tools/perf/util/session.c   |  3 +--

You need to update the doc too.

>  5 files changed, 43 insertions(+), 17 deletions(-)
[SNIP]
> @@ -1211,8 +1214,13 @@ static int machine__resolve_callchain_sample(struct machine *machine,
>  					   MAP__FUNCTION, ip, &al, NULL);
>  		if (al.sym != NULL) {
>  			if (sort__has_parent && !*parent &&
> -			    symbol__match_parent_regex(al.sym))
> +			    symbol__match_regex(al.sym, &parent_regex))
>  				*parent = al.sym;
> +			else if (have_blackbox && root_al &&
> +			         symbol__match_regex(al.sym, &blackbox_regex)) {
> +				*root_al = al;
> +				callchain_cursor_reset(&callchain_cursor);

Okay, this is where the magic happens. :)

So it overwrites the original 'al' in process_sample_event() to
blackboxed symbol and drop the callchain.  Wouldn't it deserve a
comment? :)


> +			}
>  			if (!symbol_conf.use_callchain)
>  				break;
pp
This is unrelated to this patch, but why is this line needed?  I guess
this check should be done before calling this function.

Thanks,
Namhyung

--
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/