[PATCH 1/3] perf report: Show error message when processing sample fails

[PATCH 1/3] perf report: Show error message when processing sample fails

[Namhyung Kim]

Currently when perf fails to process samples for some reason, it doesn't
show any message about the failure.  This is very inconvenient for users
especially on TUI as screen is reset after the failure.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 8a9c6908f54e..af5db885ea9c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -513,20 +513,26 @@ static int __cmd_report(struct report *rep)
 	if (rep->cpu_list) {
 		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
 					       rep->cpu_bitmap);
-		if (ret)
+		if (ret) {
+			ui__error("failed to set cpu bitmap\n");
 			return ret;
+		}
 	}
 
 	if (rep->show_threads)
 		perf_read_values_init(&rep->show_threads_values);
 
 	ret = report__setup_sample_type(rep);
-	if (ret)
+	if (ret) {
+		/* report__setup_sample_type() already showed error message */
 		return ret;
+	}
 
 	ret = perf_session__process_events(session);
-	if (ret)
+	if (ret) {
+		ui__error("failed to process sample\n");
 		return ret;
+	}
 
 	report__warn_kptr_restrict(rep);
 
-- 
2.6.2

[PATCH 2/3] perf hist: Do not skip elided fields when processing samples

[Namhyung Kim]

If user gives a filter, perf marks the corresponding column elided and
omits the output.  But it should process and aggregates samples using
the field, otherwise samples will be aggregated as if the column was not
there resulted in incorrect output.

For example, I'd like to set a filter on native_write_msr_safe.  The
original overhead of the function is negligible.

  $ perf report | grep native_write_msr_safe
      0.00%  swapper  [kernel.vmlinux]  native_write_msr_safe
      0.00%  perf     [kernel.vmlinux]  native_write_msr_safe

However adding -S option gives different output.

  $ perf report -S native_write_msr_safe --percentage absolute | \
  > grep -e swapper -e perf
     51.47%  swapper  [kernel.vmlinux]
      4.14%  perf     [kernel.vmlinux]

Since it aggregated samples using comm and dso only.  In fact, the above
values are same when it sorts with -s comm,dso.

  $ perf report -s comm,dso | grep -e swapper -e perf
     51.47%  swapper  [kernel.vmlinux]
      4.14%  perf     [kernel.vmlinux]

This resulted in TUI failure with -ERANGE since it tries to increase
sample hit count for annotation with wrong symbols due to incorrect
aggregation.

This patch fixes it not to skip elided fields when comparing samples
in order to insert them to the hists.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 4fd37d6708cb..6e8e0ee9ec37 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -924,9 +924,6 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
-		if (perf_hpp__should_skip(fmt))
-			continue;
-
 		cmp = fmt->cmp(fmt, left, right);
 		if (cmp)
 			break;
@@ -942,9 +939,6 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
-		if (perf_hpp__should_skip(fmt))
-			continue;
-
 		cmp = fmt->collapse(fmt, left, right);
 		if (cmp)
 			break;
-- 
2.6.2

Re: [PATCH 2/3] perf hist: Do not skip elided fields when processing samples

[Arnaldo Carvalho de Melo]

Em Sat, Nov 28, 2015 at 02:32:38AM +0900, Namhyung Kim escreveu:
> If user gives a filter, perf marks the corresponding column elided and
> omits the output.  But it should process and aggregates samples using
> the field, otherwise samples will be aggregated as if the column was not
> there resulted in incorrect output.

Thanks, applied, I've tested it and it now seems to work in both the
--stdio and --tui interfaces, good job!

- Arnaldo

[tip:perf/core] perf hists: Do not skip elided fields when processing samples

[tip-bot for Namhyung Kim]

Commit-ID:  e72655d97d24fff559b4ab59de791c3741a74c8c
Gitweb:     http://git.kernel.org/tip/e72655d97d24fff559b4ab59de791c3741a74c8c
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sat, 28 Nov 2015 02:32:38 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 27 Nov 2015 21:42:13 -0300

perf hists: Do not skip elided fields when processing samples

If user gives a filter, perf marks the corresponding column elided and
omits the output.  But it should process and aggregates samples using
the field, otherwise samples will be aggregated as if the column was not
there resulted in incorrect output.

For example, I'd like to set a filter on native_write_msr_safe.  The
original overhead of the function is negligible.

  $ perf report | grep native_write_msr_safe
      0.00%  swapper  [kernel.vmlinux]  native_write_msr_safe
      0.00%  perf     [kernel.vmlinux]  native_write_msr_safe

However adding -S option gives different output.

  $ perf report -S native_write_msr_safe --percentage absolute | \
  > grep -e swapper -e perf
     51.47%  swapper  [kernel.vmlinux]
      4.14%  perf     [kernel.vmlinux]

Since it aggregated samples using comm and dso only.  In fact, the above
values are same when it sorts with -s comm,dso.

  $ perf report -s comm,dso | grep -e swapper -e perf
     51.47%  swapper  [kernel.vmlinux]
      4.14%  perf     [kernel.vmlinux]

This resulted in TUI failure with -ERANGE since it tries to increase
sample hit count for annotation with wrong symbols due to incorrect
aggregation.

This patch fixes it not to skip elided fields when comparing samples in
order to insert them to the hists.

Commiter note:

After the patch, with a different workloads:

  # perf report --show-total-period -S native_write_msr_safe --stdio
  #
  # symbol: native_write_msr_safe
  #
  # Samples: 455  of event 'cycles:pp'
  # Event count (approx.): 134787489
  #
  # Overhead Period Command         Shared Object
  # ........ ...... ............... ................
  #
       0.22% 293081 qemu-system-x86 [vmlinux]
       0.19% 255914 swapper         [vmlinux]
       0.00%   2054 Timer           [vmlinux]
       0.00%   1021 firefox         [vmlinux]
       0.00%      2 perf            [vmlinux]

  # perf report --show-total-period | grep native_write_msr_safe
  Failed to open /tmp/perf-14838.map, continuing without symbols
       0.22% 293081 qemu-system-x86 [vmlinux]  [k] native_write_msr_safe
       0.19% 255914 swapper         [vmlinux]  [k] native_write_msr_safe
       0.00%   2054 Timer           [vmlinux]  [k] native_write_msr_safe
       0.00%   1021 firefox         [vmlinux]  [k] native_write_msr_safe
       0.00%      2 perf            [vmlinux]  [k] native_write_msr_safe
  #

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1448645559-31167-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 4fd37d6..6e8e0ee 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -924,9 +924,6 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
-		if (perf_hpp__should_skip(fmt))
-			continue;
-
 		cmp = fmt->cmp(fmt, left, right);
 		if (cmp)
 			break;
@@ -942,9 +939,6 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
-		if (perf_hpp__should_skip(fmt))
-			continue;
-
 		cmp = fmt->collapse(fmt, left, right);
 		if (cmp)
 			break;

[PATCH 3/3] perf hists browser: Update nr entries regardless of min percent

[Namhyung Kim]

When perf report on TUI was called with -S symbol filter, it should
update nr entries even if min_pcnt is 0.  IIRC the reason was to update
nr entries after applying minimum percent threshold.  But if symbol
filter was given on command line (with -S option), it should use
hists->nr_non_filtered_entries instead of hists->nr_entries.

So this patch fixes a bug of navigating hists browser that the cursor
goes beyond the number of entries when -S (or similar) option is used.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a211b7b6a81e..dcdcbafb078b 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2055,10 +2055,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	SLang_reset_tty();
 	SLang_init_tty(0, 0, 0);
 
-	if (min_pcnt) {
+	if (min_pcnt)
 		browser->min_pcnt = min_pcnt;
-		hist_browser__update_nr_entries(browser);
-	}
+	hist_browser__update_nr_entries(browser);
 
 	browser->pstack = pstack__new(3);
 	if (browser->pstack == NULL)
-- 
2.6.2

Re: [PATCH 3/3] perf hists browser: Update nr entries regardless of min percent

[Arnaldo Carvalho de Melo]

Em Sat, Nov 28, 2015 at 02:32:39AM +0900, Namhyung Kim escreveu:
> When perf report on TUI was called with -S symbol filter, it should
> update nr entries even if min_pcnt is 0.  IIRC the reason was to update
> nr entries after applying minimum percent threshold.  But if symbol
> filter was given on command line (with -S option), it should use
> hists->nr_non_filtered_entries instead of hists->nr_entries.
> 
> So this patch fixes a bug of navigating hists browser that the cursor
> goes beyond the number of entries when -S (or similar) option is used.

Thanks, tested and applied.

- Arnaldo

[tip:perf/core] perf hists browser: Update nr entries regardless of min percent

[tip-bot for Namhyung Kim]

Commit-ID:  039050482573e168690d365b8ea1d4f599ebbbd8
Gitweb:     http://git.kernel.org/tip/039050482573e168690d365b8ea1d4f599ebbbd8
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sat, 28 Nov 2015 02:32:39 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 27 Nov 2015 21:53:33 -0300

perf hists browser: Update nr entries regardless of min percent

When perf report on TUI was called with -S symbol filter, it should
update nr entries even if min_pcnt is 0.  IIRC the reason was to update
nr entries after applying minimum percent threshold.  But if symbol
filter was given on command line (with -S option), it should use
hists->nr_non_filtered_entries instead of hists->nr_entries.

So this patch fixes a bug of navigating hists browser that the cursor
goes beyond the number of entries when -S (or similar) option is used.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1448645559-31167-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a211b7b..dcdcbaf 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2055,10 +2055,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	SLang_reset_tty();
 	SLang_init_tty(0, 0, 0);
 
-	if (min_pcnt) {
+	if (min_pcnt)
 		browser->min_pcnt = min_pcnt;
-		hist_browser__update_nr_entries(browser);
-	}
+	hist_browser__update_nr_entries(browser);
 
 	browser->pstack = pstack__new(3);
 	if (browser->pstack == NULL)

Re: [PATCH 1/3] perf report: Show error message when processing sample fails

[Arnaldo Carvalho de Melo]

Em Sat, Nov 28, 2015 at 02:32:37AM +0900, Namhyung Kim escreveu:
> Currently when perf fails to process samples for some reason, it doesn't
> show any message about the failure.  This is very inconvenient for users
> especially on TUI as screen is reset after the failure.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-report.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 8a9c6908f54e..af5db885ea9c 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -513,20 +513,26 @@ static int __cmd_report(struct report *rep)
>  	if (rep->cpu_list) {
>  		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
>  					       rep->cpu_bitmap);
> -		if (ret)
> +		if (ret) {
> +			ui__error("failed to set cpu bitmap\n");

Why? See below.

>  			return ret;
> +		}
>  	}
>  
>  	if (rep->show_threads)
>  		perf_read_values_init(&rep->show_threads_values);
>  
>  	ret = report__setup_sample_type(rep);
> -	if (ret)
> +	if (ret) {
> +		/* report__setup_sample_type() already showed error message */
>  		return ret;
> +	}
>  
>  	ret = perf_session__process_events(session);
> -	if (ret)
> +	if (ret) {
> +		ui__error("failed to process sample\n");
>  		return ret;
> +	}

I'm applying this because it removes the potential flash, but would be
good to have the reason for this failure shown to the user.

- Arnaldo

Re: [PATCH 1/3] perf report: Show error message when processing sample fails

[Arnaldo Carvalho de Melo]

Em Fri, Nov 27, 2015 at 04:56:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, Nov 28, 2015 at 02:32:37AM +0900, Namhyung Kim escreveu:
> > Currently when perf fails to process samples for some reason, it doesn't
> > show any message about the failure.  This is very inconvenient for users
> > especially on TUI as screen is reset after the failure.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Ah, I added a :

Reported-by: Ingo Molnar <mingo@kernel.org>

As this was due to those messages from Ingo, right?

- Arnaldo

> > ---
> >  tools/perf/builtin-report.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 8a9c6908f54e..af5db885ea9c 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -513,20 +513,26 @@ static int __cmd_report(struct report *rep)
> >  	if (rep->cpu_list) {
> >  		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
> >  					       rep->cpu_bitmap);
> > -		if (ret)
> > +		if (ret) {
> > +			ui__error("failed to set cpu bitmap\n");
> 
> Why? See below.
> 
> >  			return ret;
> > +		}
> >  	}
> >  
> >  	if (rep->show_threads)
> >  		perf_read_values_init(&rep->show_threads_values);
> >  
> >  	ret = report__setup_sample_type(rep);
> > -	if (ret)
> > +	if (ret) {
> > +		/* report__setup_sample_type() already showed error message */
> >  		return ret;
> > +	}
> >  
> >  	ret = perf_session__process_events(session);
> > -	if (ret)
> > +	if (ret) {
> > +		ui__error("failed to process sample\n");
> >  		return ret;
> > +	}
> 
> I'm applying this because it removes the potential flash, but would be
> good to have the reason for this failure shown to the user.
> 
> - Arnaldo

Re: [PATCH 1/3] perf report: Show error message when processing sample fails

[Namhyung Kim]

Hi Arnaldo,

On Fri, Nov 27, 2015 at 04:57:21PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 27, 2015 at 04:56:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Sat, Nov 28, 2015 at 02:32:37AM +0900, Namhyung Kim escreveu:
> > > Currently when perf fails to process samples for some reason, it doesn't
> > > show any message about the failure.  This is very inconvenient for users
> > > especially on TUI as screen is reset after the failure.
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Ah, I added a :
> 
> Reported-by: Ingo Molnar <mingo@kernel.org>
> 
> As this was due to those messages from Ingo, right?

Right.


> 
> > > ---
> > >  tools/perf/builtin-report.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > index 8a9c6908f54e..af5db885ea9c 100644
> > > --- a/tools/perf/builtin-report.c
> > > +++ b/tools/perf/builtin-report.c
> > > @@ -513,20 +513,26 @@ static int __cmd_report(struct report *rep)
> > >  	if (rep->cpu_list) {
> > >  		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
> > >  					       rep->cpu_bitmap);
> > > -		if (ret)
> > > +		if (ret) {
> > > +			ui__error("failed to set cpu bitmap\n");
> > 
> > Why? See below.
> > 
> > >  			return ret;
> > > +		}
> > >  	}
> > >  
> > >  	if (rep->show_threads)
> > >  		perf_read_values_init(&rep->show_threads_values);
> > >  
> > >  	ret = report__setup_sample_type(rep);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		/* report__setup_sample_type() already showed error message */
> > >  		return ret;
> > > +	}
> > >  
> > >  	ret = perf_session__process_events(session);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		ui__error("failed to process sample\n");
> > >  		return ret;
> > > +	}
> > 
> > I'm applying this because it removes the potential flash, but would be
> > good to have the reason for this failure shown to the user.

Yes, but at least the last error/warning will be shown in the bottom.

Thanks,
Namhyung

[tip:perf/core] perf report: Show error message when processing sample fails

[tip-bot for Namhyung Kim]

Commit-ID:  25b1606be1a910a63a23c3d1006581c9aad4e6e3
Gitweb:     http://git.kernel.org/tip/25b1606be1a910a63a23c3d1006581c9aad4e6e3
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sat, 28 Nov 2015 02:32:37 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 27 Nov 2015 16:56:32 -0300

perf report: Show error message when processing sample fails

Currently when perf fails to process samples for some reason, it doesn't
show any message about the failure.  This is very inconvenient for users
especially on TUI as screen is reset after the failure.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1448645559-31167-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 8a9c690..af5db88 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -513,20 +513,26 @@ static int __cmd_report(struct report *rep)
 	if (rep->cpu_list) {
 		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
 					       rep->cpu_bitmap);
-		if (ret)
+		if (ret) {
+			ui__error("failed to set cpu bitmap\n");
 			return ret;
+		}
 	}
 
 	if (rep->show_threads)
 		perf_read_values_init(&rep->show_threads_values);
 
 	ret = report__setup_sample_type(rep);
-	if (ret)
+	if (ret) {
+		/* report__setup_sample_type() already showed error message */
 		return ret;
+	}
 
 	ret = perf_session__process_events(session);
-	if (ret)
+	if (ret) {
+		ui__error("failed to process sample\n");
 		return ret;
+	}
 
 	report__warn_kptr_restrict(rep);