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: <1672745976-2800146-1-git-send-email-renyu.zj@linux.alibaba.com>
 <1672745976-2800146-3-git-send-email-renyu.zj@linux.alibaba.com>
 <CAP-5=fVP_=FcEQChQwVpis9iyZsdb6NrVJQUo4GJA4_j=knfcA@mail.gmail.com>
 <14a2ca08-e946-f319-e2a8-f5f91e1eb8e6@linux.alibaba.com> <d90c35db-c801-02ba-d9ea-148324a1de13@linux.alibaba.com>
In-Reply-To: <d90c35db-c801-02ba-d9ea-148324a1de13@linux.alibaba.com>
From:   Ian Rogers <irogers@google.com>
Date:   Wed, 4 Jan 2023 08:57:44 -0800
Message-ID: <CAP-5=fUVU2+bAyzU+ZKYAdw_G5u0eMRq4Cyw_CVKd3UsG2U_Bw@mail.gmail.com>
Subject: Re: [PATCH v5 2/6] perf vendor events arm64: Add TLB metrics for neoverse-n2
To:     Jing Zhang <renyu.zj@linux.alibaba.com>
Cc:     John Garry <john.g.garry@oracle.com>,
        Xing Zhengjun <zhengjun.xing@linux.intel.com>,
        Will Deacon <will@kernel.org>,
        James Clark <james.clark@arm.com>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        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>,
        Andrew Kilroy <andrew.kilroy@arm.com>,
        Shuai Xue <xueshuai@linux.alibaba.com>,
        Zhuo Song <zhuo.song@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jan 4, 2023 at 12:40 AM Jing Zhang <renyu.zj@linux.alibaba.com> wro=
te:
>
>
>
> =E5=9C=A8 2023/1/4 =E4=B8=8B=E5=8D=881:21, Jing Zhang =E5=86=99=E9=81=93:
> >
> >
> > =E5=9C=A8 2023/1/4 =E4=B8=8A=E5=8D=881:14, Ian Rogers =E5=86=99=E9=81=
=93:
> >> On Tue, Jan 3, 2023 at 3:39 AM Jing Zhang <renyu.zj@linux.alibaba.com>=
 wrote:
> >>>
> >>> Add TLB related metrics.
> >>>
> >>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
> >>> Acked-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>  .../arch/arm64/arm/neoverse-n2/metrics.json        | 49 ++++++++++++=
++++++++++
> >>>  1 file changed, 49 insertions(+)
> >>>
> >>> diff --git a/tools/perf/pmu-events/arch/arm64/arm/neoverse-n2/metrics=
.json b/tools/perf/pmu-events/arch/arm64/arm/neoverse-n2/metrics.json
> >>> index c126f1bc..8a74e07 100644
> >>> --- a/tools/perf/pmu-events/arch/arm64/arm/neoverse-n2/metrics.json
> >>> +++ b/tools/perf/pmu-events/arch/arm64/arm/neoverse-n2/metrics.json
> >>> @@ -26,5 +26,54 @@
> >>>          "MetricGroup": "TopdownL1",
> >>>          "MetricName": "backend_bound",
> >>>          "ScaleUnit": "100%"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "L1D_TLB_REFILL / L1D_TLB",
> >>> +        "BriefDescription": "The rate of L1D TLB refill to the overa=
ll L1D TLB lookups",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "l1d_tlb_miss_rate",
> >>> +        "ScaleUnit": "100%"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "L1I_TLB_REFILL / L1I_TLB",
> >>> +        "BriefDescription": "The rate of L1I TLB refill to the overa=
ll L1I TLB lookups",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "l1i_tlb_miss_rate",
> >>> +        "ScaleUnit": "100%"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "L2D_TLB_REFILL / L2D_TLB",
> >>> +        "BriefDescription": "The rate of L2D TLB refill to the overa=
ll L2D TLB lookups",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "l2_tlb_miss_rate",
> >>> +        "ScaleUnit": "100%"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "DTLB_WALK / INST_RETIRED * 1000",
> >>> +        "BriefDescription": "The rate of TLB Walks per kilo instruct=
ions for data accesses",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "dtlb_mpki",
> >>> +        "ScaleUnit": "MPKI"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "DTLB_WALK / L1D_TLB",
> >>> +        "BriefDescription": "The rate of DTLB Walks to the overall L=
1D TLB lookups",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "dtlb_walk_rate",
> >>> +        "ScaleUnit": "100%"
> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "ITLB_WALK / INST_RETIRED * 1000",
> >>> +        "BriefDescription": "The rate of TLB Walks per kilo instruct=
ions for instruction accesses",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "itlb_mpki",
> >>> +        "ScaleUnit": "MPKI"
> >>
> >> Did you test this? IIRC if there is no number in the ScaleUnit then
> >> the scale factor becomes 0 and the metric value is always multiplied
> >> by zero. Perhaps:
> >>
> >> "MetricName": "itlb_miss_rate",
> >> "MetricExpr": "ITLB / INST_RETIRED"
> >> "ScaleUnit": "1000MPKI"
> >>
> >> Thanks,
> >> Ian
> >>
> >
> > You are absolutely right, I only tested TLB metrics. Sorry for not doub=
le checking. I will repost the corrected patches.
> >
>
> I rethought it. I want to change the ScaleUnit to "1MPKI" and keep the Me=
tricExpr multiplied by 1000,
> so that the "MetricExpr" expresses the value of per kilo instruciton, whi=
ch can be consistent with the
> description in "BriefDescription". Like:
>    {
>         "MetricExpr": "DTLB_WALK / INST_RETIRED * 1000",
>         "BriefDescription": "The rate of TLB Walks per kilo instructions =
for data accesses",
>         "MetricGroup": "TLB",
>         "MetricName": "dtlb_mpki",
>         "ScaleUnit": "1MPKI"
>     },
>
>
> In addition, I think it is more reasonable for ScaleUnit to have a defaul=
t scale factor of 1 when there
> is no number. I want to try to fix this bug.
>
> Ian, what's your opnion?

I like intention revealing, itlb_mpki is something of a soup of
characters to de-acronym-ify compared to itlb_miss_rate, but rate may
not be completely intuitive in that name. I'm happy to follow your
lead. Putting the 1000 in the ScaleUnit or the expression doesn't
matter, so again happy to follow what you think is best.

Thanks,
Ian

> >>> +    },
> >>> +    {
> >>> +        "MetricExpr": "ITLB_WALK / L1I_TLB",
> >>> +        "BriefDescription": "The rate of ITLB Walks to the overall L=
1I TLB lookups",
> >>> +        "MetricGroup": "TLB",
> >>> +        "MetricName": "itlb_walk_rate",
> >>> +        "ScaleUnit": "100%"
> >>>      }
> >>>  ]
> >>> --
> >>> 1.8.3.1
> >>>