Message-ID: <351904c9-d661-46bc-94cf-b6cea8371dad@linaro.org>
Date: Thu, 15 Aug 2024 16:20:22 +0100
From: James Clark <james.clark@...aro.org>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Thorsten Leemhuis <regressions@...mhuis.info>,
 Arnaldo Carvalho de Melo <acme@...hat.com>, Ian Rogers <irogers@...gle.com>,
 Mark Rutland <mark.rutland@....com>,
 Linux perf Profiling <linux-perf-users@...r.kernel.org>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 James Clark <james.clark@....com>, "cc: Marc Zyngier" <maz@...nel.org>,
 Hector Martin <marcan@...can.st>, Asahi Linux <asahi@...ts.linux.dev>,
 Linux regressions mailing list <regressions@...ts.linux.dev>
Subject: Re: [REGRESSION] Perf (userspace) broken on big.LITTLE systems since
 v6.5



On 15/08/2024 4:15 pm, James Clark wrote:
> 
> 
> On 14/08/2024 5:41 pm, Arnaldo Carvalho de Melo wrote:
>> 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@...mhuis.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
> 
> Hi Arnaldo,
> 
> In one of your investigations here 
> https://lore.kernel.org/lkml/Zld3dlJHjFMFG02v@x1/ comparing "cycles", 
> "cpu-cycles" and "cpu_cycles" events on Arm you say only some of them 
> open events on both core types. I wasn't able to reproduce that on 
> perf-tools-next (27ac597c0e) or v6.9 (a38297e3fb) for perf record or 
> stat. I guessed the 6.9 tag because you only mentioned it was on tip and 
> it was 29th May. For me they all open exactly the same two legacy events 
> with the extended type ID set.

Minor correction, one opens using the PMU type rather a legacy event 
with extended type ID. But importantly they do all open on both CPU types.

Powered by blists - more mailing lists