On Wed, Jan 29, 2025 at 10:03:03PM -0800, Ian Rogers wrote: > On Wed, Jan 29, 2025 at 9:16 PM Namhyung Kim <namhyung@xxxxxxxxxx> 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@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: > > > > > > 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@xxxxxxxxxx > > > 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@xxxxxxxxxxxx/ > > > > 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? Because I think 'event' and 'pmu/event/' are different and it's natural that 'event' (w/o PMU) to use the legacy encoding. Maybe ARM Apple M? has broken implementation, but then they should fix it. IIRC what they wanted was to pick sysfs encoding when they use the 'pmu/event/' format. And that's perfectly fine. > 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. Right, so it should use full 'pmu/event/' format. > > 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. I don't think raw event accepts legacy encoding. Also I don't want the additional legacy-* PMUs. Maybe what I want is to remove surprises - it'd be confusing if I have two events (w/o PMU) and one is using legacy encoding and the other is using sysfs because (core?) PMU has an alias. > > 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. Is Apple M* currently broken? I'm not sure if we won't ever want the legacy encoding. > > > > > > > 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. That's great! > > 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. I cannot agree. I think 'event' and 'pmu/event/' are different. And we need to clarify how to handle 'event' case correctly. And I hope we can disable the wildcard behavior. Thanks, Namhyung
