diff mbox

[Xenial,SRU] perf stat: Fix interval output values

Message ID 1477668078-1904-1-git-send-email-tim.gardner@canonical.com
State New
Headers show

Commit Message

Tim Gardner Oct. 28, 2016, 3:21 p.m. UTC
From: Jiri Olsa <jolsa@kernel.org>

BugLink: http://bugs.launchpad.net/bugs/1636647

We broke interval data displays with commit:

  3f416f22d1e2 ("perf stat: Do not clean event's private stats")

This commit removed stats cleaning, which is important for '-r' option
to carry counters data over the whole run. But it's necessary to clean
it for interval mode, otherwise the displayed value is avg of all
previous values.

Before:
  $ perf stat -e cycles -a -I 1000 record
  #           time             counts unit events
       1.000240796         75,216,287      cycles
       2.000512791        107,823,524      cycles

  $ perf stat report
  #           time             counts unit events
       1.000240796         75,216,287      cycles
       2.000512791         91,519,906      cycles

Now:
  $ perf stat report
  #           time             counts unit events
       1.000240796         75,216,287      cycles
       2.000512791        107,823,524      cycles

Notice the second value being bigger (91,.. < 107,..).

This could be easily verified by using perf script which displays raw
stat data:

  $ perf script
  CPU  THREAD       VAL         ENA         RUN        TIME EVENT
    0      -1  23855779  1000209530  1000209530  1000240796 cycles
    1      -1  33340397  1000224964  1000224964  1000240796 cycles
    2      -1  15835415  1000226695  1000226695  1000240796 cycles
    3      -1   2184696  1000228245  1000228245  1000240796 cycles
    0      -1  97014312  2000514533  2000514533  2000512791 cycles
    1      -1  46121497  2000543795  2000543795  2000512791 cycles
    2      -1  32269530  2000543566  2000543566  2000512791 cycles
    3      -1   7634472  2000544108  2000544108  2000512791 cycles

The sum of the first 4 values is the first interval aggregated value:

  23855779 + 33340397 + 15835415 + 2184696 = 75,216,287

The sum of the second 4 values minus first value is the second interval
aggregated value:

  97014312 + 46121497 + 32269530 + 7634472 - 75216287 = 107,823,524

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1454485436-20639-1-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
(cherry picked from commit 51fd2df1e882a3c2a3f4b6c9ff243a93c9046dba)
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 tools/perf/util/stat.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Stefan Bader Nov. 4, 2016, 1:20 p.m. UTC | #1

Seth Forshee Nov. 7, 2016, 8:26 p.m. UTC | #2

Luis Henriques Nov. 8, 2016, 3:32 p.m. UTC | #3
On Fri, Oct 28, 2016 at 09:21:18AM -0600, Tim Gardner wrote:
> From: Jiri Olsa <jolsa@kernel.org>
> 
> BugLink: http://bugs.launchpad.net/bugs/1636647
>

This commit is already applied to xenial; it came in through an upstream
stable update.

Cheers,
--
Luís


> We broke interval data displays with commit:
> 
>   3f416f22d1e2 ("perf stat: Do not clean event's private stats")
> 
> This commit removed stats cleaning, which is important for '-r' option
> to carry counters data over the whole run. But it's necessary to clean
> it for interval mode, otherwise the displayed value is avg of all
> previous values.
> 
> Before:
>   $ perf stat -e cycles -a -I 1000 record
>   #           time             counts unit events
>        1.000240796         75,216,287      cycles
>        2.000512791        107,823,524      cycles
> 
>   $ perf stat report
>   #           time             counts unit events
>        1.000240796         75,216,287      cycles
>        2.000512791         91,519,906      cycles
> 
> Now:
>   $ perf stat report
>   #           time             counts unit events
>        1.000240796         75,216,287      cycles
>        2.000512791        107,823,524      cycles
> 
> Notice the second value being bigger (91,.. < 107,..).
> 
> This could be easily verified by using perf script which displays raw
> stat data:
> 
>   $ perf script
>   CPU  THREAD       VAL         ENA         RUN        TIME EVENT
>     0      -1  23855779  1000209530  1000209530  1000240796 cycles
>     1      -1  33340397  1000224964  1000224964  1000240796 cycles
>     2      -1  15835415  1000226695  1000226695  1000240796 cycles
>     3      -1   2184696  1000228245  1000228245  1000240796 cycles
>     0      -1  97014312  2000514533  2000514533  2000512791 cycles
>     1      -1  46121497  2000543795  2000543795  2000512791 cycles
>     2      -1  32269530  2000543566  2000543566  2000512791 cycles
>     3      -1   7634472  2000544108  2000544108  2000512791 cycles
> 
> The sum of the first 4 values is the first interval aggregated value:
> 
>   23855779 + 33340397 + 15835415 + 2184696 = 75,216,287
> 
> The sum of the second 4 values minus first value is the second interval
> aggregated value:
> 
>   97014312 + 46121497 + 32269530 + 7634472 - 75216287 = 107,823,524
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Stephane Eranian <eranian@google.com>
> Link: http://lkml.kernel.org/r/1454485436-20639-1-git-send-email-jolsa@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> (cherry picked from commit 51fd2df1e882a3c2a3f4b6c9ff243a93c9046dba)
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  tools/perf/util/stat.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 4a3a72c..6ce624c 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -311,6 +311,16 @@ int perf_stat_process_counter(struct perf_stat_config *config,
>  
>  	aggr->val = aggr->ena = aggr->run = 0;
>  
> +	/*
> +	 * We calculate counter's data every interval,
> +	 * and the display code shows ps->res_stats
> +	 * avg value. We need to zero the stats for
> +	 * interval mode, otherwise overall avg running
> +	 * averages will be shown for each interval.
> +	 */
> +	if (config->interval)
> +		init_stats(ps->res_stats);
> +
>  	if (counter->per_pkg)
>  		zero_per_pkg(counter);
>  
> -- 
> 2.7.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox

Patch

diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 4a3a72c..6ce624c 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -311,6 +311,16 @@  int perf_stat_process_counter(struct perf_stat_config *config,
 
 	aggr->val = aggr->ena = aggr->run = 0;
 
+	/*
+	 * We calculate counter's data every interval,
+	 * and the display code shows ps->res_stats
+	 * avg value. We need to zero the stats for
+	 * interval mode, otherwise overall avg running
+	 * averages will be shown for each interval.
+	 */
+	if (config->interval)
+		init_stats(ps->res_stats);
+
 	if (counter->per_pkg)
 		zero_per_pkg(counter);