Re: [REGRESSION] Perf (userspace) broken on big.LITTLE systems since v6.5

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Wed, Aug 14, 2024 at 05:28:42PM +0100, James Clark wrote:
> 
> 
> On 07/08/2024 9:54 am, Thorsten Leemhuis wrote:
> > On 01.08.24 21:05, Ian Rogers wrote:
> > > On Wed, Dec 6, 2023 at 4:09 AM Linux regression tracking #update
> > > (Thorsten Leemhuis) <regressions@leemhuis.info> wrote:
> > > > 
> > > > [TLDR: This mail in primarily relevant for Linux kernel regression
> > > > tracking. See link in footer if these mails annoy you.]
> > > > 
> > > > On 22.11.23 00:43, Bagas Sanjaya wrote:
> > > > > On Tue, Nov 21, 2023 at 09:08:48PM +0900, Hector Martin wrote:
> > > > > > 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.
> > > > 
> > > > #regzbot fix: perf parse-events: Make legacy events lower priority than
> > > > sysfs/JSON
> > > > #regzbot ignore-activity
> > > 
> > > Note, this is still broken.
> > 
> > Hmmm, so all that became somewhat messy. Arnaldo, what's the way out of
> > this? Or is this a "we are screwed one way or another and someone has to
> > bite the bullet" situation?
> > 
> > Ciao, Thorsten
> > 
> > > The patch changed the priority in the case
> > > that you do something like:
> > > 
> > > $ perf stat -e 'armv8_pmuv3_0/cycles/' benchmark
> > > 
> > > but if you do:
> > > 
> > > $ perf stat -e 'cycles' benchmark
> > > 
> > > then the broken behavior will happen as legacy events have priority
> > > over sysfs/json events in that case. To fix this you need to revert:
> > > 4f1b067359ac Revert "perf parse-events: Prefer sysfs/JSON hardware
> > > events over legacy"
> > > 
> > > This causes some testing issues resolved in this unmerged patch series:
> > > https://lore.kernel.org/lkml/20240510053705.2462258-1-irogers@google.com/
> > > 
> > > There is a bug as the arm_dsu PMU advertises an event called "cycles"
> > > and this PMU is present on Ampere systems. Reverting the commit above
> > > will cause an issue as the commit 7b100989b4f6 ("perf evlist: Remove
> > > __evlist__add_default") to fix ARM's BIG.little systems (opening a
> > > cycles event on all PMUs not just 1) will cause the arm_dsu event to
> > > be opened by perf record and fail as the event won't support sampling.
> > > 
> > > The patch https://lore.kernel.org/lkml/20240525152927.665498-1-irogers@google.com/
> > > fixes this by only opening the cycles event on core PMUs when choosing
> > > default events.
> > > 
> > > Rather than take this patch the revert happened as Linus runs the
> > > command "perf record -e cycles:pp" (ie using a specified event and not
> > > defaults) and considers it a regression in the perf tool that on an
> > > Ampere system to need to do "perf record -e
> > > 'armv8_pmuv3_0/cycles/pp'". It was pointed out that not specifying -e
> > > will choose the cycles event correctly and with better precision the
> > > pp for systems that support it, but it was still considered a
> > > regression in the perf tool so the revert was made to happen. There is
> > > a lack of perf testing coverage for ARM, in particular as they choose
> > > to do everything in a different way to x86. The patch in question was
> > > in the linux-next tree for weeks without issues.
> > > 
> > > ARM/Ampere could fix this by renaming the event from cycles to
> > > cpu_cycles, or by following Intel's convention that anything uncore
> > > uses the name clockticks rather than cycles. This could break people
> > > who rely on an event called arm_dsu/cycles/ but I imagine such people
> > > are rare. There has been no progress I'm aware of on renaming the
> > > event.
> > > 
> > > Making perf not terminate on opening an event for perf record seems
> > > like the most likely workaround as that is at least something under
> > > the tool maintainers control. ARM have discussed doing this on the
> > > lists:
> > > https://lore.kernel.org/lkml/f30f676e-a1d7-4d6b-94c1-3bdbd1448887@arm.com/
> > > but since the revert in v6.10 no patches have appeared for the v6.11
> > > merge window. Feature work like coresight improvements and ARMv9 are
> > > being actively pursued by ARM, but feature work won't resolve this
> > > regression.
> > > 
> 
> I got some hardware with the DSU PMU so I'm going to have a go at trying to
> send some fixes for this. My initial idea was to try incorporate the "not
> terminate on opening" change as discussed in the link directly above. And
> then do the revert of the "revert of prefer sysfs/json".
> 
> FWIW I don't think Juno currently is broken if the kernel supports extended
> type ID? I could have missed some output in this thread but it seems like
> it's mostly related to Apple M hardware. I'm also a bit confused why the
> "supports extended type" check fails there, but maybe the v6.9 commit
> 25412c036 from Mark is missing?
> 
> I sent a small fix the other day to make perf stat default arguments work on
> Juno, and didn't notice anything out of the ordinary: https://lore.kernel.org/linux-perf-users/dac6ad1d-5aca-48b4-9dcb-ff7e54ca43f6@linaro.org/T/#t
> I agree that change is quite narrow but it does incrementally improve things
> for the time being. It's possible that it would become redundant if I can
> just include Ian's change to use strings for Perf stat.
> 
> Of course I only think I have a handle on the issue right now, seems like it
> has a lot of moving parts and something else always comes up. If I hit a
> wall at some point I will come back here.

Thanks for working on this, hopefully we'll get to a solution that keeps
all the expectations expressed in this thread about not breaking
existing muscle memory and that allows us to progress on this matter.

- Arnaldo