Date:   Tue, 6 Dec 2022 11:29:51 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.g.garry@...cle.com>
Cc:     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>,
        Sumanth Korikkar <sumanthk@...ux.ibm.com>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v3] perf jevents: Parse metrics during conversion

On Mon, Dec 5, 2022 at 7:24 AM John Garry <john.g.garry@...cle.com> wrote:
>
> On 01/12/2022 03:41, Ian Rogers wrote:
> > Currently the 'MetricExpr' json value is passed from the json
> > file to the pmu-events.c. This change introduces an expression
> > tree that is parsed into. The parsing is done largely by using
> > operator overloading and python's 'eval' function. Two advantages
> > in doing this are:
> >
> > 1) Broken metrics fail at compile time rather than relying on
> >     `perf test` to detect. `perf test` remains relevant for checking
> >     event encoding and actual metric use.
>
> Do we still require the code to "resolve metrics" in resolve_metric()?
> But I'm not sure it even ever had any users.

We use metrics referencing other metrics for topdown metrics on x86.
For example:

https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/pmu-events/arch/x86/icelakex/icx-metrics.json?h=perf/core#n34
    {
        "BriefDescription": "This metric represents fraction of cycles
the CPU was stalled due to Branch Resteers",
        "MetricExpr": "INT_MISC.CLEAR_RESTEER_CYCLES / CLKS +
tma_unknown_branches",
        "MetricGroup": "FetchLat;TopdownL3;tma_fetch_latency_group",
        "MetricName": "tma_branch_resteers",
        "PublicDescription": "This metric represents fraction of
cycles the CPU was stalled due to Branch Resteers. Branch Resteers
estimates the Frontend delay in fetching operations from corrected
path; following all sorts of miss-predicted branches. For example;
branchy code with lots of miss-predictions might get categorized under
Branch Resteers. Note the value of this node may overlap with its
siblings. Sample with: BR_MISP_RETIRED.ALL_BRANCHES",
        "ScaleUnit": "100%"
    },
...
    {
        "BriefDescription": "This metric represents fraction of cycles
the CPU was stalled due to new branch address clears",
        "MetricExpr": "10 * BACLEARS.ANY / CLKS",
        "MetricGroup": "BigFoot;FetchLat;TopdownL4;tma_branch_resteers_group",
        "MetricName": "tma_unknown_branches",
        "PublicDescription": "This metric represents fraction of
cycles the CPU was stalled due to new branch address clears. These are
fetched branches the Branch Prediction Unit was unable to recognize
(First fetch or hitting BPU capacity limit). Sample with:
BACLEARS.ANY",
        "ScaleUnit": "100%"
    },

> >
> > 2) The conversion to a string from the tree can minimize the metric's
> >     string size, for example, preferring 1e6 over 1000000, avoiding
> >     multiplication by 1 and removing unnecessary whitespace. On x86
> >     this reduces the string size by 3,050bytes (0.07%).
>
> Out of curiosity, did you try the exponent change on its own (to see the
> impact on size)?

The file size savings are very modest. Without removing the "1 * " the
savings were roughly 2KB, perhaps 1KB was shrinking the constant
exponents.

> Nit:
>
> Unrelated, really, I notice that sometimes we lose the parenthesis and
> sometimes never had them, like:
>
> /* offset=11526 */ "\000\000metrics\000Ave [...] 0\000( 1000000000 * (
> UNC_CHA
> /* offset=11207 */ "\000\000metrics\000Ave [...] 0\0001e9 * (UNC_CHA_TOR
>
> To me, it seems neater to have the expression contained within (a
> parenthesis) ever since we moved to this "big string". This seems to be
> a preexisting feature.

You can also read the metrics through "perf list --detail", we could
add parentheses there if it helps readability. We can also expand out
what the big string values are for comments. Fwiw, I want to start
refactoring jevents.py in follow up work and that would impact
readability. Some thoughts there are:

1) we shouldn't parse all json events for all PMUs in prior to parsing
events, we should initialize a PMU when an event references it and
then possibly then go through the json events. To facilitate this it
would be useful to organize events by their PMU.
2) metrics and events should be separated at least in the C code.
Currently on x86 ScaleUnit in the json will apply both to an event and
its metric, even though the uses of an event and a metric should have
different units.
3) for some operating systems with limited disk, it would be nice to
be able to have the build exclude models.

Let me know if there's anything more outstanding to fix on this patch set.

Thanks,
Ian

> Thanks,
> John
>
>
> >
> > In future changes it would be possible to programmatically
> > generate the json expressions (a single line of text and so a
> > pain to write manually) for an architecture using the expression
> > tree. This could avoid copy-pasting metrics for all architecture
> > variants.
> >
> > Signed-off-by: Ian Rogers<irogers@...gle.com>
>

Powered by blists - more mailing lists