Message-ID: <572C7FB5.4000101@intel.com>
Date:	Fri, 6 May 2016 14:27:49 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Chris Phlipot <cphlipot0@...il.com>, jolsa@...nel.org,
	acme@...nel.org, peterz@...radead.org, mingo@...hat.com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] perf script: enable db export to output sampled
 callchains

On 28/04/16 11:19, Chris Phlipot wrote:
> This change enables the db export api to export callchains. This
> is accomplished by adding callchains obtained from samples to the
> call_path_root structure and exporting them via the current call path
> export API.
> 
> While the current API does support exporting call paths, this is not
> supported when sampling. This commit addresses that missing feature by
> allowing the export of call paths when callchains are present in samples.
> 
> Summary:
> -This feature is activated by initializing the call_path_root member
> 	inside the db_export structure to a non-null value.
> -Callchains are resolved with thread__resolve_callchain() and then stored
> 	and exported by adding a call path under call path root.
> -Symbol and DSO for each callchain node are exported via db_ids_from_al()
> 
> This commit puts in place infrastructure to be used by subsequent commits,
> and by itself, does not introduce any user-visible changes.
> 
> Signed-off-by: Chris Phlipot <cphlipot0@...il.com>

Looks good.  A few of nitpicks below.  But apart from those:

Acked-by: Adrian Hunter <adrian.hunter@...el.com>

> ---
>  tools/perf/util/db-export.c | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/db-export.h |  2 ++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
> index 4fc607c..cb96591 100644
> --- a/tools/perf/util/db-export.c
> +++ b/tools/perf/util/db-export.c
> @@ -15,6 +15,8 @@
>  
>  #include <errno.h>
>  
> +#include <sys/types.h>

Why?

> +
>  #include "evsel.h"
>  #include "machine.h"
>  #include "thread.h"
> @@ -23,6 +25,7 @@
>  #include "event.h"
>  #include "util.h"
>  #include "thread-stack.h"
> +#include "callchain.h"
>  #include "call-path.h"
>  #include "db-export.h"
>  
> @@ -277,6 +280,81 @@ static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
>  	return 0;
>  }
>  
> +static struct call_path *call_path_from_sample(struct db_export *dbe,
> +					       struct machine *machine,
> +					       struct thread *thread,
> +					       struct perf_sample *sample,
> +					       struct perf_evsel *evsel
> +					       )
> +{
> +	u64 kernel_start = machine__kernel_start(machine);
> +
> +	struct call_path *current = &dbe->cpr->call_path;
> +	enum chain_order saved_order = callchain_param.order;
> +
> +	int err = 0;

err needn't be initialized.  And remove blank lines in the group of local
declarations.

> +
> +	if (!symbol_conf.use_callchain || !sample->callchain)
> +		return NULL;
> +
> +	/* Since the call path tree must be built starting with the root, we

Normal kernel comment style has "/*" on separate line.

> +	 * must use ORDER_CALL for call chain resolution, in order to process
> +	 * the callchain starting with the root node and ending with the leaf.
> +	 */
> +	callchain_param.order = ORDER_CALLER;
> +	err = thread__resolve_callchain(thread, &callchain_cursor, evsel,
> +					sample, NULL, NULL,
> +					PERF_MAX_STACK_DEPTH);

Should probably be sysctl_perf_event_max_stack not PERF_MAX_STACK_DEPTH

> +	if (err) {
> +		callchain_param.order = saved_order;
> +		return NULL;
> +	}
> +	callchain_cursor_commit(&callchain_cursor);
> +
> +	while (1) {
> +		struct callchain_cursor_node *node;
> +		struct addr_location al;
> +		u64 dso_db_id = 0, sym_db_id = 0, offset = 0;
> +
> +		memset(&al, 0, sizeof(al));
> +
> +		node = callchain_cursor_current(&callchain_cursor);
> +		if (!node)
> +			break;
> +
> +		/* handle export of symbol and dso for this node by

Normal kernel comment style has "/*" on separate line.

> +		 * constructing an addr_location struct and then passing it to
> +		 * db_ids_from_al() to perform the export.
> +		 */
> +		al.sym = node->sym;
> +		al.map = node->map;
> +		al.machine = machine;
> +		if (al.map)
> +			al.addr = al.map->map_ip(al.map, node->ip);
> +		else
> +			al.addr = node->ip;
> +
> +		db_ids_from_al(dbe, &al, &dso_db_id, &sym_db_id, &offset);
> +
> +		/* add node to the call path tree if it doesn't exist */
> +		current = call_path__findnew(dbe->cpr, current,
> +					     al.sym, node->ip,
> +					     kernel_start);
> +
> +		callchain_cursor_advance(&callchain_cursor);
> +	}
> +
> +	/* Reset the callchain order to its prior value. */
> +	callchain_param.order = saved_order;
> +
> +	if (current == &dbe->cpr->call_path) {
> +		/* Bail because the callchain was empty. */
> +		return NULL;
> +	}
> +
> +	return current;
> +}
> +
>  int db_export__branch_type(struct db_export *dbe, u32 branch_type,
>  			   const char *name)
>  {
> @@ -330,6 +408,14 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
>  	if (err)
>  		goto out_put;
>  
> +	if (dbe->cpr) {
> +		struct call_path *cp = call_path_from_sample(dbe, al->machine,
> +							     thread, sample,
> +							     evsel);
> +		if (cp)
> +			db_export__call_path(dbe, cp);
> +	}
> +
>  	if ((evsel->attr.sample_type & PERF_SAMPLE_ADDR) &&
>  	    sample_addr_correlates_sym(&evsel->attr)) {
>  		struct addr_location addr_al;
> diff --git a/tools/perf/util/db-export.h b/tools/perf/util/db-export.h
> index 25e22fd..f5daf55 100644
> --- a/tools/perf/util/db-export.h
> +++ b/tools/perf/util/db-export.h
> @@ -27,6 +27,7 @@ struct dso;
>  struct perf_sample;
>  struct addr_location;
>  struct call_return_processor;
> +struct call_path_root;
>  struct call_path;
>  struct call_return;
>  
> @@ -64,6 +65,7 @@ struct db_export {
>  	int (*export_call_return)(struct db_export *dbe,
>  				  struct call_return *cr);
>  	struct call_return_processor *crp;
> +	struct call_path_root *cpr;
>  	u64 evsel_last_db_id;
>  	u64 machine_last_db_id;
>  	u64 thread_last_db_id;
> 

Powered by blists - more mailing lists