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: <20220510104758.64677-1-nick.forrington@arm.com>
 <YnqJwMfi/bcDmN4H@kernel.org> <b1081013-2cd9-f223-8125-d7e51d784f90@huawei.com>
 <eec03f0d-d491-511c-d9b0-694c6d527b38@arm.com> <1062b28d-9c7b-89fb-d3bf-519f84029b96@huawei.com>
In-Reply-To: <1062b28d-9c7b-89fb-d3bf-519f84029b96@huawei.com>
From:   Ian Rogers <irogers@google.com>
Date:   Sun, 15 May 2022 15:03:17 -0700
Message-ID: <CAP-5=fWYU1hzhGTc6q_u9r82g+rbn18+-i9o0W8sUVyk+t694w@mail.gmail.com>
Subject: Re: [PATCH 00/20] perf vendors events arm64: Multiple Arm CPUs
To:     John Garry <john.garry@huawei.com>
Cc:     Nick Forrington <nick.forrington@arm.com>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        Will Deacon <will@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        James Clark <james.clark@arm.com>,
        Andrew Kilroy <andrew.kilroy@arm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, May 12, 2022 at 8:53 AM John Garry <john.garry@huawei.com> wrote:
>
> On 12/05/2022 14:01, Nick Forrington wrote:
> > On 10/05/2022 16:55, John Garry wrote:
> >> On 10/05/2022 16:50, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, May 10, 2022 at 11:47:38AM +0100, Nick Forrington escreveu:
> >>>> Add Performance Monitoring Unit event data for the Arm CPUs listed
> >>>> below.
> >>>>
> >>>> Changesets are dependent due to incremental updates to the common
> >>>> events
> >>>> file and mapfile.csv.
> >>>>
> >>>> Data is sourced fromhttps://github.com/ARM-software/data
> >>> Waiting for reviews to merge this.
> >>>
> >>
> >> I'll have a closer look this week
>
> Generally this looks ok:
>
> Reviewed-by: John Garry <john.garry@huawei.com>
>
> If you are feeling particularly helpful then you can add support for any
> events missing to pre-existing core support, like a57-a72.
>
> Thanks,
> john

I'll raise John's "ok" and say this looks great! :-D Some thoughts:

The mapfile.csv cpuid values don't directly align with:
https://github.com/ARM-software/data/blob/master/cpus.json
but this definitely looks deliberate.

The new events lack the PMU "Unit" value. The current perf json is
pretty free form and leads to problems if two PMUs are present.
Context is here:
https://lore.kernel.org/lkml/CAP-5=fWRRZsyJZ-gky-FOFz79zW_3r78d_0APpj5sf66HqTpLw@mail.gmail.com/

My idea to rationalize this is to mirror what is already done in
sysfs, that is the event data is specific to a PMU. As a lot of "Unit"
values are missing from events on x86 a reasonable guess if the "Unit"
is missing is to use "cpu". Poking a Google Pixel 4a, I see that all
PMU data is in "armv8_pmuv3". So for ARM I could guess this is always
the case, ie all events should belong to armv8_pmuv3. This may not be
right and could lead to confusion like an event BR_COND_MIS_PRED
having an alias of "armv8_pmuv3/BR_COND_MIS_PRED/" but it really
should have some other PMU name in there. I just raise this in case
there is a fix for this we could incorporate into this patch series,
maybe "armv8_pmuv3" is always the PMU and my life is easy.

Thanks,
Ian