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.wo...@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.wo...@kdab.com>
> Cc: David Ahern <dsah...@gmail.com>
> Cc: Jin Yao <yao....@linux.intel.com>
> Cc: Namhyung Kim <namhy...@kernel.org>
> Cc: Peter Zijlstra <a.p.zijls...@chello.nl>
> Cc: Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com>
> [ Fixed up wrt always using absolute addresses ]
> Link:
> http://lkml.kernel.org/r/20171018185350.14893-2-milian.wo...@kdab.com
> Signed-off-by: Arnaldo Carvalho de Melo <a...@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.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
