From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758408AbcEFPil (ORCPT ); Fri, 6 May 2016 11:38:41 -0400 Received: from mail.kernel.org ([198.145.29.136]:54688 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757596AbcEFPik (ORCPT ); Fri, 6 May 2016 11:38:40 -0400 Date: Fri, 6 May 2016 12:38:31 -0300 From: Arnaldo Carvalho de Melo To: Adrian Hunter Cc: Chris Phlipot , jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/6] perf script: enable db export to output sampled callchains Message-ID: <20160506153831.GE11069@kernel.org> References: <1461831551-12213-1-git-send-email-cphlipot0@gmail.com> <1461831551-12213-4-git-send-email-cphlipot0@gmail.com> <572C7FB5.4000101@intel.com> <20160506130712.GD11069@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160506130712.GD11069@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 > > > > Looks good. A few of nitpicks below. But apart from those: > > Agreed, will fix those. > > > Acked-by: Adrian Hunter > > > > > --- > > > 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 > > > > > > +#include > > > > 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; > > >