Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220928072204.1613330-1-irogers@google.com> <20220928072204.1613330-4-irogers@google.com>
 <e685d636-52cd-b34a-2d10-90c16b2e57cf@linux.intel.com>
In-Reply-To: <e685d636-52cd-b34a-2d10-90c16b2e57cf@linux.intel.com>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 3 Oct 2022 18:34:14 -0700
Message-ID: <CAP-5=fXnzHBuNORVCznEi6yhoJXXD6bVaevw6qygsGf8BgmiWA@mail.gmail.com>
Subject: Re: [PATCH v1 03/22] perf metrics: Don't scale counts going into metrics
To:     Xing Zhengjun <zhengjun.xing@linux.intel.com>
Cc:     Kan Liang <kan.liang@linux.intel.com>,
        Andi Kleen <ak@linux.intel.com>, perry.taylor@intel.com,
        caleb.biggers@intel.com, kshipra.bopardikar@intel.com,
        samantha.alt@intel.com, ahmad.yasin@intel.com,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        John Garry <john.garry@huawei.com>,
        James Clark <james.clark@arm.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        Thomas Richter <tmricht@linux.ibm.com>,
        Miaoqian Lin <linmq006@gmail.com>,
        Florian Fischer <florian.fischer@muhq.space>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stephane Eranian <eranian@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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