[REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Krzysztof Kozlowski]

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

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Ravi Bangoria]

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/20230525212723.3361524-1-oliver.upton@linux.dev

Thanks,
Ravi

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Krzysztof Kozlowski]

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/20230525212723.3361524-1-oliver.upton@linux.dev


It's not a KVM guest. It's regular board as listed at the end of the mail.

Best regards,
Krzysztof

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Mark Rutland]

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

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Peter Zijlstra]

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.

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Peter Zijlstra]

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.

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Ravi Bangoria]

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

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Ravi Bangoria]

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

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Mark Rutland]

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.

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Ravi Bangoria]

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

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Peter Zijlstra]

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 <peterz@infradead.org>
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 <nathan@kernel.org>
Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 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);
 

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Krzysztof Kozlowski]

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 <peterz@infradead.org>
> 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 <nathan@kernel.org>
> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks.

Best regards,
Krzysztof

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Mark Rutland]

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 <peterz@infradead.org>
> 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 <nathan@kernel.org>
> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

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);
>  

Re: [REGRESSION][BISECT] perf/core: Remove pmu linear searching code

[Linux regression tracking #update (Thorsten Leemhuis)]

[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.