Message-ID: <CAP-5=fXFZffdRMFH+aFzFech2pjbfYyQ+fx+E-YnfMBHifD4gw@mail.gmail.com>
Date:   Mon, 14 Aug 2023 15:31:55 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Jing Zhang <renyu.zj@...ux.alibaba.com>
Cc:     John Garry <john.g.garry@...cle.com>,
        Will Deacon <will@...nel.org>,
        James Clark <james.clark@....com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-doc@...r.kernel.org,
        Zhuo Song <zhuo.song@...ux.alibaba.com>,
        Shuai Xue <xueshuai@...ux.alibaba.com>
Subject: Re: [PATCH v6 3/7] perf jevents: Support more event fields

On Mon, Aug 7, 2023 at 12:51 AM Jing Zhang <renyu.zj@...ux.alibaba.com> wrote:
>
> The usual event descriptions are "event=xxx" or "config=xxx", while the
> event descriptions of CMN are "type=xxx, eventid=xxx" or more complex.

I found this difficult to read in relation to the code. Perhaps:

The previous code assumes an event has either an "event=" or "config"
field at the beginning. For CMN neither of these may be present, as an
event is typically "type=xx,eventid=xxx".

I think the use of the name "type" here is unfortunate. It conflicts
with the PMU's type as defined in perf_event_attr.

In general I think the jevents.py code needs improving, the
event_fields dictionary is convoluted, we shouldn't be afraid to
change the event json for example to get rid of things like ExtSel, we
should really ensure the formats in the events are valid for the PMU
they are for.

> $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill
> type=0x5,eventid=0x3
>
> When adding aliases for events described as "event=xxx" or "config=xxx",
> EventCode or ConfigCode can be used in the JSON files to describe the
> events. But "eventid=xxx, type=xxx" cannot be supported at present.
>
> If EventCode and ConfigCode is not added in the alias JSON file, the
> event description will add "event=0" by default. So, even if the event
> field is added to supplement "eventid=xxx" and "type=xxx", the final
> parsing result will be "event=0, eventid=xxx, type=xxx".
>
> Therefore, when EventCode and ConfigCode are missing in JSON, "event=0" is
> no longer added by default. EventIdCode and Type are added to the event
> field, and ConfigCode is moved into the event_field array which can also
> guarantee its original function.

A useful test can be to build with JEVENTS_ARCH=all and confirm the
before and after change generated pmu-events.c is the same.

> Signed-off-by: Jing Zhang <renyu.zj@...ux.alibaba.com>
> ---
>  tools/perf/pmu-events/jevents.py | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index f57a8f2..9c0f63a 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -275,12 +275,6 @@ class JsonEvent:
>        }
>        return table[unit] if unit in table else f'uncore_{unit.lower()}'
>
> -    eventcode = 0
> -    if 'EventCode' in jd:
> -      eventcode = int(jd['EventCode'].split(',', 1)[0], 0)
> -    if 'ExtSel' in jd:
> -      eventcode |= int(jd['ExtSel']) << 8
> -    configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None
>      self.name = jd['EventName'].lower() if 'EventName' in jd else None
>      self.topic = ''
>      self.compat = jd.get('Compat')
> @@ -317,7 +311,15 @@ class JsonEvent:
>      if precise and self.desc and '(Precise Event)' not in self.desc:
>        extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise '
>                                                                   'event)')
> -    event = f'config={llx(configcode)}' if configcode is not None else f'event={llx(eventcode)}'
> +    eventcode = None
> +    if 'EventCode' in jd:
> +      eventcode = int(jd['EventCode'].split(',', 1)[0], 0)
> +    if 'ExtSel' in jd:
> +      if eventcode is None:
> +        eventcode = int(jd['ExtSel']) << 8
> +      else:
> +        eventcode |= int(jd['ExtSel']) << 8
> +    event = f'event={llx(eventcode)}' if eventcode is not None else None
>      event_fields = [
>          ('AnyThread', 'any='),
>          ('PortMask', 'ch_mask='),
> @@ -327,10 +329,13 @@ class JsonEvent:
>          ('Invert', 'inv='),
>          ('SampleAfterValue', 'period='),
>          ('UMask', 'umask='),
> +        ('ConfigCode', 'config='),

This loses the int and potential base conversion of ConfigCode.
Clearly the code was taking care to maintain this behavior so I
suspect this change has broken something. JEVENTS_ARCH=all should
reveal the answer.

> +        ('Type', 'type='),
> +        ('EventIdCode', 'eventid='),
>      ]
>      for key, value in event_fields:
>        if key in jd and jd[key] != '0':
> -        event += ',' + value + jd[key]
> +        event = event + ',' + value + jd[key] if event is not None else value + jd[key]

Perhaps initialize event above to the empty string then:

if key in jd and jd[key] != '0':
  if event:
     event += ','
  event += value + jd[key]

Thanks,
Ian

>      if filter:
>        event += f',{filter}'
>      if msr:
> --
> 1.8.3.1
>

Powered by blists - more mailing lists