Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1479358 > unrolled thread

Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information

Started byArnaldo Carvalho de Melo <acme@kernel.org>
First post2016-09-08 18:20 +0200
Last post2016-09-08 20:20 +0200
Articles 9 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-09-08 18:20 +0200
    Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Peter Zijlstra <peterz@infradead.org> - 2016-09-08 18:50 +0200
      Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Peter Zijlstra <peterz@infradead.org> - 2016-09-08 19:00 +0200
        Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-09-08 19:10 +0200
    Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information Stephane Eranian <eranian@google.com> - 2016-09-08 18:50 +0200
      Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Andi Kleen <andi@firstfloor.org> - 2016-09-08 19:00 +0200
        Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-09-08 19:20 +0200
          RE: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information "Jin, Yao" <yao.jin@intel.com> - 2016-09-09 04:50 +0200
      Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block  information Peter Zijlstra <peterz@infradead.org> - 2016-09-08 20:20 +0200

#1479358 — Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-09-08 18:20 +0200
SubjectRe: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information
Message-ID<sfadr-84F-3@gated-at.bofh.it>
Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> 
> > I've been thinking of filtering all targets and branches that are
> > smaller than 0.1% in order to avoid this, but so far I've just been
> > ignoring these things.
> 
> Like so... seems to 'work'.

So I merged this one with 7/7 and this is the result, screenshot to
capture the colors:

  http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png

Please let me know if I should go ahead and push with the combined
patch, that is now at:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/annotate_basic_blocks

Is that ok?

The problem with it is that it is done only for --stdio, I'll check how
to properly make it UI agnostic...

- Arnaldo

 
> ---
>  tools/perf/util/annotate.c | 45 ++++++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 8eeb151..c78b16f0 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -907,6 +907,7 @@ static void annotate__branch_printf(struct block_range *br, u64 addr)
>  #if 1
>  	if (br->is_target && br->start == addr) {
>  		struct block_range *branch = br;
> +		double p;
>  
>  		/*
>  		 * Find matching branch to our target.
> @@ -914,31 +915,37 @@ static void annotate__branch_printf(struct block_range *br, u64 addr)
>  		while (!branch->is_branch)
>  			branch = block_range__next(branch);
>  
> -		if (emit_comment) {
> -			emit_comment = false;
> -			printf("\t#");
> -		}
> +		p = 100 *(double)br->entry / branch->coverage;
>  
> -		/*
> -		 * The percentage of coverage joined at this target in relation
> -		 * to the next branch.
> -		 */
> -		printf(" +%.2f%%", 100*(double)br->entry / branch->coverage);
> +		if (p > 0.1) {
> +			if (emit_comment) {
> +				emit_comment = false;
> +				printf("\t#");
> +			}
> +
> +			/*
> +			 * The percentage of coverage joined at this target in relation
> +			 * to the next branch.
> +			 */
> +			printf(" +%.2f%%", p);
> +		}
>  	}
>  #endif
>  	if (br->is_branch && br->end == addr) {
> +		double p = 100*(double)br->taken / br->coverage;
>  
> -		if (emit_comment) {
> -			emit_comment = false;
> -			printf("\t#");
> -		}
> +		if (p > 0.1) {
> +			if (emit_comment) {
> +				emit_comment = false;
> +				printf("\t#");
> +			}
>  
> -		/*
> -		 * The percentage of coverage leaving at this branch, and
> -		 * its prediction ratio.
> -		 */
> -		printf(" -%.2f%% / %.2f%%", 100*(double)br->taken / br->coverage,
> -					    100*(double)br->pred  / br->taken);
> +			/*
> +			 * The percentage of coverage leaving at this branch, and
> +			 * its prediction ratio.
> +			 */
> +			printf(" -%.2f%% (p:%.2f%%)", p, 100*(double)br->pred  / br->taken);
> +		}
>  	}
>  }
>

[toc] | [next] | [standalone]

#1479374

FromPeter Zijlstra <peterz@infradead.org>
Date2016-09-08 18:50 +0200
Message-ID<sfaGt-8ej-1@gated-at.bofh.it>
In reply to#1479358
On Thu, Sep 08, 2016 at 01:18:57PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> > On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> > 
> > > I've been thinking of filtering all targets and branches that are
> > > smaller than 0.1% in order to avoid this, but so far I've just been
> > > ignoring these things.
> > 
> > Like so... seems to 'work'.
> 
> So I merged this one with 7/7 and this is the result, screenshot to
> capture the colors:
> 
>   http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png
> 
> Please let me know if I should go ahead and push with the combined
> patch, that is now at:
> 
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/annotate_basic_blocks
> 
> Is that ok?
> 
> The problem with it is that it is done only for --stdio, I'll check how
> to properly make it UI agnostic...

Yep, much thanks!

[toc] | [prev] | [next] | [standalone]

#1479378

FromPeter Zijlstra <peterz@infradead.org>
Date2016-09-08 19:00 +0200
Message-ID<sfaQa-8hz-7@gated-at.bofh.it>
In reply to#1479374
On Thu, Sep 08, 2016 at 06:41:35PM +0200, Peter Zijlstra wrote:
> > Please let me know if I should go ahead and push with the combined
> > patch, that is now at:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5

Uh, you seem to have lost block-range.h.

[toc] | [prev] | [next] | [standalone]

#1479385

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-09-08 19:10 +0200
Message-ID<sfaZP-8c-1@gated-at.bofh.it>
In reply to#1479378
Em Thu, Sep 08, 2016 at 06:51:41PM +0200, Peter Zijlstra escreveu:
> On Thu, Sep 08, 2016 at 06:41:35PM +0200, Peter Zijlstra wrote:
> > > Please let me know if I should go ahead and push with the combined
> > > patch, that is now at:
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5
> 
> Uh, you seem to have lost block-range.h.

Yeah, 'make -C tools/perf build-test' also told me that, I fixed it
already:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=70fbe0574558e934f93bde26e4949c8c206bae43

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1479375 — Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information

FromStephane Eranian <eranian@google.com>
Date2016-09-08 18:50 +0200
SubjectRe: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information
Message-ID<sfaGt-8ej-5@gated-at.bofh.it>
In reply to#1479358
Hi,

On Thu, Sep 8, 2016 at 9:18 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> > On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> >
> > > I've been thinking of filtering all targets and branches that are
> > > smaller than 0.1% in order to avoid this, but so far I've just been
> > > ignoring these things.
> >
> > Like so... seems to 'work'.
>
> So I merged this one with 7/7 and this is the result, screenshot to
> capture the colors:
>
>   http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png
>
> Please let me know if I should go ahead and push with the combined
> patch, that is now at:
>
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5
>
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/annotate_basic_blocks
>
> Is that ok?
>
I like the idea and yes, branch stack can be used for this, but I have
a hard time understanding the colored output.
What is the explanation for the color changes?
How do I interpret the percentages in the comments of the assembly:
-54.50% (p: 42%)
Why not have dedicated columns before the assembly with proper column headers?

As for the command line:

  $ perf record -b my_workload

Will do it right, for both kernel and user by default.

If you want user level only, you can simply do:

  $ perf record -b -e cycles:up my_workload

The branch stack inherit the priv level of the event automatically.

>
> The problem with it is that it is done only for --stdio, I'll check how
> to properly make it UI agnostic...
>
> - Arnaldo
>
>
> > ---
> >  tools/perf/util/annotate.c | 45 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 8eeb151..c78b16f0 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -907,6 +907,7 @@ static void annotate__branch_printf(struct block_range *br, u64 addr)
> >  #if 1
> >       if (br->is_target && br->start == addr) {
> >               struct block_range *branch = br;
> > +             double p;
> >
> >               /*
> >                * Find matching branch to our target.
> > @@ -914,31 +915,37 @@ static void annotate__branch_printf(struct block_range *br, u64 addr)
> >               while (!branch->is_branch)
> >                       branch = block_range__next(branch);
> >
> > -             if (emit_comment) {
> > -                     emit_comment = false;
> > -                     printf("\t#");
> > -             }
> > +             p = 100 *(double)br->entry / branch->coverage;
> >
> > -             /*
> > -              * The percentage of coverage joined at this target in relation
> > -              * to the next branch.
> > -              */
> > -             printf(" +%.2f%%", 100*(double)br->entry / branch->coverage);
> > +             if (p > 0.1) {
> > +                     if (emit_comment) {
> > +                             emit_comment = false;
> > +                             printf("\t#");
> > +                     }
> > +
> > +                     /*
> > +                      * The percentage of coverage joined at this target in relation
> > +                      * to the next branch.
> > +                      */
> > +                     printf(" +%.2f%%", p);
> > +             }
> >       }
> >  #endif
> >       if (br->is_branch && br->end == addr) {
> > +             double p = 100*(double)br->taken / br->coverage;
> >
> > -             if (emit_comment) {
> > -                     emit_comment = false;
> > -                     printf("\t#");
> > -             }
> > +             if (p > 0.1) {
> > +                     if (emit_comment) {
> > +                             emit_comment = false;
> > +                             printf("\t#");
> > +                     }
> >
> > -             /*
> > -              * The percentage of coverage leaving at this branch, and
> > -              * its prediction ratio.
> > -              */
> > -             printf(" -%.2f%% / %.2f%%", 100*(double)br->taken / br->coverage,
> > -                                         100*(double)br->pred  / br->taken);
> > +                     /*
> > +                      * The percentage of coverage leaving at this branch, and
> > +                      * its prediction ratio.
> > +                      */
> > +                     printf(" -%.2f%% (p:%.2f%%)", p, 100*(double)br->pred  / br->taken);
> > +             }
> >       }
> >  }
> >

[toc] | [prev] | [next] | [standalone]

#1479382

FromAndi Kleen <andi@firstfloor.org>
Date2016-09-08 19:00 +0200
Message-ID<sfaQa-8hz-15@gated-at.bofh.it>
In reply to#1479375
On Thu, Sep 08, 2016 at 09:43:53AM -0700, Stephane Eranian wrote:
> Hi,
> 
> On Thu, Sep 8, 2016 at 9:18 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> > > On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> > >
> > > > I've been thinking of filtering all targets and branches that are
> > > > smaller than 0.1% in order to avoid this, but so far I've just been
> > > > ignoring these things.
> > >
> > > Like so... seems to 'work'.
> >
> > So I merged this one with 7/7 and this is the result, screenshot to
> > capture the colors:
> >
> >   http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png
> >
> > Please let me know if I should go ahead and push with the combined
> > patch, that is now at:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/annotate_basic_blocks
> >
> > Is that ok?
> >
> I like the idea and yes, branch stack can be used for this, but I have
> a hard time understanding the colored output.
> What is the explanation for the color changes?
> How do I interpret the percentages in the comments of the assembly:
> -54.50% (p: 42%)
> Why not have dedicated columns before the assembly with proper column headers?

Yes columns with headers are better. Jin Yao has been looking at this and already has
some patches.

-Andi

[toc] | [prev] | [next] | [standalone]

#1479391

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-09-08 19:20 +0200
Message-ID<sfb9v-bn-5@gated-at.bofh.it>
In reply to#1479382
Em Thu, Sep 08, 2016 at 09:59:15AM -0700, Andi Kleen escreveu:
> On Thu, Sep 08, 2016 at 09:43:53AM -0700, Stephane Eranian wrote:
> > Hi,
> > 
> > On Thu, Sep 8, 2016 at 9:18 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> > > > On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> > > >
> > > > > I've been thinking of filtering all targets and branches that are
> > > > > smaller than 0.1% in order to avoid this, but so far I've just been
> > > > > ignoring these things.
> > > >
> > > > Like so... seems to 'work'.
> > >
> > > So I merged this one with 7/7 and this is the result, screenshot to
> > > capture the colors:
> > >
> > >   http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png
> > >
> > > Please let me know if I should go ahead and push with the combined
> > > patch, that is now at:
> > >
> > > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858d5cee1e5
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/annotate_basic_blocks
> > >
> > > Is that ok?
> > >
> > I like the idea and yes, branch stack can be used for this, but I have
> > a hard time understanding the colored output.
> > What is the explanation for the color changes?
> > How do I interpret the percentages in the comments of the assembly:
> > -54.50% (p: 42%)
> > Why not have dedicated columns before the assembly with proper column headers?
> 
> Yes columns with headers are better. Jin Yao has been looking at this and already has
> some patches.

For --tui as well?

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1479623

From"Jin, Yao" <yao.jin@intel.com>
Date2016-09-09 04:50 +0200
Message-ID<sfk37-5BV-5@gated-at.bofh.it>
In reply to#1479391
Hi, 

The idea of my patch is a little bit different.  It targets to associate the branch
coverage percentage and branch mispredict rate with the source code then user
can see them directly in perf annotate source code / assembly view.

The screenshot to show the branch%. 
https://github.com/yaoj/perf/blob/master/2.png

The screenshot to show the mispredict rate.
https://github.com/yaoj/perf/blob/master/3.png

The --stdio mode is tested working well now and will do for --tui mode in next.

The internal review comments require the patch to keep the existing "Percent"
column unchanged and add new columns "Branch%" and "Mispred%" if LBRs
are recorded in perf.data. For example, following 3 columns will be shown at
default if LBRs are in perf.data when executing perf annotate --stdio.

Percent | Branch% | Mispred% |

With this comment, the patch needs to be changed. 

Please let me know what do you think for this patch and  if I should go ahead. 

Thanks
Jin Yao

-----Original Message-----
From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org] 
Sent: Friday, September 9, 2016 1:11 AM
To: Andi Kleen <andi@firstfloor.org>
Cc: Stephane Eranian <eranian@google.com>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@kernel.org>; LKML <linux-kernel@vger.kernel.org>; Jiri Olsa <jolsa@kernel.org>; Linus Torvalds <torvalds@linux-foundation.org>; David Carrillo-Cisneros <davidcc@google.com>; Alexander Shishkin <alexander.shishkin@linux.intel.com>; Namhyung Kim <namhyung@kernel.org>; Liang, Kan <kan.liang@intel.com>; Anshuman Khandual <khandual@linux.vnet.ibm.com>; Jin, Yao <yao.jin@intel.com>
Subject: Re: [RFC][PATCH 7/7] perf/annotate: Add branch stack / basic block information

Em Thu, Sep 08, 2016 at 09:59:15AM -0700, Andi Kleen escreveu:
> On Thu, Sep 08, 2016 at 09:43:53AM -0700, Stephane Eranian wrote:
> > Hi,
> > 
> > On Thu, Sep 8, 2016 at 9:18 AM, Arnaldo Carvalho de Melo 
> > <acme@kernel.org> wrote:
> > >
> > > Em Fri, Jul 08, 2016 at 06:36:32PM +0200, Peter Zijlstra escreveu:
> > > > On Fri, Jul 08, 2016 at 06:27:33PM +0200, Peter Zijlstra wrote:
> > > >
> > > > > I've been thinking of filtering all targets and branches that 
> > > > > are smaller than 0.1% in order to avoid this, but so far I've 
> > > > > just been ignoring these things.
> > > >
> > > > Like so... seems to 'work'.
> > >
> > > So I merged this one with 7/7 and this is the result, screenshot 
> > > to capture the colors:
> > >
> > >   http://vger.kernel.org/~acme/perf/annotate_basic_blocks.png
> > >
> > > Please let me know if I should go ahead and push with the combined 
> > > patch, that is now at:
> > >
> > > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit
> > > /?h=perf/annotate_basic_blocks&id=baf41a43fa439ac534d21e41882a7858
> > > d5cee1e5
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git 
> > > perf/annotate_basic_blocks
> > >
> > > Is that ok?
> > >
> > I like the idea and yes, branch stack can be used for this, but I 
> > have a hard time understanding the colored output.
> > What is the explanation for the color changes?
> > How do I interpret the percentages in the comments of the assembly:
> > -54.50% (p: 42%)
> > Why not have dedicated columns before the assembly with proper column headers?
> 
> Yes columns with headers are better. Jin Yao has been looking at this 
> and already has some patches.

For --tui as well?

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1479423

FromPeter Zijlstra <peterz@infradead.org>
Date2016-09-08 20:20 +0200
Message-ID<sfc5A-OX-5@gated-at.bofh.it>
In reply to#1479375
On Thu, Sep 08, 2016 at 09:43:53AM -0700, Stephane Eranian wrote:

> I like the idea and yes, branch stack can be used for this, but I have
> a hard time understanding the colored output.

> What is the explanation for the color changes?

In general, or the changes acme made? I can only answer the first.

For code: NORMAL <1%, BLUE otherwise, quickly shows you the code in a
function that's not ran at all. This quickly eliminated a big chunk of
the function I was looking at at the time, since the benchmark in
question simply didn't touch most of it.

For address: NORMAL <1%, RED > 75%, MAGENTA otherwise. Quickly shows the
hottest blocks in a function. The 75% is a random number otherwise. Not
sure if we can do better.

> How do I interpret the percentages in the comments of the assembly:
> -54.50% (p: 42%)

-54.50% is 54.40% of the coverage is leaving here, aka 54.40% take this
branch. p: 42% mean the branch is predicted 42% of the time.

Similarly, +50.46% is a branch target and means that of all the times
this instruction gets executed, 50.46% of those joined at this
instruction.

> Why not have dedicated columns before the assembly with proper column headers?

I found it too noisy, you only want to annotate branch instructions and
branch targets. Adding columns just adds a whole heap of whitespace
(wasted screen-estate) on the left.

Something I did want to look at was attempting to align the # comments,
but I never did bother.

But to each their own I suppose.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web