[PATCH] [perf] Add inverted call graph report support.

[PATCH] [perf] Add inverted call graph report support.

[Sam Liao]

Add "caller/callee" option to support inverted butterfly report,
in the inverted report (with caller option), the call graph start
from the callee's ancestor. Users can use such view to catch system's
performance bottleneck from a sysprof like view. Using this option
with specified sort order like pid gives us high level view of call
graph statistics.
---
 tools/perf/builtin-report.c |   28 ++++++++++++++++++++++------
 tools/perf/util/callchain.h |    6 ++++++
 tools/perf/util/hist.c      |    3 ++-
 tools/perf/util/session.c   |    7 ++++++-
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 287a173..2ceac45 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -45,7 +45,7 @@ static struct perf_read_values	show_threads_values;
 static const char	default_pretty_printing_style[] = "normal";
 static const char	*pretty_printing_style = default_pretty_printing_style;

-static char		callchain_default_opt[] = "fractal,0.5";
+static char		callchain_default_opt[] = "fractal,0.5,callee";
 static symbol_filter_t	annotate_init;

 static int perf_session__add_hist_entry(struct perf_session *session,
@@ -386,13 +386,29 @@ parse_callchain_opt(const struct option *opt
__used, const char *arg,
 	if (!tok)
 		goto setup;

-	tok2 = strtok(NULL, ",");
 	callchain_param.min_percent = strtod(tok, &endptr);
 	if (tok == endptr)
 		return -1;

-	if (tok2)
+	/* get the print limit */
+	tok2 = strtok(NULL, ",");
+	if (!tok2)
+		goto setup;
+
+	if (tok2[0] != 'c') {
 		callchain_param.print_limit = strtod(tok2, &endptr);
+		tok2 = strtok(NULL, ",");
+		if (!tok2)
+			goto setup;
+	}
+
+	/* get the call chain order */
+	if (!strcmp(tok2, "caller"))
+		callchain_param.order = ORDER_CALLER;
+	else if (!strcmp(tok2, "callee"))
+		callchain_param.order = ORDER_CALLEE;
+	else
+		return -1;
 setup:
 	if (callchain_register_param(&callchain_param) < 0) {
 		fprintf(stderr, "Can't register callchain params\n");
@@ -436,9 +452,9 @@ static const struct option options[] = {
 		   "regex filter to identify parent, see: '--sort parent'"),
 	OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
 		    "Only display entries with parent-match"),
-	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL, "output_type,min_percent",
-		     "Display callchains using output_type (graph, flat, fractal,
or none) and min percent threshold. "
-		     "Default: fractal,0.5", &parse_callchain_opt, callchain_default_opt),
+	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL,
"output_type,min_percent, call_order",
+		     "Display callchains using output_type (graph, flat, fractal,
or none) , min percent threshold and callchain order. "
+		     "Default: fractal,0.5,callee", &parse_callchain_opt,
callchain_default_opt),
 	OPT_STRING('d', "dsos", &symbol_conf.dso_list_str, "dso[,dso...]",
 		   "only consider symbols in these dsos"),
 	OPT_STRING('C', "comms", &symbol_conf.comm_list_str, "comm[,comm...]",
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 1a79df9..9b4ff16 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -14,6 +14,11 @@ enum chain_mode {
 	CHAIN_GRAPH_REL
 };

+enum chain_order {
+	ORDER_CALLER,
+	ORDER_CALLEE
+};
+
 struct callchain_node {
 	struct callchain_node	*parent;
 	struct list_head	siblings;
@@ -41,6 +46,7 @@ struct callchain_param {
 	u32			print_limit;
 	double			min_percent;
 	sort_chain_func_t	sort;
+	enum chain_order	order;
 };

 struct callchain_list {
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 627a02e..dae4202 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -14,7 +14,8 @@ enum hist_filter {

 struct callchain_param	callchain_param = {
 	.mode	= CHAIN_GRAPH_REL,
-	.min_percent = 0.5
+	.min_percent = 0.5,
+	.order  = ORDER_CALLEE
 };

 u16 hists__col_len(struct hists *self, enum hist_column col)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f5a8fbd..e545a4d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -247,9 +247,14 @@ int perf_session__resolve_callchain(struct
perf_session *self,
 	callchain_cursor_reset(&self->callchain_cursor);

 	for (i = 0; i < chain->nr; i++) {
-		u64 ip = chain->ips[i];
+		u64 ip;
 		struct addr_location al;

+		if (callchain_param.order == ORDER_CALLEE)
+			ip = chain->ips[i];
+		else
+			ip = chain->ips[chain->nr - i - 1];
+
 		if (ip >= PERF_CONTEXT_MAX) {
 			switch (ip) {
 			case PERF_CONTEXT_HV:
-- 
1.7.4.1

Re: [PATCH] [perf] Add inverted call graph report support.

[Arun Sharma]

On Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao wrote:
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -247,9 +247,14 @@ int perf_session__resolve_callchain(struct
> perf_session *self,
>  	callchain_cursor_reset(&self->callchain_cursor);
> 
>  	for (i = 0; i < chain->nr; i++) {
> -		u64 ip = chain->ips[i];
> +		u64 ip;
>  		struct addr_location al;
> 
> +		if (callchain_param.order == ORDER_CALLEE)
> +			ip = chain->ips[i];
> +		else
> +			ip = chain->ips[chain->nr - i - 1];

This can dereference a bad pointer if chain->nr == 0.

 -Arun

Re: [PATCH] [perf] Add inverted call graph report support.

[David Ahern]

On 06/09/2011 01:33 PM, Arun Sharma wrote:
> On Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao wrote:
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -247,9 +247,14 @@ int perf_session__resolve_callchain(struct
>> perf_session *self,
>>  	callchain_cursor_reset(&self->callchain_cursor);
>>
>>  	for (i = 0; i < chain->nr; i++) {
>> -		u64 ip = chain->ips[i];
>> +		u64 ip;
>>  		struct addr_location al;
>>
>> +		if (callchain_param.order == ORDER_CALLEE)
>> +			ip = chain->ips[i];
>> +		else
>> +			ip = chain->ips[chain->nr - i - 1];
> 
> This can dereference a bad pointer if chain->nr == 0.

Should not enter the loop if chain->nr is 0 (See for arg).

David


> 
>  -Arun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [perf] Add inverted call graph report support.

[Arun Sharma]

On 6/9/11 1:10 PM, David Ahern wrote:
>
>
> On 06/09/2011 01:33 PM, Arun Sharma wrote:
>> On Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao wrote:
>>> --- a/tools/perf/util/session.c
>>> +++ b/tools/perf/util/session.c
>>> @@ -247,9 +247,14 @@ int perf_session__resolve_callchain(struct
>>> perf_session *self,
>>>   	callchain_cursor_reset(&self->callchain_cursor);
>>>
>>>   	for (i = 0; i<  chain->nr; i++) {
>>> -		u64 ip = chain->ips[i];
>>> +		u64 ip;
>>>   		struct addr_location al;
>>>
>>> +		if (callchain_param.order == ORDER_CALLEE)
>>> +			ip = chain->ips[i];
>>> +		else
>>> +			ip = chain->ips[chain->nr - i - 1];
>>
>> This can dereference a bad pointer if chain->nr == 0.
>
> Should not enter the loop if chain->nr is 0 (See for arg).

Ah right. I had this problem with an earlier version of the patch. This 
version seems to be fine.

  -Arun

Re: [PATCH] [perf] Add inverted call graph report support.

[Arun Sharma]

Sam,

$ perf report -g graph,0.5,caller --sort pid

is a bit hard to type. Perhaps add a simpler command line option
that has the same effect?

 -Arun

Re: [PATCH] [perf] Add inverted call graph report support.

[Sam Liao]

On Wed, Jun 15, 2011 at 6:15 AM, Arun Sharma <asharma@fb.com> wrote:
> Sam,
>
> $ perf report -g graph,0.5,caller --sort pid
>
> is a bit hard to type. Perhaps add a simpler command line option
> that has the same effect?
>
>  -Arun
>
>

How about perf report -g [caller/callee,][graph,0.1] -s pid

or remove the order of the options for "-g"

-Sam

Re: [PATCH] [perf] Add inverted call graph report support.

[Ingo Molnar]

* Sam Liao <phyomh@gmail.com> wrote:

> On Wed, Jun 15, 2011 at 6:15 AM, Arun Sharma <asharma@fb.com> wrote:
> > Sam,
> >
> > $ perf report -g graph,0.5,caller --sort pid
> >
> > is a bit hard to type. Perhaps add a simpler command line option
> > that has the same effect?
> >
> >  -Arun
> >
> >
> 
> How about perf report -g [caller/callee,][graph,0.1] -s pid
> 
> or remove the order of the options for "-g"

We could alias the -G flag to the inverse graph.

Plus it would be nice to allow a .perfconfig flag for people to 
configure their desired default reporting parameters.

Thanks,

	Ingo

Re: [PATCH] [perf] Add inverted call graph report support.

[Frederic Weisbecker]

On Wed, Jun 15, 2011 at 10:15:18AM +0200, Ingo Molnar wrote:
> 
> * Sam Liao <phyomh@gmail.com> wrote:
> 
> > On Wed, Jun 15, 2011 at 6:15 AM, Arun Sharma <asharma@fb.com> wrote:
> > > Sam,
> > >
> > > $ perf report -g graph,0.5,caller --sort pid
> > >
> > > is a bit hard to type. Perhaps add a simpler command line option
> > > that has the same effect?
> > >
> > >  -Arun
> > >
> > >
> > 
> > How about perf report -g [caller/callee,][graph,0.1] -s pid
> > 
> > or remove the order of the options for "-g"
> 
> We could alias the -G flag to the inverse graph.

Agreed.
-g could map to fine tuning of callchain parameters like it does right now
and we can use -G as a shortcut for inverted callchain keeping the default
settings.
 
> Plus it would be nice to allow a .perfconfig flag for people to 
> configure their desired default reporting parameters.

Yeah.

It would be also nice to have a "caller" sorting mode:

	perf report -G -s caller

This can be useful to get the entire tree of calls for each callers.

Re: [PATCH] [perf] Add inverted call graph report support.

[Arnaldo Carvalho de Melo]

Em Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao escreveu:
> Add "caller/callee" option to support inverted butterfly report,
> in the inverted report (with caller option), the call graph start
> from the callee's ancestor. Users can use such view to catch system's
> performance bottleneck from a sysprof like view. Using this option
> with specified sort order like pid gives us high level view of call
> graph statistics.

Looks OK, haven't had time to test tho.

Frédéric, can you process this one? Tomorrow I'll be off the grid,
vacations!

Thanks a lot,

- Arnaldo

> ---
>  tools/perf/builtin-report.c |   28 ++++++++++++++++++++++------
>  tools/perf/util/callchain.h |    6 ++++++
>  tools/perf/util/hist.c      |    3 ++-
>  tools/perf/util/session.c   |    7 ++++++-
>  4 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 287a173..2ceac45 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -45,7 +45,7 @@ static struct perf_read_values	show_threads_values;
>  static const char	default_pretty_printing_style[] = "normal";
>  static const char	*pretty_printing_style = default_pretty_printing_style;
> 
> -static char		callchain_default_opt[] = "fractal,0.5";
> +static char		callchain_default_opt[] = "fractal,0.5,callee";
>  static symbol_filter_t	annotate_init;
> 
>  static int perf_session__add_hist_entry(struct perf_session *session,
> @@ -386,13 +386,29 @@ parse_callchain_opt(const struct option *opt
> __used, const char *arg,
>  	if (!tok)
>  		goto setup;
> 
> -	tok2 = strtok(NULL, ",");
>  	callchain_param.min_percent = strtod(tok, &endptr);
>  	if (tok == endptr)
>  		return -1;
> 
> -	if (tok2)
> +	/* get the print limit */
> +	tok2 = strtok(NULL, ",");
> +	if (!tok2)
> +		goto setup;
> +
> +	if (tok2[0] != 'c') {
>  		callchain_param.print_limit = strtod(tok2, &endptr);
> +		tok2 = strtok(NULL, ",");
> +		if (!tok2)
> +			goto setup;
> +	}
> +
> +	/* get the call chain order */
> +	if (!strcmp(tok2, "caller"))
> +		callchain_param.order = ORDER_CALLER;
> +	else if (!strcmp(tok2, "callee"))
> +		callchain_param.order = ORDER_CALLEE;
> +	else
> +		return -1;
>  setup:
>  	if (callchain_register_param(&callchain_param) < 0) {
>  		fprintf(stderr, "Can't register callchain params\n");
> @@ -436,9 +452,9 @@ static const struct option options[] = {
>  		   "regex filter to identify parent, see: '--sort parent'"),
>  	OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
>  		    "Only display entries with parent-match"),
> -	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL, "output_type,min_percent",
> -		     "Display callchains using output_type (graph, flat, fractal,
> or none) and min percent threshold. "
> -		     "Default: fractal,0.5", &parse_callchain_opt, callchain_default_opt),
> +	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL,
> "output_type,min_percent, call_order",
> +		     "Display callchains using output_type (graph, flat, fractal,
> or none) , min percent threshold and callchain order. "
> +		     "Default: fractal,0.5,callee", &parse_callchain_opt,
> callchain_default_opt),
>  	OPT_STRING('d', "dsos", &symbol_conf.dso_list_str, "dso[,dso...]",
>  		   "only consider symbols in these dsos"),
>  	OPT_STRING('C', "comms", &symbol_conf.comm_list_str, "comm[,comm...]",
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index 1a79df9..9b4ff16 100644
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -14,6 +14,11 @@ enum chain_mode {
>  	CHAIN_GRAPH_REL
>  };
> 
> +enum chain_order {
> +	ORDER_CALLER,
> +	ORDER_CALLEE
> +};
> +
>  struct callchain_node {
>  	struct callchain_node	*parent;
>  	struct list_head	siblings;
> @@ -41,6 +46,7 @@ struct callchain_param {
>  	u32			print_limit;
>  	double			min_percent;
>  	sort_chain_func_t	sort;
> +	enum chain_order	order;
>  };
> 
>  struct callchain_list {
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 627a02e..dae4202 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -14,7 +14,8 @@ enum hist_filter {
> 
>  struct callchain_param	callchain_param = {
>  	.mode	= CHAIN_GRAPH_REL,
> -	.min_percent = 0.5
> +	.min_percent = 0.5,
> +	.order  = ORDER_CALLEE
>  };
> 
>  u16 hists__col_len(struct hists *self, enum hist_column col)
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f5a8fbd..e545a4d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -247,9 +247,14 @@ int perf_session__resolve_callchain(struct
> perf_session *self,
>  	callchain_cursor_reset(&self->callchain_cursor);
> 
>  	for (i = 0; i < chain->nr; i++) {
> -		u64 ip = chain->ips[i];
> +		u64 ip;
>  		struct addr_location al;
> 
> +		if (callchain_param.order == ORDER_CALLEE)
> +			ip = chain->ips[i];
> +		else
> +			ip = chain->ips[chain->nr - i - 1];
> +
>  		if (ip >= PERF_CONTEXT_MAX) {
>  			switch (ip) {
>  			case PERF_CONTEXT_HV:
> -- 
> 1.7.4.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [perf] Add inverted call graph report support.

[Frederic Weisbecker]

On Fri, Jun 10, 2011 at 05:42:08PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao escreveu:
> > Add "caller/callee" option to support inverted butterfly report,
> > in the inverted report (with caller option), the call graph start
> > from the callee's ancestor. Users can use such view to catch system's
> > performance bottleneck from a sysprof like view. Using this option
> > with specified sort order like pid gives us high level view of call
> > graph statistics.
> 
> Looks OK, haven't had time to test tho.
> 
> Frédéric, can you process this one? Tomorrow I'll be off the grid,
> vacations!
> 
> Thanks a lot,
> 
> - Arnaldo

Yeah, the patch is nice and works well.

I'm applying it.

Thanks.

Re: [PATCH] [perf] Add inverted call graph report support.

[Frederic Weisbecker]

On Thu, Jun 09, 2011 at 02:44:19PM +0800, Sam Liao wrote:
> Add "caller/callee" option to support inverted butterfly report,
> in the inverted report (with caller option), the call graph start
> from the callee's ancestor. Users can use such view to catch system's
> performance bottleneck from a sysprof like view. Using this option
> with specified sort order like pid gives us high level view of call
> graph statistics.

Sam, can you give me your "Signed-off-by:" tag otherwise I can't apply
the patch.

Thanks.