Re: [PATCH v3 2/2] perf jevents: Autogenerate empty-pmu-events.c

[Ian Rogers]

On Wed, Jul 31, 2024 at 1:41 PM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> On Wed, Jul 31, 2024 at 01:20:06PM -0700, Ian Rogers wrote:
> > On Wed, Jul 31, 2024 at 11:53 AM Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 08:58:43AM -0700, Ian Rogers wrote:
> > > > On Wed, Jul 31, 2024 at 8:46 AM Arnaldo Carvalho de Melo
> > > > <acme@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Wed, Jul 31, 2024 at 12:33:50PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > On Wed, Jul 31, 2024 at 07:08:18AM -0700, Ian Rogers wrote:
> > > > > > > On Wed, Jul 31, 2024 at 6:18 AM Arnaldo Carvalho de Melo
> > > > > > > <acme@xxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jul 30, 2024 at 12:17:44PM -0700, Ian Rogers wrote:
> > > > > > > > > empty-pmu-events.c exists so that builds may occur without python
> > > > > > > > > being installed on a system. Manually updating empty-pmu-events.c to
> > > > > > > > > be in sync with jevents.py is a pain, let's use jevents.py to generate
> > > > > > > > > empty-pmu-events.c.
> > > > > > > >
> > > > > > > > What am I missing here?
> > > > > > > >
> > > > > > > > If it exists so that we can build on a system without python how can we
> > > > > > > > use python to generate it?
> > > > > > > >
> > > > > > > > Now having python in the system is a requirement and thus we don't need
> > > > > > > > empty-pmu-events.c anymore?
> > > > > > > >
> > > > > > > > Can you guys please clarify that?
> > > > > > >
> > > > > > > The requirement for python hasn't changed.
> > > > > > >
> > > > > > > Case 1: no python or NO_JEVENTS=1
> > > > > > > Build happens using empty-pmu-events.c that is checked in, no python
> > > > > > > is required.
> > > > > > >
> > > > > > > Case 2: python
> > > > > > > pmu-events.c is created by jevents.py (requiring python) and then built.
> > > > > > > This change adds a step where the empty-pmu-events.c is created using
> > > > > > > jevents.py and that file is diffed against the checked in version.
> > > > > > > This stops the checked in empty-pmu-events.c diverging if changes are
> > > > > > > made to jevents.py. If the diff causes the build to fail then you just
> > > > > > > copy the diff empty-pmu-events.c over the checked in one.
> > > > > >
> > > > > > I'll try and add your explanation to the log message, thanks for
> > > > > > clarifying it!
> > > > >
> > > > > So, with it in place I'm now noticing:
> > > > >
> > > > > ⬢[acme@toolbox perf-tools-next]$ rm -rf /tmp/build/$(basename $PWD)/ ; mkdir -p /tmp/build/$(basename $PWD)/
> > > > > ⬢[acme@toolbox perf-tools-next]$ alias m='rm -rf ~/libexec/perf-core/ ; make -k CORESIGHT=1 O=/tmp/build/$(basename $PWD)/ -C tools/perf install-bin && perf test python'
> > > > > ⬢[acme@toolbox perf-tools-next]$ m
> > > > > <SNIP>
> > > > >   GEN     /tmp/build/perf-tools-next/pmu-events/test-empty-pmu-events.c
> > > > >   MKDIR   /tmp/build/perf-tools-next/arch/x86/util/
> > > > >   CC      /tmp/build/perf-tools-next/util/annotate.o
> > > > >   CC      /tmp/build/perf-tools-next/arch/x86/util/tsc.o
> > > > >   CC      /tmp/build/perf-tools-next/arch/x86/tests/hybrid.o
> > > > >   CC      /tmp/build/perf-tools-next/util/block-info.o
> > > > >   CC      /tmp/build/perf-tools-next/arch/x86/tests/intel-pt-test.o
> > > > >   CC      /tmp/build/perf-tools-next/arch/x86/util/pmu.o
> > > > >   MKDIR   /tmp/build/perf-tools-next/ui/browsers/
> > > > >   CC      /tmp/build/perf-tools-next/ui/browsers/annotate.o
> > > > >   CC      /tmp/build/perf-tools-next/builtin-kallsyms.o
> > > > >   CC      /tmp/build/perf-tools-next/util/block-range.o
> > > > >   TEST    /tmp/build/perf-tools-next/pmu-events/empty-pmu-events.log
> > > > > --- pmu-events/empty-pmu-events.c       2024-07-31 12:44:14.355042296 -0300
> > > > > +++ /tmp/build/perf-tools-next/pmu-events/test-empty-pmu-events.c       2024-07-31 12:45:35.048682785 -0300
> > > > > @@ -380,7 +380,7 @@
> > > > >                          continue;
> > > > >
> > > > >                  ret = pmu_events_table__for_each_event_pmu(table, table_pmu, fn, data);
> > > > > -                if (pmu || ret)
> > > > > +                if (ret)
> > > >
> > > > Right, you need to copy:
> > > >  /tmp/build/perf-tools-next/pmu-events/test-empty-pmu-events.c
> > > > to
> > > > tools/perf/pmu-events/empty-pmu-events.c
> > > > to fix this.
> > > >
> > > > This change has happened as you are testing with:
> > > > https://lore.kernel.org/lkml/20240716132951.1748662-1-kan.liang@xxxxxxxxxxxxxxx/
> > > > which isn't in the git repo yet (therefore, I can't make a patch set
> > > > on it). The change is WAI as it is telling you empty-pmu-events.c has
> > > > become stale and needs Kan's fix applying to it.
> > >
> > > ok, I'll remove Kan's patch, publish perf-tools-next and wait for the
> > > now normal flow of patches.
> >
> > I can resend Kan's patch with the empty-pmu-events.c fix applied. I
> > don't see the changes in tmp.perf-tools-next so I can do it with
> > cherry picks.
>
> Just force pushed one more time. After a while should be there, there
> are still some issues here and there, notably:
>
>
> root@x1:~# perf test 105 106 118
> 105: perf all metricgroups test                                      : FAILED!
> 106: perf all metrics test                                           : FAILED!

These tests can be sensitive to the NMI watchdog being enabled.
> 118: Miscellaneous Intel PT testing                                  : FAILED!

I bisected this failure to:

```
Author: Leo Yan <leo.yan@xxxxxxx>
Date:   Thu Jul 25 07:46:17 2024 +0100

   perf auxtrace: Iterate all AUX events when finish reading

   When finished to read AUX trace data from mmaped buffer, based on the
   AUX buffer index the core layer needs to search the corresponding PMU
   event and re-enable it to continue tracing.

   However, current code only searches the first AUX event. It misses to
   search other enabled AUX events, thus, it returns failure if the buffer
   index does not belong to the first AUX event.

   This patch extends the auxtrace_record__read_finish() function to
   search for every enabled AUX events, so all the mmaped buffer indexes
   can be covered.

   Signed-off-by: Leo Yan <leo.yan@xxxxxxx>
   Cc: Mark Rutland <mark.rutland@xxxxxxx>
   Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
   Cc: Ian Rogers <irogers@xxxxxxxxxx>
   Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
   Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
   Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
   Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
   Cc: James Clark <james.clark@xxxxxxxxxx>
   Cc: Mike Leach <mike.leach@xxxxxxxxxx>
   Cc:  <coresight@xxxxxxxxxxxxxxxx>
   Cc: John Garry <john.g.garry@xxxxxxxxxx>
   Cc:  <linux-perf-users@xxxxxxxxxxxxxxx>
   Link: https://lore.kernel.org/lkml/
   Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
```

> root@x1:~# perf test 110
> 110: perf stat --bpf-counters --for-each-cgroup test                 : FAILED!

This one flakes if the test machine has load on it.

> root@x1:~#
>
> I'm running out of time today, so I'll probably just push what I have to
> perf-tools-next so that we can start getting testing from linux-next and
> we can then go on fixing up stuff from there.

Thanks,
Ian