Received-SPF: pass (google.com: domain of linux-kernel+bounces-88585-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e1e04f03-5d99-466d-a2d1-ce7fb15e8b8e@linux.intel.com>
Date: Fri, 1 Mar 2024 09:52:23 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 04/20] perf jevents: Add tsx metric group for Intel
 models
Content-Language: en-US
To: Ian Rogers <irogers@google.com>
Cc: Perry Taylor <perry.taylor@intel.com>,
 Samantha Alt <samantha.alt@intel.com>,
 Caleb Biggers <caleb.biggers@intel.com>, Weilin Wang
 <weilin.wang@intel.com>, Edward Baker <edward.baker@intel.com>,
 Andi Kleen <ak@linux.intel.com>, Peter Zijlstra <peterz@infradead.org>,
 Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>,
 John Garry <john.g.garry@oracle.com>, Jing Zhang
 <renyu.zj@linux.alibaba.com>, Thomas Richter <tmricht@linux.ibm.com>,
 James Clark <james.clark@arm.com>, linux-kernel@vger.kernel.org,
 linux-perf-users@vger.kernel.org, Stephane Eranian <eranian@google.com>
References: <20240229001806.4158429-1-irogers@google.com>
 <20240229001806.4158429-5-irogers@google.com>
 <7aa2d2a2-b8f9-478f-9699-7b717d38a8ab@linux.intel.com>
 <CAP-5=fWSidi1zjwn8Zr93dAQvRBtngKrkVwDxorTMmVSVc9FWg@mail.gmail.com>
From: "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <CAP-5=fWSidi1zjwn8Zr93dAQvRBtngKrkVwDxorTMmVSVc9FWg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2024-02-29 8:01 p.m., Ian Rogers wrote:
> On Thu, Feb 29, 2024 at 1:15 PM Liang, Kan <kan.liang@linux.intel.com> 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@google.com>
>>> ---
>>>  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? :-) 

Yes, at least for the meteorlake and graniterapids. They should be the
same as alderlake and sapphirerapids. I'm not sure about the future
platforms.

Maybe we can have a if args.model in list here to include all the
non-hybrid models which doesn't support TSX. I think the list should not
be changed shortly.

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

I think there may an alternative way, to check the RTM events, e.g.,
RTM_RETIRED.START event. We only need to generate the metrics for the
platform which supports the RTM_RETIRED.START event.


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

Yes, I think using the "Unit" is good enough.

> 
> 
>>> +    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 :-(

The below is the definition of el-start in the kernel.
EVENT_ATTR_STR(el-start,	el_start,	"event=0xc8,umask=0x1");

The corresponding event in the event list should be HLE_RETIRED.START
      "EventCode": "0xC8",
      "UMask": "0x01",
      "EventName": "HLE_RETIRED.START",

I think we may check the HLE_RETIRED.START instead. If the
HLE_RETIRED.START doesn't exist, I don't see a reason why the
tsx_cycles_per_elision should be supported.

Again, in the virtualization world, it's possible that the
HLE_RETIRED.START exists in the event list but el_start isn't available
in the sysfs. I think it has to be specially handle in the test as well.

Thanks,
Kan

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