Messages in this thread | | | Date | Fri, 6 May 2016 12:38:31 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 3/6] perf script: enable db export to output sampled callchains |
| |
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; > > >
| |