Re: [PATCH v1 04/20] perf jevents: Add tsx metric group for Intel models

[Ian Rogers]

On Thu, Feb 29, 2024 at 1:15 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2024-02-28 7:17 p.m., Ian Rogers wrote:
> > Allow duplicated metric to be dropped from json files.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/pmu-events/intel_metrics.py | 51 ++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/tools/perf/pmu-events/intel_metrics.py b/tools/perf/pmu-events/intel_metrics.py
> > index 20c25d142f24..1096accea2aa 100755
> > --- a/tools/perf/pmu-events/intel_metrics.py
> > +++ b/tools/perf/pmu-events/intel_metrics.py
> > @@ -7,6 +7,7 @@ import argparse
> >  import json
> >  import math
> >  import os
> > +from typing import Optional
> >
> >  parser = argparse.ArgumentParser(description="Intel perf json generator")
> >  parser.add_argument("-metricgroups", help="Generate metricgroups data", action='store_true')
> > @@ -77,10 +78,60 @@ def Smi() -> MetricGroup:
> >      ])
> >
> >
> > +def Tsx() -> Optional[MetricGroup]:
> > +    if args.model not in [
> > +        'alderlake',
> > +        'cascadelakex',
> > +        'icelake',
> > +        'icelakex',
> > +        'rocketlake',
> > +        'sapphirerapids',
> > +        'skylake',
> > +        'skylakex',
> > +        'tigerlake',> +    ]:
>
> Can we get ride of the model list? Otherwise, we have to keep updating
> the list.

Do we expect the list to update? :-) The issue is the events are in
sysfs and not the json. If we added the tsx events to json then this
list wouldn't be necessary, but it also would mean the events would be
present in "perf list" even when TSX is disabled.

> > +        return None
> > +> +    pmu = "cpu_core" if args.model == "alderlake" else "cpu"
>
> Is it possible to change the check to the existence of the "cpu" PMU
> here? has_pmu("cpu") ? "cpu" : "cpu_core"

The "Unit" on "cpu" events in json always just blank. On hybrid it is
either "cpu_core" or "cpu_atom", so I can make this something like:

pmu = "cpu_core" if metrics.HasPmu("cpu_core") else "cpu"

which would be a build time test.


> > +    cycles = Event('cycles')
> > +    cycles_in_tx = Event(f'{pmu}/cycles\-t/')
> > +    transaction_start = Event(f'{pmu}/tx\-start/')
> > +    cycles_in_tx_cp = Event(f'{pmu}/cycles\-ct/')
> > +    metrics = [
> > +        Metric('tsx_transactional_cycles',
> > +                      'Percentage of cycles within a transaction region.',
> > +                      Select(cycles_in_tx / cycles, has_event(cycles_in_tx), 0),
> > +                      '100%'),
> > +        Metric('tsx_aborted_cycles', 'Percentage of cycles in aborted transactions.',
> > +                      Select(max(cycles_in_tx - cycles_in_tx_cp, 0) / cycles,
> > +                                    has_event(cycles_in_tx),
> > +                                    0),
> > +                      '100%'),
> > +        Metric('tsx_cycles_per_transaction',
> > +                      'Number of cycles within a transaction divided by the number of transactions.',
> > +                      Select(cycles_in_tx / transaction_start,
> > +                                    has_event(cycles_in_tx),
> > +                                    0),
> > +                      "cycles / transaction"),
> > +    ]
> > +    if args.model != 'sapphirerapids':
>
> Add the "tsx_cycles_per_elision" metric only if
> has_event(f'{pmu}/el\-start/')?

It's a sysfs event, so this wouldn't work :-(

Thanks,
Ian

> Thanks,
> Kan
>
> > +        elision_start = Event(f'{pmu}/el\-start/')
> > +        metrics += [
> > +            Metric('tsx_cycles_per_elision',
> > +                          'Number of cycles within a transaction divided by the number of elisions.',
> > +                          Select(cycles_in_tx / elision_start,
> > +                                        has_event(elision_start),
> > +                                        0),
> > +                          "cycles / elision"),
> > +        ]
> > +    return MetricGroup('transaction', metrics)
> > +
> > +
> >  all_metrics = MetricGroup("", [
> >      Idle(),
> >      Rapl(),
> >      Smi(),
> > +    Tsx(),
> >  ])
> >
> >  if args.metricgroups: