Message-ID: <CAP-5=fVLCV5bskAMDTvxZYxgbBYg9-yccF1-EhZPwAgVYU9Htg@mail.gmail.com>
Date:   Tue, 10 Jan 2023 22:14:34 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     James Clark <james.clark@....com>
Cc:     John Garry <john.g.garry@...cle.com>,
        Jing Zhang <renyu.zj@...ux.alibaba.com>,
        Xing Zhengjun <zhengjun.xing@...ux.intel.com>,
        Will Deacon <will@...nel.org>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Andrew Kilroy <andrew.kilroy@....com>,
        Shuai Xue <xueshuai@...ux.alibaba.com>,
        Zhuo Song <zhuo.song@...ux.alibaba.com>
Subject: Re: [PATCH v5 1/6] perf vendor events arm64: Add topdown L1 metrics
 for neoverse-n2

On Mon, Jan 9, 2023 at 7:35 AM James Clark <james.clark@....com> wrote:
>
>
>
> On 06/01/2023 10:14, John Garry wrote:
> > On 05/01/2023 21:13, Ian Rogers wrote:
> >>>> This may be a feasible idea. The value of slots comes from the
> >>>> register PMMIR_EL1, which I can read in
> >>>> /sys/bus/event_source/device/armv8_pmuv3_*/caps/slots. But how do I
> >>>> replace the slots in MetricExpr with the
> >>>> read slots values? Currently I understand that parameters in
> >>>> metricExpr only support events and constants.
> >>>>
> >>> Maybe during runtime we could create a pseudo metric/event for SLOT.
> >> For Intel we do this by just having a different constant for each
> >> architecture. It is fairly easy to add a new "literal", so you could
> >> add a #slots in expr__get_literal:
> >> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/expr.c?h=perf*core*n407__;LyM!!ACWV5N9M2RV99hQ!IHcZFuFaLdQDQvVOnHVlbbME2S4aW8GohWUkydlejpi7ifFz61r7RutGXReRt0d88X_vDfkTySCiuD2PqOA$  Populating it would be the challenge 😄
> >
> > Thanks for the pointer. I think that the challenge in populating it
> > really comes down to whether we would really want to make this generic.
> >
> > I suppose that for arm64 we could have a method which accesses this
> > PMMIR_EL1 register, while for other archs we could have a weak function
> > which just returns NAN. If other archs want to use this key expr, they
> > can add their own method.
> >
>
> I wonder if it would be worthwhile and even more generic to add some
> sort of int containing file accessor construct. It could also have
> support for a default value when the file doesn't exist. For example:
>
>   "MetricExpr": "ITLB / {file://<pmu>/caps/slots(5)}"
>
> It gets a bit fiddly because you might want to support absolute paths
> and paths relative to whatever PMU is being used. But it could prevent
> having to add some custom identifier and glue code for every possible
> file that just has an integer in it.
>
> It also wouldn't be possible to support the case where the file has
> bitfields in it that need to be extracted, so maybe we shouldn't do it.
>
> James

Thanks James,

I think there are many opportunities to improve the metrics. One step
in this direction is:
https://lore.kernel.org/lkml/20221221223420.2157113-1-irogers@google.com/
(which is looking for reviews :-D ). Some areas we could improve include:
 - the expression code has support for longs but I don't believe any
metrics use it.
 - the modulus is weird and again unused.
 - I think divide (/) should behave like d_ratio as aborting parsing
is next to useless.
 - events like Intel's msr/tsc/ don't have to be programmed on every
CPU/hyperthread and doing so is quite wasteful.
 - we may have a read but no write counter, so being able to read a
sibling CPUs/socket's read counter may inform about writes. This isn't
currently expressible as metrics compute based on whatever the
aggregation mode is, you can't get a particular count.
 - perf stat record/report don't work/compute metrics, but just
provide counters.
 - the json format should resemble sysfs rather than being a flat
list, metrics and events in the list should be separated.
 - metrics use / as divide and so @ is used in /'s place for event
modifiers. BPF events use / as a directory separator.

For the filesystems reading I think it is a good idea. I'd like to
make it so that things like #num_dies become tool events and remove
the notion of literals. Perhaps we can make reading a file something
that is an event. The current event parsing logic is overly complex,
for example the handling of '-' which has some legacy PMU separation
properties. A proposal mentioned at LPC was to have a new event
parsing library that doesn't carry legacy baggage. We can make metric
code use this as the metrics encode the events. If the new library
fails parsing the code can fall back on the existing parser. I'd like
it if the event parsing logic more closely resembled the sysfs style.
I'd like it if we could have events in sysfs, built into the tool (but
with a layout resembling sysfs) and also allow events, etc. to be
added by having say a zip of a sysfs directory/file structure. I'm
hoping libraries like metric.py:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/pmu-events/metric.py
can be used by vendors, so that it is easy to update vendor generated
json if/when the format changes.

Thanks,
Ian

Powered by blists - more mailing lists