From: Andi Kleen <andi@firstfloor.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>,
jolsa@redhat.com, linux-kernel@vger.kernel.org, acme@kernel.org,
Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH 1/8] perf, tools: Support handling complete branch stacks as histograms
Date: Wed, 12 Nov 2014 00:31:53 +0100 [thread overview]
Message-ID: <20141111233153.GK12538@two.firstfloor.org> (raw)
In-Reply-To: <87r3y0gbk8.fsf@sejong.aot.lge.com>
Sorry for the long delay. Just revisiting that.
On Wed, Oct 22, 2014 at 10:03:51AM +0900, Namhyung Kim wrote:
> > | | f2 tcall.c:5
> > | | f1 tcall.c:12
> > | | f1 tcall.c:12
> > | | f2 tcall.c:7
> > | | f2 tcall.c:5
> > | | f1 tcall.c:11
>
> I think it'd be better if it just prints function names as normal
> callchain does (and optionally srcline with a switch) and duplicates
> removed like below:
>
> 54.91% tcall.c:6 [.] f2 tcall
> |
> |--65.53%-- f2 tcall.c:5
> | |
> | |--70.83%-- f1
> | | main
> | | f1
> | | f2
> | | f1
> | | f2
I considered this. For this example it doesn't make much difference
because the functions are so small.
But for anything larger I really need the line numbers to make
sense of it.
So I prefer to keep them. I'll look into some easy switch
to turn them off though.
> > + if (sort__has_parent && !*parent &&
> > + symbol__match_regex(al.sym, &parent_regex))
> > + *parent = al.sym;
> > + else if (have_ignore_callees && root_al &&
> > + symbol__match_regex(al.sym, &ignore_callees_regex)) {
> > + /* Treat this symbol as the root,
> > + forgetting its callees. */
> > + *root_al = al;
> > + callchain_cursor_reset(&callchain_cursor);
> > + }
> > + if (!symbol_conf.use_callchain)
> > + return -EINVAL;
>
> This check already went away.
>
> And, to remove duplicates, I think we need to check last callchain
> cursor node wrt the callchain_param.key here.
I don't understand the comment. I'm not modifying anything
that has been already added to the callchain. Just things
to be added in the future. So why would I need to check
or change the cursor?
>
> Also, by comparing 'from' address, I'd expect you add the from address
> alone but you add both of 'from' and 'to'. Do we really need to do
> that?
Adding from and to makes it much clearer to the user what happens,
especially with conditional branches, so they can follow the
control flow.
> And the first address saved in normal callchain is address of the
> function itself so it might be 'to' you need to check if sampled before
> any branch in a function.
I'm checking against the CALL, not the target.
>
> > + } else
> > + be[i] = branch->entries[branch->nr - i - 1];
> > + }
> > +
> > + nr = remove_loops(be, nr);
> > +
> > + for (i = 0; i < nr; i++) {
> > + err = add_callchain_ip(machine, thread, parent,
> > + root_al,
> > + -1, be[i].to);
> > + if (!err)
> > + err = add_callchain_ip(machine, thread,
> > + parent, root_al,
> > + -1, be[i].from);
> > + if (err == -EINVAL)
> > + break;
> > + if (err)
> > + return err;
> > + }
> > + chain_nr -= nr;
>
> I'm not sure this line is needed.
Without that i could exceed the limit.
-Andi
next prev parent reply other threads:[~2014-11-11 23:32 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-26 23:37 Implement lbr-as-callgraph v10 Andi Kleen
2014-09-26 23:37 ` [PATCH 1/8] perf, tools: Support handling complete branch stacks as histograms Andi Kleen
2014-10-20 7:54 ` Jiri Olsa
2014-10-20 10:10 ` Jiri Olsa
2014-10-22 1:03 ` Namhyung Kim
2014-11-11 23:31 ` Andi Kleen [this message]
2014-11-17 6:23 ` Namhyung Kim
2014-11-18 2:08 ` Andi Kleen
2014-09-26 23:37 ` [PATCH 2/8] perf, tools: Add --branch-history option to report Andi Kleen
2014-10-20 7:54 ` Jiri Olsa
2014-10-20 17:51 ` Jiri Olsa
2014-10-22 1:26 ` Namhyung Kim
2014-11-12 0:05 ` Andi Kleen
2014-11-17 6:31 ` Namhyung Kim
2014-11-18 2:01 ` Andi Kleen
2014-09-26 23:37 ` [PATCH 3/8] perf, tools: Enable printing the srcline in the history Andi Kleen
2014-10-20 17:57 ` Jiri Olsa
2014-09-26 23:37 ` [PATCH 4/8] perf, tools: Only print base source file for srcline Andi Kleen
2014-09-26 23:37 ` [PATCH 5/8] perf, tools: Support source line numbers in annotate Andi Kleen
2014-10-20 18:06 ` Jiri Olsa
2014-10-20 18:17 ` Jiri Olsa
2014-11-12 0:36 ` Andi Kleen
2014-09-26 23:37 ` [PATCH 6/8] tools, perf: Make get_srcline fall back to sym+offset Andi Kleen
2014-09-26 23:37 ` [PATCH 7/8] tools, perf: Make srcline output address with -v Andi Kleen
2014-10-20 18:20 ` Jiri Olsa
2014-09-26 23:37 ` [PATCH 8/8] tools, perf: Add asprintf replacement Andi Kleen
2014-10-20 18:28 ` Jiri Olsa
2014-10-20 19:13 ` Geert Uytterhoeven
2014-10-20 19:21 ` Jiri Olsa
2014-10-20 19:33 ` Geert Uytterhoeven
-- strict thread matches above, loose matches on Subject: below --
2014-09-15 23:54 Implement lbr-as-callgraph v9 Andi Kleen
2014-09-15 23:54 ` [PATCH 1/8] perf, tools: Support handling complete branch stacks as histograms Andi Kleen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141111233153.GK12538@two.firstfloor.org \
--to=andi@firstfloor.org \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=namhyung@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).