| 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
| ||
|
Date: Fri, 13 Oct 2017 11:08:34 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Milian Wolff <milian.wolff@...b.com>
Cc: Jiri Olsa <jolsa@...nel.org>, Jin Yao <yao.jin@...ux.intel.com>,
Linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Arnaldo Carvalho de Melo <acme@...hat.com>,
David Ahern <dsahern@...il.com>,
Namhyung Kim <namhyung@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH v5 11/16] perf report: properly handle branch count in
match_chain
Em Fri, Oct 13, 2017 at 10:39:03AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Oct 09, 2017 at 10:33:05PM +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.
>
> Fixing up this one now.
Millian, this is all fresher in your mind, can you please take a look at
my perf/core branch and check if the change i made to ]PATCH v5 09/16]
"perf report: compare symbol name for inlined frames when matching" is
ok wrt Ravi's fix and then, please, rebase v5 on top of what is there?
Ravi, please take a look at this as well, to see if with these changes
your fix remains valid, ok?
Thanks,
- Arnaldo
> > Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> > Cc: David Ahern <dsahern@...il.com>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> > Cc: Yao Jin <yao.jin@...ux.intel.com>
> > Signed-off-by: Milian Wolff <milian.wolff@...b.com>
> > ---
> > tools/perf/util/callchain.c | 117 +++++++++++++++++++++++---------------------
> > 1 file changed, 60 insertions(+), 57 deletions(-)
> >
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 3f1431bf71bd..782de047c902 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -666,78 +666,81 @@ static enum match_result match_chain_strings(const char *left,
> > return ret;
> > }
> >
> > +static enum match_result match_address(u64 left, u64 right)
> > +{
> > + if (left == right)
> > + return MATCH_EQ;
> > + else if (left < right)
> > + 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;
> > - enum match_result match;
> > - u64 left, right;
> > + enum match_result match = MATCH_ERROR;
> >
> > - if (callchain_param.key == CCKEY_SRCLINE) {
> > + switch (callchain_param.key) {
> > + case CCKEY_SRCLINE:
> > 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);
> > -
> > if (match != MATCH_ERROR)
> > - return match;
> > -
> > - /* otherwise fall-back to IP-based comparison below */
> > - }
> > -
> > - 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;
> > - } else {
> > - left = cnode->ip;
> > - right = node->ip;
> > + break;
> > + __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);
> > + else
> > + match = match_address(cnode->ms.sym->start,
> > + node->sym->start);
> > + if (match != MATCH_ERROR)
> > + break;
> > + }
> > + __fallthrough;
> > + case CCKEY_ADDRESS:
> > + default:
> > + match = match_address(cnode->ip, node->ip);
> > + break;
> > }
> >
> > - 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;
> > }
> >
> > /*
> > --
> > 2.14.2
Powered by blists - more mailing lists