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

From: Arnaldo Carvalho de Melo
Date: Fri May 06 2016 - 11:38:44 EST

Next message: Tomasz Grabiec: "PROBLEM: Scheduling latency not respected in CFS with HRTICK enabled"
Previous message: Markus Heiser: "Re: Kernel docs: muddying the waters a bit"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH 3/6] perf script: enable db export to output sampled callchains"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> >
> > Looks good. A few of nitpicks below. But apart from those:
>
> Agreed, will fix those.
>
> > Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >
> > > ---
> > > 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;
> > >

Next message: Tomasz Grabiec: "PROBLEM: Scheduling latency not respected in CFS with HRTICK enabled"
Previous message: Markus Heiser: "Re: Kernel docs: muddying the waters a bit"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH 3/6] perf script: enable db export to output sampled callchains"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]