Message-ID: <CAP-5=fXHuR37Q-1qhZx_wLeSTh6k-T1DrV0EquDuLEpwnHa21A@mail.gmail.com>
Date: Wed, 29 Jan 2025 22:03:03 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@...aro.org>, Ze Gao <zegao2021@...il.com>, 
	Weilin Wang <weilin.wang@...el.com>, Dominique Martinet <asmadeus@...ewreck.org>, 
	Jean-Philippe Romain <jean-philippe.romain@...s.st.com>, Junhao He <hejunhao3@...wei.com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org, 
	bpf@...r.kernel.org, Aditya Bodkhe <Aditya.Bodkhe1@....com>, 
	Atish Patra <atishp@...osinc.com>, Leo Yan <leo.yan@....com>, 
	Beeman Strong <beeman@...osinc.com>, Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v5 4/4] perf parse-events: Reapply "Prefer sysfs/JSON
 hardware events over legacy"

On Wed, Jan 29, 2025 at 9:16 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, Jan 29, 2025 at 05:16:58PM -0800, Ian Rogers wrote:
> > On Wed, Jan 29, 2025 at 1:55 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > > On Wed, Jan 15, 2025 at 01:20:32PM -0800, Ian Rogers wrote:
> > > > On Wed, Jan 15, 2025 at 9:59 AM Namhyung Kim <namhyung@...nel.org> wrote:
> > > > > I think the behavior should be:
> > > > >
> > > > >   cycles -> PERF_COUNT_HW_CPU_CYCLES
> > > > >   cpu-cycles -> PERF_COUNT_HW_CPU_CYCLES
> > > > >   cpu_cycles -> no legacy -> sysfs or json
> > > > >   cpu/cycles/ -> sysfs or json
> > > > >   cpu/cpu-cycles/ -> sysfs or json
> > > >
> > > > So I disagree as if you add a PMU to an event name the encoding
> > > > shouldn't change:
> > > > 1) This historically was perf's behavior.
> > >
> > > Well.. I'm not sure about the history.  I believe the logic I said above
> > > is the historic and (I think) right behavior.
> >
> > You're wrong as you are describing the behavior post:
> > https://lore.kernel.org/r/20231123042922.834425-1-irogers@google.com
> > commit a24d9d9dc096fc0d0bd85302c9a4fe4fe3b1107b from Nov 2022, but
> > somehow without legacy event fall backs which Intel added with a PMU
> > for hybrid.
> >
> > The behavior in this patch series is best for RISC-V, presumably ARM
> > (particularly for Apple M? CPUs), carries ARM and Intel's tags,
> > implements the behavior Arnaldo asked for, and solves the
> > inconsistency that I think is fundamentally wrong in the tool that PMU
> > names shouldn't matter on an event name (an inconsistency my past
> > fixes introduced). It is also part of solving other problems:
> > https://lore.kernel.org/linux-perf-users/20250127-counter_delegation-v3-0-64894d7e16d5@rivosinc.com/
>
> So you think the below behavior is preferred, right?
>
>   cycles -> cpu/cycles/ (or whatever PMU name) -> sysfs or json
>
> And there's no way to use legacy event encodings anymore?

This is absolutely the right thing to do! If sysfs/json knows better
than to allow a legacy event named cycles, advertises it, then perf
should select it. Not doing this was the cause of the ARM Apple M?
breakage - because their PMUs looked uncore before hybrid fixes and so
weren't known previously to accept legacy events and always used the
sysfs/json encodings in preference. Why would or not having the PMU in
the event name imply a different and sometimes known broken encoding?
And then in the perf stat uniquification we can rename the event to be
the version with a different encoding. It is madness to me.

If a user wants to force a legacy event, even though most typically
the driver is saying it knows better, they can use a raw event
encoding or in the case of cycles its alias cpu-cycles. If there
really is a use-case for using legacy encodings, we could introduce
new legacy-cpu and legacy-cache PMUs that advertise the events, but
then the wildcard behavior would be weird.

To be clear, I do not know of a single use-case where the legacy
encodings are actually wanted when sysfs/json have an encoding. The
opposite is very much true, that legacy encodings are not wanted -
hence wanting the lowering of their priority everywhere originally by
ARM to fix Apple M? and then by RISC-V.

> >
> > You've not pointed at anything wrong in the scheme that these patches
> > introduce, and are supported by vendors, except that it is a behavior
> > change. I can, and have, pointed at many issues with your proposal
> > above and the current behavior. The behavior change came about to work
> > around PMU bugs over 2 years ago but only partially did so. It makes
> > sense to remedy this and for the clean, consistent behavior this
> > series achieves. It is unfortunate that it is a behavior change, but
> > the first step for that was made 2 years ago. I think it also makes
> > sense that something self described as legacy is a lower priority and
> > of the past (wrt event naming moving forward).
>
> I want to clarify the event parsing behavior and to find the right way
> to deal with various cases.  I haven't followed the activities in this
> area closely so I missed some changes in the past.  Maybe the problem
> is that the behavior is complex and not clarified.  Hopefully we can
> write it down in a doc.

I think what is typical in the kernel is the source is the best
documentation. By simplifying event parsing, for example,
parse-events.y has been reduced from 952 lines (in v5.10) to 762 lines
- so we're about 25% simpler whilst being more correct (I've fixed all
the memory leaks, etc.) and avoiding expensive start-up costs, lazy
initialization, etc.

Having a single priority for which events are preferred, legacy vs
sysfs/json with or without PMU, will further make the code base
simpler and easy to understand.

Thanks,
Ian

Powered by blists - more mailing lists