Re: [PATCH v5 4/4] perf parse-events: Reapply "Prefer sysfs/JSON hardware events over legacy"

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Jan 10, 2025 at 02:15:18PM -0800, Ian Rogers wrote:
> On Fri, Jan 10, 2025 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Jan 09, 2025 at 02:21:09PM -0800, Ian Rogers wrote:
> > > Originally posted and merged from:
> > > https://lore.kernel.org/r/20240416061533.921723-10-irogers@google.com
> > > This reverts commit 4f1b067359ac8364cdb7f9fda41085fa85789d0f although
> > > the patch is now smaller due to related fixes being applied in commit
> > > 22a4db3c3603 ("perf evsel: Add alternate_hw_config and use in
> > > evsel__match").
> > > The original commit message was:
> > >
> > > It was requested that RISC-V be able to add events to the perf tool so
> > > the PMU driver didn't need to map legacy events to config encodings:
> > > https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
> > >
> > > This change makes the priority of events specified without a PMU the
> > > same as those specified with a PMU, namely sysfs and JSON events are
> > > checked first before using the legacy encoding.
> >
> > I'm still not convinced why we need this change despite of these
> > troubles.  If it's because RISC-V cannot define the lagacy hardware
> > events in the kernel driver, why not using a different name in JSON and
> > ask users to use the name specifically?  Something like:
> >
> >   $ perf record -e riscv-cycles ...
> 
> So ARM and RISC-V are more than able to speak for themselves and have
> their tags on the series, but let's recap why I'm motivated to do this
> change:
> 
> 1) perf supported legacy events;
> 2) perf supported sysfs and json events, but at a lower priority than
> legacy events;
> 3) hybrid support was added but in a way where all the hybrid PMUs
> needed to be known, assumptions about PMU were implicit and baked into
> the tool;
> 4) metric support for hybrid was going in a similar implicit direction
> and I objected, what would cycles mean in a metric if the core PMU was

If the legacy cycles event in a metric is a problem, can we change the
metric to be more specific?


> implicit? Rather than pursue this the hybrid code was overhauled, PMUs
> became more of a thing and we added a notion of a "core" PMU which
> would support legacy events;
> 5) ARM core PMUs differ in naming, etc. than just about every other
> platform. Their core events had been being programmed as if they were
> uncore events - ie without the legacy priority. Fixing hybrid, and
> fixing ARM PMUs to know they supported legacy events, broke perf on
> Apple-M? series due to a PMU driver issue with legacy events:
> https://lore.kernel.org/lkml/08f1f185-e259-4014-9ca4-6411d5c1bc65@marcan.st/
> "Perf broke on all Apple ARM64 systems (tested almost everything), and
> according to maz also on Juno (so, probably all big.LITTLE) since
> v6.5."
> 6) sysfs/json events were made the priority over legacy to unbreak
> perf on Apple-M? CPUs, but only if the PMU is specified:
> https://lore.kernel.org/r/20231123042922.834425-1-irogers@google.com
>    Reported-by: Hector Martin <marcan@marcan.st>
>    Signed-off-by: Ian Rogers <irogers@google.com>
>    Tested-by: Hector Martin <marcan@marcan.st>
>    Tested-by: Marc Zyngier <maz@kernel.org>
>    Acked-by: Mark Rutland <mark.rutland@arm.com>

I think ARM/Apple-Mx is fine without this change, right?

> 
> This gets us to the current code where I can trivially get an
> inconsistency. Here on Intel with no PMU in the event name:
> ```
> $ perf stat -vv -e cpu-cycles true
> Using CPUID GenuineIntel-6-8D-1
> Control descriptor is not initialized
> ------------------------------------------------------------
> perf_event_attr:
>   type                             0 (PERF_TYPE_HARDWARE)
>   size                             136
>   config                           0 (PERF_COUNT_HW_CPU_CYCLES)
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   enable_on_exec                   1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid 752915  cpu -1  group_fd -1  flags 0x8 = 3
> cpu-cycles: -1: 1293076 273429 273429
> cpu-cycles: 1293076 273429 273429
> 
>  Performance counter stats for 'true':
> 
>          1,293,076      cpu-cycles
> 
>        0.000809752 seconds time elapsed
> 
>        0.000841000 seconds user
>        0.000000000 seconds sys
> ```
> 
> Here with a PMU event name:
> ```
> $ sudo perf stat -vv -e cpu/cpu-cycles/ true
> Using CPUID GenuineIntel-6-8D-1
> Attempt to add: cpu/cpu-cycles=0/
> ..after resolving event: cpu/event=0x3c/
> Control descriptor is not initialized
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu)
>   size                             136
>   config                           0x3c (cpu-cycles)
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   enable_on_exec                   1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid 752839  cpu -1  group_fd -1  flags 0x8 = 3
> cpu/cpu-cycles/: -1: 1421235 531150 531150
> cpu/cpu-cycles/: 1421235 531150 531150
> 
>  Performance counter stats for 'true':
> 
>          1,421,235      cpu/cpu-cycles/
> 
>        0.001292908 seconds time elapsed
> 
>        0.001340000 seconds user
>        0.000000000 seconds sys
> ```
> 
> That is the no PMU event is opened as type=0/config=0 (legacy) while
> the PMU event is opened as type=4/config=0x3c (sysfs encoding). Now

I'm not sure it's a problem.  I think it works as expected...?


> let's cross our fingers and hope that in the driver they are really
> the same thing. I take objection to the idea that there should be two
> different priorities for sysfs/json and legacy depending on whether a
> PMU is or isn't specified in the event name. The priority could be
> legacy then sysfs/json, or it could be sysfs/json then legacy, but it
> should be the same regardless of whether the PMU is put in the event

Well, I think having PMU name in the event is a big difference.  Legacy
events were there since Day 1, I guess it's natural to think that an
event without PMU name means a legacy event and others should come with
PMU names explicitly.

Thanks,
Namhyung


> name. The PMU in the event name should be optional, for example we may
> or may not show it in the stat output. The encoding being consistent
> was the case prior to the Apple-M? fix and this patch aims to make it
> consistent once more. Given the ARM bug mentioned above it should also
> fix specifying or not the PMU on Apple-M? CPUs as it will avoid the
> same legacy event issue that may only exist on old kernels. RISC-V is
> motivated because of not wanting hard coded legacy events in the
> driver for all potential vendors and models.
> 
> Thanks,
> Ian