Messages in this thread | | | From | Milian Wolff <> | Subject | Re: [PATCH v6 1/6] perf report: properly handle branch count in match_chain | Date | Fri, 20 Oct 2017 21:52:22 +0200 |
| |
On Freitag, 20. Oktober 2017 17:22:22 CEST Arnaldo Carvalho de Melo wrote: > Em Wed, Oct 18, 2017 at 08:53:45PM +0200, Milian Wolff escreveu: > > 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. > > + 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); > > + else > > + match = match_address_dso(cnode->ms.map->dso, > > + cnode->ms.sym->start, > > + node->map->dso, > > + node->sym->start); > > + if (match != MATCH_ERROR) > > + break; > > + } > > > > /* otherwise fall-back to IP-based comparison below */ > > > > + __fallthrough; > > If we take this __falltrough because cnode->sym or cnode->ms.sym is > NULL, then cnode->ms.map may be NULL if we got a sample for which we > somehow couldn't find a map.
Yes, that was fixed in v7.
> And we don't really need to deal with DSOs, just with MAPs, to go from > relative to absolute when we _have_ a symbol resolved, cnode->ip and > node->ip are already absolute.
That's confusing, can you rephrase? Either we have a MAP/DSO and the ip can be relative or absolute. Or we don't, and then we don't have a symbol and the ip will remain absolute as we cannot remap it to the relative address. So is the sentence above maybe missing a negation somewhere? I.e. "when we _have *not*_ resolved a symbol, cnode->ip and node->ip are already absolute"?
> > + case CCKEY_ADDRESS: > > + default: > > + match = match_address_dso(cnode->ms.map->dso, cnode->ip, > > + node->map->dso, node->ip); > > Ok, below is this patch updated on top of my previous patch, please take > a look, I'll be adding all this to my tmp.perf/core branch, holler if > you disagree on moving it to perf/core, which I'd like to do soon.
I'll have a look at tmp.perf/core now, thanks.
> commit ab950c4f4a262af1afd8cfb02c0f71acfc4eafe9 > Author: Milian Wolff <milian.wolff@kdab.com> > 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 <milian.wolff@kdab.com> > Cc: David Ahern <dsahern@gmail.com> > Cc: Jin Yao <yao.jin@linux.intel.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> > [ Fixed up wrt always using absolute addresses ] > Link: > http://lkml.kernel.org/r/20171018185350.14893-2-milian.wolff@kdab.com > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c > index 1ac3f4a5afab..eac1c9bc9d5b 100644 > --- a/tools/perf/util/callchain.c > +++ b/tools/perf/util/callchain.c > @@ -666,79 +666,88 @@ static enum match_result match_chain_strings(const > char *left, return ret; > } > > +static enum match_result match_chain_addresses(u64 left_ip, u64 right_ip) > +{ > + if (left_ip == right_ip) > + return MATCH_EQ; > + else if (left_ip < right_ip) > + return MATCH_LT; > + else > + return MATCH_GT; > +} > + > static enum match_result match_chain(struct callchain_cursor_node *node, > struct callchain_list *cnode) > { > - struct symbol *sym = node->sym; > - u64 left, right; > - > - if (callchain_param.key == CCKEY_SRCLINE) { > - enum match_result match = match_chain_strings(cnode->srcline, > - node->srcline); > - > - /* 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); > + enum match_result match = MATCH_ERROR; > > + switch (callchain_param.key) { > + case CCKEY_SRCLINE: > + match = match_chain_strings(cnode->srcline, node->srcline); > if (match != MATCH_ERROR) > - return match; > + 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 { > + u64 left = cnode->ms.map->unmap_ip(cnode->ms.map, > cnode->ms.sym->start), + right = node->map->unmap_ip(node- >map, > node->sym->start); > > + match = match_chain_addresses(left, right); > + break; > + } > + } > /* otherwise fall-back to IP-based comparison below */ > + __fallthrough; > + case CCKEY_ADDRESS: > + default: > + match = match_chain_addresses(cnode->ip, node->ip); > + break; > } > > - 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.map->unmap_ip(cnode->ms.map, cnode->ms.sym->start); > - right = node->map->unmap_ip(node->map, sym->start); > - } else { > - left = cnode->ip; > - right = node->ip; > - } > - > - if (left == right) { > - 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; > } > > /*
-- Milian Wolff | milian.wolff@kdab.com | Senior Software Engineer KDAB (Deutschland) GmbH&Co KG, a KDAB Group company Tel: +49-30-521325470 KDAB - The Qt Experts
| |