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

[Namhyung Kim]

On Sat, Feb 01, 2025 at 12:45:04AM -0800, Ian Rogers wrote:
> On Fri, Jan 31, 2025 at 2:42 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 29, 2025 at 10:12:14PM -0800, Atish Kumar Patra wrote:
> > > On Wed, Jan 29, 2025 at 1:55 PM Namhyung Kim <namhyung@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > > On Mon, Jan 13, 2025 at 2:51 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > > > > > > There was an explicit, and reviewed by Jiri and Arnaldo, intent with
> > > > > > > > the hybrid work that using a legacy event with a hybrid PMU, even
> > > > > > > > though the PMU doesn't advertise through json or sysfs the legacy
> > > > > > > > event, the perf tool supports it.
> > > > > >
> > > > > > I thought legacy events on hybrid were converted to PMU events.
> > > > >
> > > > > No, when BIG.little was created nothing changed in perf events but
> > > > > when Intel did hybrid they wanted to make the hybrid CPUs (atom and
> > > > > performance) appear as if they were one type. The PMU event encodings
> > > > > vary a lot for this on Intel, ARM has standards for the encoding.
> > > > > Intel extended the legacy format to take a PMU type id:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/uapi/linux/perf_event.h?h=perf-tools-next#n41
> > > > > "EEEEEEEE: PMU type ID"
> > > > > that is in the top 32-bits of the config.
> > > >
> > > > Oh right, I forgot the extended type thing.  Then we can keep the legacy
> > > > encoding with it on hybrid systems when users give well-known names (w/o
> > > > PMU) for legacy event.
> > > >
> > > > >
> > > > > > > >
> > > > > > > > Making it so that events without PMUs are only legacy events just
> > > > > > > > doesn't work. There are far too many existing uses of non-legacy
> > > > > > > > events without PMU, the metrics contain 100s of examples.
> > > > > >
> > > > > > That's unfortunate.  It'd be nice if metrics were written with PMU
> > > > > > names.
> > > > >
> > > > > But then we'd end up with things like on Intel:
> > > > > UNC_CHA_TOR_OCCUPANCY.IA_MISS_DRD
> > > > > becoming:
> > > > > uncore_cha/UNC_CHA_TOR_OCCUPANCY.IA_MISS_DRD/
> > > > > or just:
> > > > > cha/UNC_CHA_TOR_OCCUPANCY.IA_MISS_DRD/
> > > > > As a user the first works for me and doesn't have any ambiguity over
> > > > > PMUs as the event name already encodes the PMU. AMD similarly place
> > > > > the part of a pipeline into event names. Were we to break everybody by
> > > > > requiring the PMU we'd also need to explain which PMU to use. Sites
> > > > > with event lists (like https://perfmon-events.intel.com/) don't
> > > > > explain the PMU and it'd be messy as on Intel you have a CHA PMU for
> > > > > server chips but a CBOX on client chips, etc.
> > > >
> > > > While I prefer having PMU names in the JSON events/metrics, it may not
> > > > be pratical to change them all.  Probably we can allow them without PMU
> > > > and hope that they have unique prefixes.
> > > >
> > > > >
> > > > > > I have a question.  What if an event name in a metric matches to
> > > > > > multiple unrelated PMUs?
> > > > >
> > > > > The metric may break or we'd aggregate the unrelated counts together.
> > > >
> > > > Ok, then they should use unique names.
> > > >
> > > >
> > > > > Take a metric like IPC as "instructions/cycles", that metric should
> > > > > work on a hybrid system as they have instructions and cycles. If you
> > > > > used an event for instructions like inst_retired.any then maybe the
> > > > > metric will fail on one kind of core that didn't have that event. Now
> > > >
> > > > The metrics is for specific CPU model then the vendor should be
> > > > responsible to provide accurate metrics using approapriate PMU/events
> > > > IMHO.
> > > >
> > > >
> > > > > if we have accelerators advertising instructions and cycles events, we
> > > > > should be able to compute the metric for the accelerator. What could
> > > > > happen today is that the accelerator will have a cpumask of a single
> > > > > CPU, we could aggregate the accelerator counter into the CPU event
> > > > > with the same CPU as the cpumask, we'd end up with a weird quasi CPU
> > > > > and accelerator IPC metric for that CPU. What should happen is that we
> > > > > get an IPC for the accelerator and IPC for each hybrid core
> > > > > independently, but the way we handle evsels, CPUs, PMUs is not really
> > > > > set up for that. Hopefully getting a set of PMUs into the evsel will
> > > > > clear that up. Assuming all of that is cleared up, is it wrong if the
> > > > > IPC metric is computed for the accelerator if it was originally
> > > > > written as a CPU metric? Not really. Could there be metrics where that
> > > > > is the case?
> > > >
> > > > Yes, I think there should be separate metrics for the accelerators.
> > > >
> > > >
> > > > > Probably, and specifying PMUs in the event names would be
> > > > > a fix. There have also been proposals that we restrict the PMUs for
> > > > > certain metrics. As event names are currently so distinct it isn't a
> > > > > problem we've faced yet and it is not clear it is a problem other than
> > > > > highlighting tech debt in areas of the tool like aggregation.
> > > > >
> > > > > > > >
> > > > > > > > Prior to switching json/sysfs to being the priority when a PMU is
> > > > > > > > specified, it was the case that all encodings were the same, with or
> > > > > > > > without a PMU.
> > > > > > > >
> > > > > > > > I don't think there is anything natural about assuming things about
> > > > > > > > event names. Take cycles, cpu-cycles and cpu_cycles:
> > > > > > > >  - cycles on x86 is only encoded via a legacy event;
> > > > > > > >  - cpu-cycles on Intel exists as a sysfs event, but cpu-cycles is also
> > > > > > > > a legacy event name;
> > > > > > > >  - cpu_cycles exists as a sysfs event on ARM but doesn't have a
> > > > > > > > corresponding legacy event name.
> > > > > >
> > > > > > 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.
> > > >
> > > > > 2) Different event encodings can have different behaviors (broken in
> > > > > some notable cases).
> > > >
> > > > Yep, let's make it clear.
> > > >
> > > > > 3) Intuitively what wildcarding does is try to open "*/event/" where *
> > > > > is every possible PMU name. Having different event encodings is
> > > > > breaking that intuition it could also break situations where you try
> > > > > to assert equivalence based on type/config.
> > > >
> > > > While I don't like the wildcard matching, I think it doesn't matter as
> > > > long as we keep the above behavior.  If it can find a legacy name, then
> > > > go with it, done.  If not, try all PMUs as if it's given with PMU name
> > > > in the event.
> > > >
> > > > > 4) The legacy encodings were (are?) broken on ARM Apple M? CPUs,
> > > > > that's why the priority was changed.
> > > >
> > > > I guess that why they use cpu_cycles.
> > > >
> > > > > 5) RISC-V would like the tool tackle the legacy to config mapping
> > > > > challenge, rather than the PMU driver, given the potential diversity
> > > > > of hardware implementations.
> > > >
> > > > I hope they can find a better solution. :)
> > > >
> > >
> > > Sorry for reposing. Gmail converted it to html for some reason.
> > >
> > > I have posted the latest support here.
> > > https://lore.kernel.org/kvm/20250127-counter_delegation-v3-12-64894d7e16d5@xxxxxxxxxxxx/T/
> > >
> > > As of now, we have adopted a hybrid approach where a vendor can decide
> > > whether to encode the legacy events
> > > in the json or in the driver (if this series is merged). In absence of
> > > that, every vendor has to define it in the driver.
> > > We will deal with the fall out of the exploding driver when the
> > > situation arrives.
> >
> > I don't know how hard it'd be cause I'm not familiar with RISC-V.  But
> > basically you only need to maintain 9 legacy encodings (PERF_COUNT_HW_*)
> > and a few dozen combinations of supported cache events (PERF_COUNT_HW_
> > CACHE_*) for each vendor.  All others can go to json anyway.
> >
> > I think this is what all other archs (including x86) do.
> 
> This is well known to the people involved.
> 
> While the PMU driver needs to encode or avoid these event names, they
> become special "legacy" names inside the perf tool. Magically a name
> like cpu_cycles will wildcard match (match on >1 PMU) whilst a name
> like cpu-cycles won't (only matching on core PMUs). This is completely
> confusing to users. It is even more confusing when you are saying the
> tool should intentionally use two different encodings.

The legacy encoding is a part of the ABI, and it's natural to use it.
We historically used 'cycles' and 'cpu-cycles' as legacy events and it
should remain as is IMHO.  I'm not sure why ARM uses 'cpu_cycles', but
I guess they don't want to use the legacy encoding for some reason.

> 
> The perf event enum types are limited but the tool recognizes more
> event names and then uses legacy encodings. I have yet to hear a
> sensible list of what are legacy event names, is cpu-cycles in there
> or just cycles? Why on earth would you want to keep synonyms like LLC
> meaning L2 cache?

I think it's clear what are legacy events: `perf list hw`.

In fact, it doesn't matter for tools what LLC means.  I think it's the
drivers' respensibility to match sensible events to legacy encoding.
We only need to use the event as they prepared.

> 
> The intention with "pmu syntax" for events is that the PMU clarifies
> the type in the perf_event_attr. Previously it was assumed that the
> PMU type would be raw (4), and the x86 PMUs even use that as their
> type number. Pretending these days we don't now have hybrid core PMUs,
> 10s of uncore PMUs. Doing that work had to reinvent event parsing and
> encoding.
> 
> If you look at the matching as it is today:
> cpu_cycles -> tries to match on all PMUs
> */cpu_cycles/ -> tries to match on all PMUs
> arm*/cpu_cycles/ -> tries matches on all PMUs that have arm at the start
> armv8_pmuv3/cpu_cycles/ -> matches only the armv8_pmuv3 PMU

I didn't realize we can use '*'.  Then I guess we can disable the
default wildcard match.  Users can add '*/.../' easily if they really
want it, right?  I still think all of this problem comes from the
wildcard behavior.

Probably we need to do these for events without PMU name:
1. use legacy event if it's the well-known name, if not
2. check core PMU (cpu) for sysfs events, if not
3. search all JSON events (not to break metrics)

> 
> I don't see why it isn't obvious that the behavior of no PMU and the
> PMU being * is expected to be exactly the same - it really is today
> and that is what the code does, please try it. There just isn't a
> notion of not having a PMU because even for legacy events we have to
> reinvent the PMUs to inject the correct extended type information
> otherwise we'd profile just a fraction of the cores. We add PMUs when
> we display events to make the events more readable. There isn't a
> notion of these events being legacy and not, they are just assumed to
> be the same, PMU or not.

Yes, it's confusing.  So I think we'd better make cycles != */cycles/.

> 
> As I've explained to you, I plan to transition the metric code to use
> event parsing and to union evlists rather than use strings and hash
> tables. This is to fix tracepoints appearing incorrectly to always
> have suffixes in the "metric-id" calculation. Recognizing modifiers
> properly would end up reinventing event parsing, so let's just make
> use of what we have and parse events early. It makes sense when
> unioning evsels in an evlist to do it off of the perf_event_attr, this
> will allow Intel's slots and topdown.slots to be correctly detected as
> aliases in metrics, something of a pain in formulas today. Why would
> the behavior of an event like cycles be different in non-hybrid
> metrics (where PMUs generally aren't specified) and in hybrid metrics
> (where PMUs generally are specified)? Events may not be recognized as
> aliases because ones without a PMU in the metric will get a legacy
> encoding. In your change:
> https://lore.kernel.org/r/20221018020227.85905-16-namhyung@xxxxxxxxxx
> you assume all events with the same name are in fact the same event,
> but that is making wild assumptions about what is placed in the evsel
> name and I am trying to fix it in:
> https://lore.kernel.org/lkml/20250201074320.746259-1-irogers@xxxxxxxxxx/
> You did similar with your proposal for hwmon events and I rejected it.
> The fact that the name term in an event configuration clobbers an
> evsel's name, its just the intent of the thing and the name was never
> supposed to have some sacred legacy or whatever meaning.

Thanks for the fix!

> 
> I still see no sense in:
> perf stat -e cpu_cycles ...
> meaning:
> perf stat -e */cpu_cycles/ ...
> and:
> perf stat -e cpu-cycles ...
> trying to mean close to:
> perf stat -e cpu/cpu-cycles/ ...
> why one is implicitly a * and the other a core PMU, I mean it is the
> definition of confusing. And in the latter cpu-cycles case you want
> those two events to be encoded differently.

Yep, I agree it's confusing.  So my opinion is to use legacy encoding
and no default wildcard. :)

> 
> All of this is overlooking that we have 1 event that is a problem on 1
> PMU on 1 architecture. If it weren't for that event we'd already have
> this patch landed and consistent event encodings. By not taking the
> patch it hurts Apple M, RISC-V users and my own work.

Well, I'm not talking about the specific event or an architecture.  What
I'm focusing on is what the sensible behavior is.

> 
> Please can you explain why keeping the current encoding is good and if
> we like legacy events so much, can we revert the changes to prioritize
> sysfs/json when a PMU name is present. I'm afraid what you are
> explaining makes no sense to me, breaks existing platforms (Apple M)
> and is a blockage to future work. Saying everyone should rewrite
> everything, that's not a workable solution - not least because in some
> situations (old PMU drivers on Apple M) we lack a time machine.

It's not clear to me if we have a problem on Apple M as of now.

And I don't have a problem with 'pmu/event/' case.  I hope to find a way
to support what I described without rewriting all metrics.

Thanks,
Namhyung