[PATCH v1 03/22] perf metrics: Don't scale counts going into metrics

Ian Rogers posted 22 patches 3 years, 6 months ago
There is a newer version of this series
[PATCH v1 03/22] perf metrics: Don't scale counts going into metrics
Posted by Ian Rogers 3 years, 6 months ago
Counts are scaled prior to going into saved_value, reverse the scaling
so that metrics don't double scale values.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-shadow.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 9e1eddeff21b..b5cedd37588f 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -865,11 +865,16 @@ static int prepare_metric(struct evsel **metric_events,
 			if (!v)
 				break;
 			stats = &v->stats;
-			scale = 1.0;
+			/*
+			 * If an event was scaled during stat gathering, reverse
+			 * the scale before computing the metric.
+			 */
+			scale = 1.0 / metric_events[i]->scale;
+
 			source_count = evsel__source_count(metric_events[i]);
 
 			if (v->metric_other)
-				metric_total = v->metric_total;
+				metric_total = v->metric_total * scale;
 		}
 		n = strdup(evsel__metric_id(metric_events[i]));
 		if (!n)
-- 
2.37.3.998.g577e59143f-goog
Re: [PATCH v1 03/22] perf metrics: Don't scale counts going into metrics
Posted by Xing Zhengjun 3 years, 6 months ago

On 9/28/2022 3:21 PM, Ian Rogers wrote:
> Counts are scaled prior to going into saved_value, reverse the scaling
> so that metrics don't double scale values.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/stat-shadow.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 9e1eddeff21b..b5cedd37588f 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -865,11 +865,16 @@ static int prepare_metric(struct evsel **metric_events,
>   			if (!v)
>   				break;
>   			stats = &v->stats;
> -			scale = 1.0;
> +			/*
> +			 * If an event was scaled during stat gathering, reverse
> +			 * the scale before computing the metric.
> +			 */
> +			scale = 1.0 / metric_events[i]->scale;
> +
This look likes not work for kernel side events like 
/sys/devices/uncore_imc_*/events/cas_count_read(write).

>   			source_count = evsel__source_count(metric_events[i]);
>   
>   			if (v->metric_other)
> -				metric_total = v->metric_total;
> +				metric_total = v->metric_total * scale;
>   		}
>   		n = strdup(evsel__metric_id(metric_events[i]));
>   		if (!n)

-- 
Zhengjun Xing
Re: [PATCH v1 03/22] perf metrics: Don't scale counts going into metrics
Posted by Ian Rogers 3 years, 6 months ago
On Thu, Sep 29, 2022 at 1:49 AM Xing Zhengjun
<zhengjun.xing@linux.intel.com> wrote:
>
> On 9/28/2022 3:21 PM, Ian Rogers wrote:
> > Counts are scaled prior to going into saved_value, reverse the scaling
> > so that metrics don't double scale values.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/util/stat-shadow.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index 9e1eddeff21b..b5cedd37588f 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -865,11 +865,16 @@ static int prepare_metric(struct evsel **metric_events,
> >                       if (!v)
> >                               break;
> >                       stats = &v->stats;
> > -                     scale = 1.0;
> > +                     /*
> > +                      * If an event was scaled during stat gathering, reverse
> > +                      * the scale before computing the metric.
> > +                      */
> > +                     scale = 1.0 / metric_events[i]->scale;
> > +
> This look likes not work for kernel side events like
> /sys/devices/uncore_imc_*/events/cas_count_read(write).

I've not been able to reproduce this. I've tried:

  {
    "MetricName": "IanTest",
    "MetricExpr": "uncore_imc@cas_count_read@ * 6.103515625e-5 +
uncore_imc@cas_count_write@ * 6.103515625e-5"
  }

where it scales the two counters and sums them. I see:

$ perf stat -M IanTest -a sleep 1

Performance counter stats for 'system wide':

           298.70 MiB  uncore_imc/cas_count_write/      #   721.00
IanTest
           429.64 MiB  uncore_imc/cas_count_read/

      1.004877710 seconds time elapsed

So the metric's value nearly matches the sum. If the scaled values had
been used the sum would have been something like 0.044454345703124995.

From the code, this is the only place a metric's "ID" (event/counter)
is associated with a value, so I would be confused if there were a
code path where reversing the scaling wasn't happening.

Thanks,
Ian

> >                       source_count = evsel__source_count(metric_events[i]);
> >
> >                       if (v->metric_other)
> > -                             metric_total = v->metric_total;
> > +                             metric_total = v->metric_total * scale;
> >               }
> >               n = strdup(evsel__metric_id(metric_events[i]));
> >               if (!n)
>
> --
> Zhengjun Xing