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

[Ian Rogers]

On Thu, Sep 29, 2022 at 1:49 AM Xing Zhengjun
<zhengjun.xing@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   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