| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20171020152222.GE30002@kernel.org>
Date: Fri, 20 Oct 2017 12:22:22 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Milian Wolff <milian.wolff@...b.com>
Cc: jolsa@...nel.org, namhyung@...nel.org,
Linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Arnaldo Carvalho de Melo <acme@...hat.com>,
David Ahern <dsahern@...il.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Yao Jin <yao.jin@...ux.intel.com>,
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH v6 1/6] perf report: properly handle branch count in
match_chain
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.
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.
> + 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.
- Arnaldo
commit ab950c4f4a262af1afd8cfb02c0f71acfc4eafe9
Author: Milian Wolff <milian.wolff@...b.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@...b.com>
Cc: David Ahern <dsahern@...il.com>
Cc: Jin Yao <yao.jin@...ux.intel.com>
Cc: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc: Ravi Bangoria <ravi.bangoria@...ux.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@...hat.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;
}
/*
Powered by blists - more mailing lists