Hi Arnaldo, On Tue, Oct 24, 2017 at 10:27:42AM -0300, Arnaldo Carvalho de Melo wrote: > Em Mon, Oct 23, 2017 at 09:39:57PM +0200, Milian Wolff escreveu: > > So to fix this all, I guess the suggested approach by Namhyung would be > > best, > > i.e. fixup my initial match_addresses to take the map, and then if the map > > is > > valid also take the dso into account when comparing the addresses: > > > > if (left_dso != right_dso) > > return left_dso < right_dso ? MATCH_LT : MATCH_GT; > > else if (left_ip != right_ip) > > return left_ip < right_ip ? MATCH_LT : MATCH_GT; > > else > > return MATCH_EQ; > > So, can you check that the patch below is the one we should commit to? > Namhyung? I'm looking at your latest patch kit, v7, to see if the branch > parts, further below, are as you submitted or if I have any issues with > it. > > I've updated my perf/core branch with all this. > > commit 275049196c64cc1233837c9f066b4b87e32cd1df > Author: Milian Wolff <[email protected]> > Date: Fri Oct 20 12:14:47 2017 -0300 > > perf report: Properly handle branch count in match_chain() > > Some of the code paths I introduced before returned too early without > running the code to handle a node's branch count. By refactoring > match_chain to only have one exit point, this can be remedied. > > Signed-off-by: Milian Wolff <[email protected]> > Cc: David Ahern <[email protected]> > Cc: Jin Yao <[email protected]> > Cc: Namhyung Kim <[email protected]> > Cc: Peter Zijlstra <[email protected]> > Cc: Ravi Bangoria <[email protected]> > Link: http://lkml.kernel.org/r/1707691.qaJ269GSZW@agathebauer > Link: > http://lkml.kernel.org/r/[email protected] > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
Acked-by: Namhyung Kim <[email protected]> Thanks, Namhyung > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c > index 35a920f09503..19bfcadcf891 100644 > --- a/tools/perf/util/callchain.c > +++ b/tools/perf/util/callchain.c > @@ -666,83 +666,99 @@ static enum match_result match_chain_strings(const char > *left, > return ret; > } > > -static enum match_result match_chain(struct callchain_cursor_node *node, > - struct callchain_list *cnode) > +/* > + * We need to always use relative addresses because we're aggregating > + * callchains from multiple threads, i.e. different address spaces, so > + * comparing absolute addresses make no sense as a symbol in a DSO may end up > + * in a different address when used in a different binary or even the same > + * binary but with some sort of address randomization technique, thus we need > + * to compare just relative addresses. -acme > + */ > +static enum match_result match_chain_dso_addresses(struct map *left_map, u64 > left_ip, > + struct map *right_map, u64 > right_ip) > { > - struct symbol *sym = node->sym; > - u64 left, right; > - struct dso *left_dso = NULL; > - struct dso *right_dso = NULL; > + struct dso *left_dso = left_map ? left_map->dso : NULL; > + struct dso *right_dso = right_map ? right_map->dso : NULL; > > - if (callchain_param.key == CCKEY_SRCLINE) { > - enum match_result match = match_chain_strings(cnode->srcline, > - node->srcline); > + if (left_dso != right_dso) > + return left_dso < right_dso ? MATCH_LT : MATCH_GT; > > - /* if no srcline is available, fallback to symbol name */ > - if (match == MATCH_ERROR && cnode->ms.sym && node->sym) > - match = match_chain_strings(cnode->ms.sym->name, > - node->sym->name); > + if (left_ip != right_ip) > + return left_ip < right_ip ? MATCH_LT : MATCH_GT; > > - if (match != MATCH_ERROR) > - return match; > + return MATCH_EQ; > +} > > - /* otherwise fall-back to IP-based comparison below */ > - } > +static enum match_result match_chain(struct callchain_cursor_node *node, > + struct callchain_list *cnode) > +{ > + enum match_result match = MATCH_ERROR; > > - if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) { > - /* > - * Compare inlined frames based on their symbol name because > - * different inlined frames will have the same symbol start > - */ > - if (cnode->ms.sym->inlined || node->sym->inlined) > - return match_chain_strings(cnode->ms.sym->name, > - node->sym->name); > - > - left = cnode->ms.sym->start; > - right = sym->start; > - left_dso = cnode->ms.map->dso; > - right_dso = node->map->dso; > - } else { > - left = cnode->ip; > - right = node->ip; > + switch (callchain_param.key) { > + case CCKEY_SRCLINE: > + match = match_chain_strings(cnode->srcline, node->srcline); > + if (match != MATCH_ERROR) > + break; > + /* otherwise fall-back to symbol-based comparison below */ > + __fallthrough; > + case CCKEY_FUNCTION: > + if (node->sym && cnode->ms.sym) { > + /* > + * Compare inlined frames based on their symbol name > + * because different inlined frames will have the same > + * symbol start. Otherwise do a faster comparison based > + * on the symbol start address. > + */ > + if (cnode->ms.sym->inlined || node->sym->inlined) { > + match = match_chain_strings(cnode->ms.sym->name, > + node->sym->name); > + if (match != MATCH_ERROR) > + break; > + } else { > + match = > match_chain_dso_addresses(cnode->ms.map, cnode->ms.sym->start, > + node->map, > node->sym->start); > + break; > + } > + } > + /* otherwise fall-back to IP-based comparison below */ > + __fallthrough; > + case CCKEY_ADDRESS: > + default: > + match = match_chain_dso_addresses(cnode->ms.map, cnode->ip, > node->map, node->ip); > + break; > } > > - if (left == right && left_dso == right_dso) { > - if (node->branch) { > - cnode->branch_count++; > + if (match == MATCH_EQ && node->branch) { > + cnode->branch_count++; > > - if (node->branch_from) { > - /* > - * It's "to" of a branch > - */ > - cnode->brtype_stat.branch_to = true; > + if (node->branch_from) { > + /* > + * It's "to" of a branch > + */ > + cnode->brtype_stat.branch_to = true; > > - if (node->branch_flags.predicted) > - cnode->predicted_count++; > + if (node->branch_flags.predicted) > + cnode->predicted_count++; > > - if (node->branch_flags.abort) > - cnode->abort_count++; > + if (node->branch_flags.abort) > + cnode->abort_count++; > > - branch_type_count(&cnode->brtype_stat, > - &node->branch_flags, > - node->branch_from, > - node->ip); > - } else { > - /* > - * It's "from" of a branch > - */ > - cnode->brtype_stat.branch_to = false; > - cnode->cycles_count += > - node->branch_flags.cycles; > - cnode->iter_count += node->nr_loop_iter; > - cnode->iter_cycles += node->iter_cycles; > - } > + branch_type_count(&cnode->brtype_stat, > + &node->branch_flags, > + node->branch_from, > + node->ip); > + } else { > + /* > + * It's "from" of a branch > + */ > + cnode->brtype_stat.branch_to = false; > + cnode->cycles_count += node->branch_flags.cycles; > + cnode->iter_count += node->nr_loop_iter; > + cnode->iter_cycles += node->iter_cycles; > } > - > - return MATCH_EQ; > } > > - return left > right ? MATCH_GT : MATCH_LT; > + return match; > } > > /*
