Re: [PATCH 3/6] perf script: enable db export to output sampled callchains

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, May 06, 2016 at 10:07:12AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, May 06, 2016 at 02:27:49PM +0300, Adrian Hunter escreveu:
> > 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@gmail.com>
> > 
> > Looks good.  A few of nitpicks below.  But apart from those:
> 
> Agreed, will fix those.
>  
> > Acked-by: Adrian Hunter <adrian.hunter@intel.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?

Removed, haven't seen any new type in this patch, so no need to add a
new header.

> > > +
> > >  #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
> > > +					       )

Moved the closing parenthesis to right after 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.

Done.

> > > +
> > > +	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.

Done

> > > +	 * 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

Yeah, and at some point this should come from
evsel->attr.sample_max_stack, but that is just in my perf/max-stack
branch, I'll take care of it when the time comes.

> > > +	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.

Done

> > > +		 * 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;
> > >