Hi,
#regzbot introduced: 9551fbb64d09
Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
searching code") as first one where all hardware events are gone from
perf for ARMv7 Exynos5422 board.
Expected:
perf list | grep "Hardware event"
branch-instructions OR branches [Hardware event]
branch-misses [Hardware event]
bus-cycles [Hardware event]
cache-misses [Hardware event]
cache-references [Hardware event]
cpu-cycles OR cycles [Hardware event]
instructions [Hardware event]
Result:
empty, no hardware events
1. Arch ARM Linux
2. exynos_defconfig
3. Board:
- Odroid HC1
ARMv7, octa-core (Cortex-A7+A15), Exynos5422 SoC
Systemd: v239
4. perf version 6.1.0
Best regards,
Krzysztof
On Sun, Jun 04, 2023 at 01:38:10PM +0200, Krzysztof Kozlowski wrote:
> Hi,
>
> #regzbot introduced: 9551fbb64d09
>
> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code") as first one where all hardware events are gone from
> perf for ARMv7 Exynos5422 board.
Fixing up regzbot entry title:
#regzbot title: All Exynos5422 hardware events are gone from perf due to removing pmu linear searching code
Thanks.
--
An old man doll... just what I always wanted! - Clara
On 04-Jun-23 5:08 PM, Krzysztof Kozlowski wrote:
> Hi,
>
> #regzbot introduced: 9551fbb64d09
>
> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code") as first one where all hardware events are gone from
> perf for ARMv7 Exynos5422 board.
Is this inside KVM guest? Does this help:
https://lore.kernel.org/all/[email protected]
Thanks,
Ravi
On 05/06/2023 04:59, Ravi Bangoria wrote:
> On 04-Jun-23 5:08 PM, Krzysztof Kozlowski wrote:
>> Hi,
>>
>> #regzbot introduced: 9551fbb64d09
>>
>> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
>> searching code") as first one where all hardware events are gone from
>> perf for ARMv7 Exynos5422 board.
>
> Is this inside KVM guest? Does this help:
> https://lore.kernel.org/all/[email protected]
It's not a KVM guest. It's regular board as listed at the end of the mail.
Best regards,
Krzysztof
On Sun, Jun 04, 2023 at 01:38:10PM +0200, Krzysztof Kozlowski wrote:
> Hi,
>
> #regzbot introduced: 9551fbb64d09
>
> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code") as first one where all hardware events are gone from
> perf for ARMv7 Exynos5422 board.
I think that commit 9551fbb64d09 is just wrong.
The commit message asserts:
Searching for the right pmu by iterating over all pmus is no longer
required since all pmus now *must* be present in the 'pmu_idr' list.
So, remove linear searching code.
... and while each PMU has *some* entry in the pmu_idr list, for its dynamic
type, that means that events with other types (e.g. PERF_TYPE_HARDWARE or
PERF_TYPE_RAW) will fail to find a PMU in the IDR whereas they'd previously
have been accepted by a PMU during the subsequent iteration over all PMUs.
So that iteration is still required.
How does this work on x86? Do you have pseudo-PMUs for PERF_TYPE_HARDWARE and
PERF_TYPE_RAW ?
Thanks,
Mark.
> Expected:
> perf list | grep "Hardware event"
> branch-instructions OR branches [Hardware event]
> branch-misses [Hardware event]
> bus-cycles [Hardware event]
> cache-misses [Hardware event]
> cache-references [Hardware event]
> cpu-cycles OR cycles [Hardware event]
> instructions [Hardware event]
>
>
> Result:
> empty, no hardware events
>
> 1. Arch ARM Linux
> 2. exynos_defconfig
> 3. Board:
> - Odroid HC1
> ARMv7, octa-core (Cortex-A7+A15), Exynos5422 SoC
> Systemd: v239
>
> 4. perf version 6.1.0
>
>
> Best regards,
> Krzysztof
On 05-Jun-23 12:40 PM, Mark Rutland wrote:
> On Sun, Jun 04, 2023 at 01:38:10PM +0200, Krzysztof Kozlowski wrote:
>> Hi,
>>
>> #regzbot introduced: 9551fbb64d09
>>
>> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
>> searching code") as first one where all hardware events are gone from
>> perf for ARMv7 Exynos5422 board.
>
> I think that commit 9551fbb64d09 is just wrong.
>
> The commit message asserts:
>
> Searching for the right pmu by iterating over all pmus is no longer
> required since all pmus now *must* be present in the 'pmu_idr' list.
> So, remove linear searching code.
>
> ... and while each PMU has *some* entry in the pmu_idr list, for its dynamic
> type, that means that events with other types (e.g. PERF_TYPE_HARDWARE or
> PERF_TYPE_RAW) will fail to find a PMU in the IDR whereas they'd previously
> have been accepted by a PMU during the subsequent iteration over all PMUs.
Not sure I follow.
PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE are aliased to PERF_TYPE_RAW in
perf_init_event(). And PERF_TYPE_RAW should be present in pmu_idr if it
was registered using:
perf_pmu_register(pmu, "name", PERF_TYPE_RAW);
In fact, all static pmu types (enum perf_type_id) are also added to pmu_idr.
See (type >= 0) checks in perf_pmu_register() code:
if (type >= 0)
max = type;
ret = idr_alloc(&pmu_idr, pmu, max, 0, GFP_KERNEL);
if (ret < 0)
goto free_pdc;
WARN_ON(type >= 0 && ret != type);
Thanks,
Ravi
On Mon, Jun 05, 2023 at 08:10:15AM +0100, Mark Rutland wrote:
> How does this work on x86? Do you have pseudo-PMUs for PERF_TYPE_HARDWARE and
> PERF_TYPE_RAW ?
Generic code maps TYPE_HARDWARE and TYPE_HW_CACHE to TYPE_RAW for a
first go, only if that fails it will try the actual type.
And x86 has the (first) CPU PMU has TYPE_RAW, on hybrid, it will
transparently pick the right actual PMU.
That said; given that this commit has been tagged twice now, I suppose I
should go revert it and we'll try again after a more thorough audit.
On Mon, Jun 05, 2023 at 03:04:45PM +0530, Ravi Bangoria wrote:
> On 05-Jun-23 12:40 PM, Mark Rutland wrote:
> > On Sun, Jun 04, 2023 at 01:38:10PM +0200, Krzysztof Kozlowski wrote:
> >> Hi,
> >>
> >> #regzbot introduced: 9551fbb64d09
> >>
> >> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
> >> searching code") as first one where all hardware events are gone from
> >> perf for ARMv7 Exynos5422 board.
> >
> > I think that commit 9551fbb64d09 is just wrong.
> >
> > The commit message asserts:
> >
> > Searching for the right pmu by iterating over all pmus is no longer
> > required since all pmus now *must* be present in the 'pmu_idr' list.
> > So, remove linear searching code.
> >
> > ... and while each PMU has *some* entry in the pmu_idr list, for its dynamic
> > type, that means that events with other types (e.g. PERF_TYPE_HARDWARE or
> > PERF_TYPE_RAW) will fail to find a PMU in the IDR whereas they'd previously
> > have been accepted by a PMU during the subsequent iteration over all PMUs.
>
> Not sure I follow.
>
> PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE are aliased to PERF_TYPE_RAW in
> perf_init_event(). And PERF_TYPE_RAW should be present in pmu_idr if it
> was registered using:
>
> perf_pmu_register(pmu, "name", PERF_TYPE_RAW);
As I said, the PMUs get registered with a dynamic type, and there's no
registration with PERF_TYPE_RAW. On arm/arm64 systems, *every* CPU PMU gets
registered with:
perf_pmu_register(pmu, name, -1);
... and *none* are registered with:
perf_pmu_register(pmu, name, PERF_TYPE_RAW)
... so those all get a dynamic IDR type, and nothing gets placed into the IDR
for PERF_TYPE_RAW, etc. We rely on the linear search to find a PMU that can
handle PERF_TYPE_RAW, etc.
I appreciate that's not ideal, but it's how it has worked for almost a decade
now, so we can't change the userspace-visible behaviour.
Thanks,
Mark.
On Mon, Jun 05, 2023 at 11:27:31AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 05, 2023 at 08:10:15AM +0100, Mark Rutland wrote:
>
> > How does this work on x86? Do you have pseudo-PMUs for PERF_TYPE_HARDWARE and
> > PERF_TYPE_RAW ?
>
> Generic code maps TYPE_HARDWARE and TYPE_HW_CACHE to TYPE_RAW for a
> first go, only if that fails it will try the actual type.
>
> And x86 has the (first) CPU PMU has TYPE_RAW, on hybrid, it will
> transparently pick the right actual PMU.
Oh, I might've misread that last bit.. TYPE_RAW is always the big core,
it will not magic the thing. If you want little events you gotta
manually find the little pmu.
On 05-Jun-23 3:04 PM, Peter Zijlstra wrote:
> On Mon, Jun 05, 2023 at 11:27:31AM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 05, 2023 at 08:10:15AM +0100, Mark Rutland wrote:
>>
>>> How does this work on x86? Do you have pseudo-PMUs for PERF_TYPE_HARDWARE and
>>> PERF_TYPE_RAW ?
>>
>> Generic code maps TYPE_HARDWARE and TYPE_HW_CACHE to TYPE_RAW for a
>> first go, only if that fails it will try the actual type.
>>
>> And x86 has the (first) CPU PMU has TYPE_RAW, on hybrid, it will
>> transparently pick the right actual PMU.
>
> Oh, I might've misread that last bit.. TYPE_RAW is always the big core,
> it will not magic the thing. If you want little events you gotta
> manually find the little pmu.
"manually find the little pmu" ... which was being done by linear searching?
Thanks,
Ravi
On 05-Jun-23 3:24 PM, Mark Rutland wrote:
> On Mon, Jun 05, 2023 at 03:04:45PM +0530, Ravi Bangoria wrote:
>> On 05-Jun-23 12:40 PM, Mark Rutland wrote:
>>> On Sun, Jun 04, 2023 at 01:38:10PM +0200, Krzysztof Kozlowski wrote:
>>>> Hi,
>>>>
>>>> #regzbot introduced: 9551fbb64d09
>>>>
>>>> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
>>>> searching code") as first one where all hardware events are gone from
>>>> perf for ARMv7 Exynos5422 board.
>>>
>>> I think that commit 9551fbb64d09 is just wrong.
>>>
>>> The commit message asserts:
>>>
>>> Searching for the right pmu by iterating over all pmus is no longer
>>> required since all pmus now *must* be present in the 'pmu_idr' list.
>>> So, remove linear searching code.
>>>
>>> ... and while each PMU has *some* entry in the pmu_idr list, for its dynamic
>>> type, that means that events with other types (e.g. PERF_TYPE_HARDWARE or
>>> PERF_TYPE_RAW) will fail to find a PMU in the IDR whereas they'd previously
>>> have been accepted by a PMU during the subsequent iteration over all PMUs.
>>
>> Not sure I follow.
>>
>> PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE are aliased to PERF_TYPE_RAW in
>> perf_init_event(). And PERF_TYPE_RAW should be present in pmu_idr if it
>> was registered using:
>>
>> perf_pmu_register(pmu, "name", PERF_TYPE_RAW);
>
> As I said, the PMUs get registered with a dynamic type, and there's no
> registration with PERF_TYPE_RAW. On arm/arm64 systems, *every* CPU PMU gets
> registered with:
>
> perf_pmu_register(pmu, name, -1);
>
> ... and *none* are registered with:
>
> perf_pmu_register(pmu, name, PERF_TYPE_RAW)
>
> ... so those all get a dynamic IDR type, and nothing gets placed into the IDR
> for PERF_TYPE_RAW, etc. We rely on the linear search to find a PMU that can
> handle PERF_TYPE_RAW, etc.
Got it. Thanks for the clarification.
Ravi
On 05/06/2023 12:14, Peter Zijlstra wrote:
> On Mon, Jun 05, 2023 at 11:27:31AM +0200, Peter Zijlstra wrote:
>
>> That said; given that this commit has been tagged twice now, I suppose I
>> should go revert it and we'll try again after a more thorough audit.
>
> I'll go queue this then...
>
> ---
> Subject: perf: Re-instate the linear PMU search
> From: Peter Zijlstra <[email protected]>
> Date: Mon Jun 5 11:42:39 CEST 2023
>
> Full revert of commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code").
>
> Some architectures (notably arm/arm64) still relied on the linear
> search in order to find the PMU that consumes
> PERF_TYPE_{HARDWARE,HW_CACHE,RAW}.
>
> This will need a more thorought audit and clean.
>
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Krzysztof Kozlowski <[email protected]>
Thanks.
Best regards,
Krzysztof
On Mon, Jun 05, 2023 at 11:27:31AM +0200, Peter Zijlstra wrote:
> That said; given that this commit has been tagged twice now, I suppose I
> should go revert it and we'll try again after a more thorough audit.
I'll go queue this then...
---
Subject: perf: Re-instate the linear PMU search
From: Peter Zijlstra <[email protected]>
Date: Mon Jun 5 11:42:39 CEST 2023
Full revert of commit 9551fbb64d09 ("perf/core: Remove pmu linear
searching code").
Some architectures (notably arm/arm64) still relied on the linear
search in order to find the PMU that consumes
PERF_TYPE_{HARDWARE,HW_CACHE,RAW}.
This will need a more thorought audit and clean.
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
kernel/events/core.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11630,27 +11630,38 @@ static struct pmu *perf_init_event(struc
}
again:
- ret = -ENOENT;
rcu_read_lock();
pmu = idr_find(&pmu_idr, type);
rcu_read_unlock();
- if (!pmu)
- goto fail;
+ if (pmu) {
+ if (event->attr.type != type && type != PERF_TYPE_RAW &&
+ !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
+ goto fail;
+
+ ret = perf_try_init_event(pmu, event);
+ if (ret == -ENOENT && event->attr.type != type && !extended_type) {
+ type = event->attr.type;
+ goto again;
+ }
- if (event->attr.type != type && type != PERF_TYPE_RAW &&
- !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
- goto fail;
-
- ret = perf_try_init_event(pmu, event);
- if (ret == -ENOENT && event->attr.type != type && !extended_type) {
- type = event->attr.type;
- goto again;
+ if (ret)
+ pmu = ERR_PTR(ret);
+
+ goto unlock;
}
+ list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
+ ret = perf_try_init_event(pmu, event);
+ if (!ret)
+ goto unlock;
+
+ if (ret != -ENOENT) {
+ pmu = ERR_PTR(ret);
+ goto unlock;
+ }
+ }
fail:
- if (ret)
- pmu = ERR_PTR(ret);
-
+ pmu = ERR_PTR(-ENOENT);
unlock:
srcu_read_unlock(&pmus_srcu, idx);
On Mon, Jun 05, 2023 at 12:14:01PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 05, 2023 at 11:27:31AM +0200, Peter Zijlstra wrote:
>
> > That said; given that this commit has been tagged twice now, I suppose I
> > should go revert it and we'll try again after a more thorough audit.
>
> I'll go queue this then...
>
> ---
> Subject: perf: Re-instate the linear PMU search
> From: Peter Zijlstra <[email protected]>
> Date: Mon Jun 5 11:42:39 CEST 2023
>
> Full revert of commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code").
>
> Some architectures (notably arm/arm64) still relied on the linear
> search in order to find the PMU that consumes
> PERF_TYPE_{HARDWARE,HW_CACHE,RAW}.
>
> This will need a more thorought audit and clean.
>
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Thanks for sorting this out!
Mark.
> ---
> kernel/events/core.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11630,27 +11630,38 @@ static struct pmu *perf_init_event(struc
> }
>
> again:
> - ret = -ENOENT;
> rcu_read_lock();
> pmu = idr_find(&pmu_idr, type);
> rcu_read_unlock();
> - if (!pmu)
> - goto fail;
> + if (pmu) {
> + if (event->attr.type != type && type != PERF_TYPE_RAW &&
> + !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
> + goto fail;
> +
> + ret = perf_try_init_event(pmu, event);
> + if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> + type = event->attr.type;
> + goto again;
> + }
>
> - if (event->attr.type != type && type != PERF_TYPE_RAW &&
> - !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
> - goto fail;
> -
> - ret = perf_try_init_event(pmu, event);
> - if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> - type = event->attr.type;
> - goto again;
> + if (ret)
> + pmu = ERR_PTR(ret);
> +
> + goto unlock;
> }
>
> + list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
> + ret = perf_try_init_event(pmu, event);
> + if (!ret)
> + goto unlock;
> +
> + if (ret != -ENOENT) {
> + pmu = ERR_PTR(ret);
> + goto unlock;
> + }
> + }
> fail:
> - if (ret)
> - pmu = ERR_PTR(ret);
> -
> + pmu = ERR_PTR(-ENOENT);
> unlock:
> srcu_read_unlock(&pmus_srcu, idx);
>
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 228020b490eda9133c9cb6f59a5ee1278d8c463f
Gitweb: https://git.kernel.org/tip/228020b490eda9133c9cb6f59a5ee1278d8c463f
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 05 Jun 2023 12:14:01 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Tue, 06 Jun 2023 10:09:21 +02:00
perf: Re-instate the linear PMU search
Full revert of commit 9551fbb64d09 ("perf/core: Remove pmu linear
searching code").
Some architectures (notably arm/arm64) still relied on the linear
search in order to find the PMU that consumes
PERF_TYPE_{HARDWARE,HW_CACHE,RAW}.
This will need a more thorought audit and cleanup.
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: Krzysztof Kozlowski <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/events/core.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 231b187..c01bbe9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11630,27 +11630,38 @@ static struct pmu *perf_init_event(struct perf_event *event)
}
again:
- ret = -ENOENT;
rcu_read_lock();
pmu = idr_find(&pmu_idr, type);
rcu_read_unlock();
- if (!pmu)
- goto fail;
+ if (pmu) {
+ if (event->attr.type != type && type != PERF_TYPE_RAW &&
+ !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
+ goto fail;
- if (event->attr.type != type && type != PERF_TYPE_RAW &&
- !(pmu->capabilities & PERF_PMU_CAP_EXTENDED_HW_TYPE))
- goto fail;
+ ret = perf_try_init_event(pmu, event);
+ if (ret == -ENOENT && event->attr.type != type && !extended_type) {
+ type = event->attr.type;
+ goto again;
+ }
- ret = perf_try_init_event(pmu, event);
- if (ret == -ENOENT && event->attr.type != type && !extended_type) {
- type = event->attr.type;
- goto again;
+ if (ret)
+ pmu = ERR_PTR(ret);
+
+ goto unlock;
}
-fail:
- if (ret)
- pmu = ERR_PTR(ret);
+ list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
+ ret = perf_try_init_event(pmu, event);
+ if (!ret)
+ goto unlock;
+ if (ret != -ENOENT) {
+ pmu = ERR_PTR(ret);
+ goto unlock;
+ }
+ }
+fail:
+ pmu = ERR_PTR(-ENOENT);
unlock:
srcu_read_unlock(&pmus_srcu, idx);
[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Closes: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]
On 04.06.23 13:38, Krzysztof Kozlowski wrote:
>
> #regzbot introduced: 9551fbb64d09
>
> Bisect pointed to commit 9551fbb64d09 ("perf/core: Remove pmu linear
> searching code") as first one where all hardware events are gone from
> perf for ARMv7 Exynos5422 board.
#regzbot fix: 228020b490eda91
#regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.