Re: [PATCH 0/7] Rewrite jevents program in python

[Ian Rogers]

On Wed, May 11, 2022 at 4:13 AM John Garry <john.garry@xxxxxxxxxx> wrote:
>
> On 11/05/2022 08:01, Ian Rogers wrote:
> > New architectures bring new complexity, such as Intel's hybrid
> > models. jevents provides an alternative to specifying events in the
> > kernel and exposing them through sysfs, however, it is difficult to
> > work with. For example, an error in the json input would yield an
> > error message but no json file or location. It is also a challenge to
> > update jsmn.c given its forked nature.
> >
> > The changes here switch from jevents.c to a rewrite in python called
> > jevents.py. This means there is a build time dependency on python, but
> > such a dependency already exists for asciidoc (used to generate perf's
> > man pages).
> >
>
> Hi Ian,
>
> This does not build for me:
>
> Auto-detecting system features:
> ...                         dwarf: [ on  ]
> ...            dwarf_getlocations: [ on  ]
> ...                         glibc: [ on  ]
> ...                        libbfd: [ OFF ]
> ...                libbfd-buildid: [ OFF ]
> ...                        libcap: [ on  ]
> ...                        libelf: [ on  ]
> ...                       libnuma: [ on  ]
> ...        numa_num_possible_cpus: [ on  ]
> ...                       libperl: [ on  ]
> ...                     libpython: [ on  ]
> ...                     libcrypto: [ on  ]
> ...                     libunwind: [ on  ]
> ...            libdw-dwarf-unwind: [ on  ]
> ...                          zlib: [ on  ]
> ...                          lzma: [ on  ]
> ...                     get_cpuid: [ on  ]
> ...                           bpf: [ on  ]
> ...                        libaio: [ on  ]
> ...                       libzstd: [ on  ]
> ...        disassembler-four-args: [ on  ]
>
>
> make[3]: Nothing to be done for 'install_headers'.
>   GEN     pmu-events/pmu-events.c
>   CC      /home/john/acme/tools/perf/libbpf/staticobjs/libbpf.o
>   LINK    dlfilters/dlfilter-test-api-v0.so
>   CC      dlfilters/dlfilter-show-cycles.o
>   CC      builtin-bench.o
> Traceback (most recent call last):
>   File "pmu-events/jevents.py", line 23, in <module>
>     def file_name_to_table_name(parents: list[str], dirname: str) -> str:
> TypeError: 'type' object is not subscriptable
> make[3]: *** [pmu-events/Build:15: pmu-events/pmu-events.c] Error 1
> make[2]: *** [Makefile.perf:662: pmu-events/pmu-events-in.o] Error 2
> make[2]: *** Waiting for unfinished jobs....
>
> What baseline do you use? It's always good to mention it I think.

You're right, sorry for that. I was testing with:
$ python3 --version
Python 3.9.10

>From your output it looks like the newer type hints feature in python
is missing. I think the type hints help the code, but they aren't a
requirement for this change. I'll look into compatibility with older
pythons.

> > A challenge with this code is in avoiding regressions. For this reason
> > the jevents.py produces identical output to jevents.c, validated with a
> > test script and build target.
>
> As you know (and have done), verifying no regression should be
> straightforward by diff'ing. For avoidance of doubt, which archs did you
> test? We also need to ensure those which don't use pmu-events (like
> arm32) work as before.

The test in:
https://lore.kernel.org/linux-perf-users/20220511070133.710721-6-irogers@xxxxxxxxxx/
tests all architectures that exist in tools/perf/pmu-eeventss/arch:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/pmu-events/arch?h=perf/core

You're right that I've missed arm32 and other architectures that don't
have a directory there. Something to fix for v2.

Thanks,
Ian

> >
> > A difference in the python to the C approach is that the python loads
> > an entire json file in to memory, while the C code works from token to
> > token. In some cases the C approach was sensitive to the order of
> > dictionary items in the json file. To ensure matching output there are
> > two changes made to jevents.c to cause it to read all values before
> > creating output.
> >
> > The changes also found a bug in Ivytown's UNC_M_ACT_COUNT.RD event
> > encoding, as well as unnecessary whitespace introduced in Alderlake's
> > metrics. In these cases the json input is fixed.
>
> Thanks,
> John