Messages in this thread | | | Date | Wed, 7 Dec 2022 11:21:38 +0000 | Subject | Re: [PATCH v3] perf jevents: Parse metrics during conversion | From | John Garry <> |
| |
On 06/12/2022 19:29, Ian Rogers wrote: > Dec 5, 2022 at 7:24 AM John Garry<john.g.garry@oracle.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@oracle.com>
| |