Messages in this thread | | | From | Ian Rogers <> | Date | Tue, 6 Dec 2022 11:29:51 -0800 | Subject | Re: [PATCH v3] perf jevents: Parse metrics during conversion |
| |
On Mon, 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:
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@google.com> >
| |