Message-ID: <dfaf436c-c669-7088-b3b6-dd6dd8b882c7@oracle.com>
Date:   Wed, 7 Dec 2022 11:21:38 +0000
From:   John Garry <john.g.garry@...cle.com>
To:     Ian Rogers <irogers@...gle.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 06/12/2022 19:29, Ian Rogers wrote:
> 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:

ok, I just wasn't sure if there were ever any metrics which did require 
"resolving". Now I know.

> 
> https://urldefense.com/v3/__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__;LyM!!ACWV5N9M2RV99hQ!PMKcRLro8XREBI-072XYolfLHVvOm4P-HBWTpvu8IxJzkE0NWydgW9wi2PclFvUrdQcuC-4uvubPf5RgWYI$  
>      {
>          "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.

At least being consistent would be nice, whichever way you want to go.

  We can also expand out
> what the big string values are for comments. 

Maybe a comment at the top of the array would be nice to tell which 
member is per column.

> 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.

Eh, do you mean an option to build just for the host system? If so, 
seems reasonable.

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

It seems fine. FWIW,

Reviewed-by: John Garry <john.g.garry@...cle.com>

Powered by blists - more mailing lists