[PATCH 1/2] perf stat: Reset aggr stats for each run

[PATCH 1/2] perf stat: Reset aggr stats for each run

[Namhyung Kim]

When it runs multiple times with -r option, it missed to reset the
aggregation counters and the values were added up.  The aggregation
count has the values to be printed in the end.  It should reset the
counters at the beginning of each run.  But the current code does that
only when -I/--interval-print option is given.

Fixes: 91f85f98da7a ("perf stat: Display event stats using aggr counts")
Reported-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c87c6897edc9..e549862f90f0 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -725,6 +725,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			all_counters_use_bpf = false;
 	}
 
+	evlist__reset_aggr_stats(evsel_list);
+
 	evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
 		counter = evlist_cpu_itr.evsel;
 
-- 
2.41.0.162.gfafddb0af9-goog

[PATCH 2/2] perf stat: Show average value on multiple runs

[Namhyung Kim]

When -r option is used, perf stat runs the command multiple times and
update stats in the evsel->stats.res_stats for global aggregation.  But
the value is never used and the value it prints at the end is just the
value from the last run.  I think we should print the average number of
multiple runs.

Add evlist__copy_res_stats() to update the aggr counter (for display)
using the values in the evsel->stats.res_stats.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c |  5 ++++-
 tools/perf/util/stat.c    | 22 ++++++++++++++++++++++
 tools/perf/util/stat.h    |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e549862f90f0..42f84975a4d5 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2829,8 +2829,11 @@ int cmd_stat(int argc, const char **argv)
 		}
 	}
 
-	if (!forever && status != -1 && (!interval || stat_config.summary))
+	if (!forever && status != -1 && (!interval || stat_config.summary)) {
+		if (stat_config.run_count > 1)
+			evlist__copy_res_stats(&stat_config, evsel_list);
 		print_counters(NULL, argc, argv);
+	}
 
 	evlist__finalize_ctlfd(evsel_list);
 
diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 0f7b8a8cdea6..967e583392c7 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -264,6 +264,28 @@ void evlist__copy_prev_raw_counts(struct evlist *evlist)
 		evsel__copy_prev_raw_counts(evsel);
 }
 
+static void evsel__copy_res_stats(struct evsel *evsel)
+{
+	struct perf_stat_evsel *ps = evsel->stats;
+
+	/*
+	 * For GLOBAL aggregation mode, it updates the counts for each run
+	 * in the evsel->stats.res_stats.  See perf_stat_process_counter().
+	 */
+	*ps->aggr[0].counts.values = avg_stats(&ps->res_stats);
+}
+
+void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist)
+{
+	struct evsel *evsel;
+
+	if (config->aggr_mode != AGGR_GLOBAL)
+		return;
+
+	evlist__for_each_entry(evlist, evsel)
+		evsel__copy_res_stats(evsel);
+}
+
 static size_t pkg_id_hash(long __key, void *ctx __maybe_unused)
 {
 	uint64_t *key = (uint64_t *) __key;
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 7abff7cbb5a1..1cbc26b587ba 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -182,6 +182,7 @@ void evlist__save_aggr_prev_raw_counts(struct evlist *evlist);
 
 int evlist__alloc_aggr_stats(struct evlist *evlist, int nr_aggr);
 void evlist__reset_aggr_stats(struct evlist *evlist);
+void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist);
 
 int perf_stat_process_counter(struct perf_stat_config *config,
 			      struct evsel *counter);
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH 2/2] perf stat: Show average value on multiple runs

[Jiri Olsa]

On Fri, Jun 16, 2023 at 12:32:11AM -0700, Namhyung Kim wrote:
> When -r option is used, perf stat runs the command multiple times and
> update stats in the evsel->stats.res_stats for global aggregation.  But
> the value is never used and the value it prints at the end is just the
> value from the last run.  I think we should print the average number of
> multiple runs.
> 
> Add evlist__copy_res_stats() to update the aggr counter (for display)
> using the values in the evsel->stats.res_stats.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

this is the 'real' fix right? I thought this was the way it worked before

anyway works nicely now, would be nice to add some tests for this,
but not sure how bad it'd be ;-)

Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/builtin-stat.c |  5 ++++-
>  tools/perf/util/stat.c    | 22 ++++++++++++++++++++++
>  tools/perf/util/stat.h    |  1 +
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e549862f90f0..42f84975a4d5 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2829,8 +2829,11 @@ int cmd_stat(int argc, const char **argv)
>  		}
>  	}
>  
> -	if (!forever && status != -1 && (!interval || stat_config.summary))
> +	if (!forever && status != -1 && (!interval || stat_config.summary)) {
> +		if (stat_config.run_count > 1)
> +			evlist__copy_res_stats(&stat_config, evsel_list);
>  		print_counters(NULL, argc, argv);
> +	}
>  
>  	evlist__finalize_ctlfd(evsel_list);
>  
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 0f7b8a8cdea6..967e583392c7 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -264,6 +264,28 @@ void evlist__copy_prev_raw_counts(struct evlist *evlist)
>  		evsel__copy_prev_raw_counts(evsel);
>  }
>  
> +static void evsel__copy_res_stats(struct evsel *evsel)
> +{
> +	struct perf_stat_evsel *ps = evsel->stats;
> +
> +	/*
> +	 * For GLOBAL aggregation mode, it updates the counts for each run
> +	 * in the evsel->stats.res_stats.  See perf_stat_process_counter().
> +	 */
> +	*ps->aggr[0].counts.values = avg_stats(&ps->res_stats);
> +}
> +
> +void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist)
> +{
> +	struct evsel *evsel;
> +
> +	if (config->aggr_mode != AGGR_GLOBAL)
> +		return;
> +
> +	evlist__for_each_entry(evlist, evsel)
> +		evsel__copy_res_stats(evsel);
> +}
> +
>  static size_t pkg_id_hash(long __key, void *ctx __maybe_unused)
>  {
>  	uint64_t *key = (uint64_t *) __key;
> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> index 7abff7cbb5a1..1cbc26b587ba 100644
> --- a/tools/perf/util/stat.h
> +++ b/tools/perf/util/stat.h
> @@ -182,6 +182,7 @@ void evlist__save_aggr_prev_raw_counts(struct evlist *evlist);
>  
>  int evlist__alloc_aggr_stats(struct evlist *evlist, int nr_aggr);
>  void evlist__reset_aggr_stats(struct evlist *evlist);
> +void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist);
>  
>  int perf_stat_process_counter(struct perf_stat_config *config,
>  			      struct evsel *counter);
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

Re: [PATCH 1/2] perf stat: Reset aggr stats for each run

[Jiri Olsa]

On Fri, Jun 16, 2023 at 12:32:10AM -0700, Namhyung Kim wrote:
> When it runs multiple times with -r option, it missed to reset the
> aggregation counters and the values were added up.  The aggregation
> count has the values to be printed in the end.  It should reset the
> counters at the beginning of each run.  But the current code does that
> only when -I/--interval-print option is given.
> 
> Fixes: 91f85f98da7a ("perf stat: Display event stats using aggr counts")
> Reported-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-stat.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index c87c6897edc9..e549862f90f0 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -725,6 +725,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  			all_counters_use_bpf = false;
>  	}
>  
> +	evlist__reset_aggr_stats(evsel_list);
> +

would it be better to call this below before read_counters call,
together with the other counts setup calls?

jirka

>  	evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
>  		counter = evlist_cpu_itr.evsel;
>  
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

Re: [PATCH 1/2] perf stat: Reset aggr stats for each run

[Namhyung Kim]

Hi Jiri,

On Sat, Jun 17, 2023 at 11:45 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 12:32:10AM -0700, Namhyung Kim wrote:
> > When it runs multiple times with -r option, it missed to reset the
> > aggregation counters and the values were added up.  The aggregation
> > count has the values to be printed in the end.  It should reset the
> > counters at the beginning of each run.  But the current code does that
> > only when -I/--interval-print option is given.
> >
> > Fixes: 91f85f98da7a ("perf stat: Display event stats using aggr counts")
> > Reported-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-stat.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index c87c6897edc9..e549862f90f0 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -725,6 +725,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >                       all_counters_use_bpf = false;
> >       }
> >
> > +     evlist__reset_aggr_stats(evsel_list);
> > +
>
> would it be better to call this below before read_counters call,
> together with the other counts setup calls?
>
> jirka

To me, the counter setup for the interval mode and summary is
kinda an exceptional case.

Thanks,
Namhyung

Re: [PATCH 2/2] perf stat: Show average value on multiple runs

[Namhyung Kim]

On Sat, Jun 17, 2023 at 11:45 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 12:32:11AM -0700, Namhyung Kim wrote:
> > When -r option is used, perf stat runs the command multiple times and
> > update stats in the evsel->stats.res_stats for global aggregation.  But
> > the value is never used and the value it prints at the end is just the
> > value from the last run.  I think we should print the average number of
> > multiple runs.
> >
> > Add evlist__copy_res_stats() to update the aggr counter (for display)
> > using the values in the evsel->stats.res_stats.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> this is the 'real' fix right? I thought this was the way it worked before

It worked like the patch 1/2 before.

>
> anyway works nicely now, would be nice to add some tests for this,
> but not sure how bad it'd be ;-)

Maybe we can parse the raw counter output from the -v option and
calculate the average then compare.

>
> Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>

Thanks,
Namhyung


>
> > ---
> >  tools/perf/builtin-stat.c |  5 ++++-
> >  tools/perf/util/stat.c    | 22 ++++++++++++++++++++++
> >  tools/perf/util/stat.h    |  1 +
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index e549862f90f0..42f84975a4d5 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -2829,8 +2829,11 @@ int cmd_stat(int argc, const char **argv)
> >               }
> >       }
> >
> > -     if (!forever && status != -1 && (!interval || stat_config.summary))
> > +     if (!forever && status != -1 && (!interval || stat_config.summary)) {
> > +             if (stat_config.run_count > 1)
> > +                     evlist__copy_res_stats(&stat_config, evsel_list);
> >               print_counters(NULL, argc, argv);
> > +     }
> >
> >       evlist__finalize_ctlfd(evsel_list);
> >
> > diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> > index 0f7b8a8cdea6..967e583392c7 100644
> > --- a/tools/perf/util/stat.c
> > +++ b/tools/perf/util/stat.c
> > @@ -264,6 +264,28 @@ void evlist__copy_prev_raw_counts(struct evlist *evlist)
> >               evsel__copy_prev_raw_counts(evsel);
> >  }
> >
> > +static void evsel__copy_res_stats(struct evsel *evsel)
> > +{
> > +     struct perf_stat_evsel *ps = evsel->stats;
> > +
> > +     /*
> > +      * For GLOBAL aggregation mode, it updates the counts for each run
> > +      * in the evsel->stats.res_stats.  See perf_stat_process_counter().
> > +      */
> > +     *ps->aggr[0].counts.values = avg_stats(&ps->res_stats);
> > +}
> > +
> > +void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist)
> > +{
> > +     struct evsel *evsel;
> > +
> > +     if (config->aggr_mode != AGGR_GLOBAL)
> > +             return;
> > +
> > +     evlist__for_each_entry(evlist, evsel)
> > +             evsel__copy_res_stats(evsel);
> > +}
> > +
> >  static size_t pkg_id_hash(long __key, void *ctx __maybe_unused)
> >  {
> >       uint64_t *key = (uint64_t *) __key;
> > diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> > index 7abff7cbb5a1..1cbc26b587ba 100644
> > --- a/tools/perf/util/stat.h
> > +++ b/tools/perf/util/stat.h
> > @@ -182,6 +182,7 @@ void evlist__save_aggr_prev_raw_counts(struct evlist *evlist);
> >
> >  int evlist__alloc_aggr_stats(struct evlist *evlist, int nr_aggr);
> >  void evlist__reset_aggr_stats(struct evlist *evlist);
> > +void evlist__copy_res_stats(struct perf_stat_config *config, struct evlist *evlist);
> >
> >  int perf_stat_process_counter(struct perf_stat_config *config,
> >                             struct evsel *counter);
> > --
> > 2.41.0.162.gfafddb0af9-goog
> >