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

- Arnaldo
 
> 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?
> 
> > +
> >  #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;
> >