Re: [PATCH v3] perf jevents: Parse metrics during conversion

[John Garry]

On 06/12/2022 19:29, Ian Rogers wrote:
> Dec 5, 2022 at 7:24 AM John Garry<john.g.garry@xxxxxxxxxx>  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@xxxxxxxxxx>