On Thu, Jan 09, 2025 at 02:21:05PM -0800, Ian Rogers wrote: > At the RISC-V summit the topic of avoiding event data being in the > RISC-V PMU kernel driver came up. There is a preference for sysfs/JSON > events being the priority when no PMU is provided so that legacy > events maybe supported via json. Originally Mark Rutland also > expressed at LPC 2023 that doing this would resolve bugs on ARM Apple > M? processors, but James Clark more recently tested this and believes > the driver issues there may not have existed or have been resolved. In > any case, it is inconsistent that with a PMU event names avoid legacy > encodings, but when wildcarding PMUs (ie without a PMU with the event > name) the legacy encodings have priority. > > The patch doing this work was reverted in a v6.10 release candidate > as, even though the patch was posted for weeks and had been on > linux-next for weeks without issue, Linus was in the habit of using > explicit legacy events with unsupported precision options on his > Neoverse-N1. This machine has SLC PMU events for bus and CPU cycles > where ARM decided to call the events bus_cycles and cycles, the latter > being also a legacy event name. ARM haven't renamed the cycles event > to a more consistent cpu_cycles and avoided the problem. With these > changes the problematic event will now be skipped, a large warning > produced, and perf record will continue for the other PMU events. This > solution was proposed by Arnaldo. > > Two minor changes have been added to help with the error message and > to work around issues occurring with "perf stat metrics (shadow stat) > test". > > The patches have only been tested on my x86 non-hybrid laptop. > > v5: Follow Namhyung's suggestion and ignore the case where command > line dummy events fail to open alongside other events that all > fail to open. Note, the Tested-by tags are left on the series as > v4 and v5 were changing an error case that doesn't occur in > testing but was manually tested by myself. > > v4: Rework the no events opening change from v3 to make it handle > multiple dummy events. Sadly an evlist isn't empty if it just > contains dummy events as the dummy event may be used with "perf > record -e dummy .." as a way to determine whether permission > issues exist. Other software events like cpu-clock would suffice > for this, but the using dummy genie has left the bottle. > > Another problem is that we appear to have an excessive number of > dummy events added, for example, we can likely avoid a dummy event > and add sideband data to the original event. For auxtrace more > dummy events may be opened too. Anyway, this has led to the > approach taken in patch 3 where the number of dummy parsed events > is computed. If the number of removed/failing-to-open non-dummy > events matches the number of non-dummy events then we want to > fail, but only if there are no parsed dummy events or if there was > one then it must have opened. The math here is hard to read, but > passes my manual testing. > > v3: Make no events opening for perf record a failure as suggested by > James Clark and Aditya Bodkhe <Aditya.Bodkhe1@xxxxxxx>. Also, > rebase. > > v2: Rebase and add tested-by tags from James Clark, Leo Yan and Atish > Patra who have tested on RISC-V and ARM CPUs, including the > problem case from before. > > Ian Rogers (4): > perf evsel: Add pmu_name helper > perf stat: Fix find_stat for mixed legacy/non-legacy events I think the first two are quite independent. I'll take them first. Thanks, Namhyung > perf record: Skip don't fail for events that don't open > perf parse-events: Reapply "Prefer sysfs/JSON hardware events over > legacy" > > tools/perf/builtin-record.c | 47 ++++++++++++++++++--- > tools/perf/util/evsel.c | 10 +++++ > tools/perf/util/evsel.h | 1 + > tools/perf/util/parse-events.c | 26 +++++++++--- > tools/perf/util/parse-events.l | 76 +++++++++++++++++----------------- > tools/perf/util/parse-events.y | 60 ++++++++++++++++++--------- > tools/perf/util/pmus.c | 20 +++++++-- > tools/perf/util/stat-shadow.c | 3 +- > 8 files changed, 169 insertions(+), 74 deletions(-) > > -- > 2.47.1.613.gc27f4b7a9f-goog >
