[PATCH] perf/arm-cmn: Fix DTC reset

[PATCH] perf/arm-cmn: Fix DTC reset

[Robin Murphy]

It turns out that my naive DTC reset logic fails to work as intended,
since clearing PMCR.PMU_EN appears to result in writes to PMOVSR_CLR
being ignored, while some hard-to-characterise combination of conditions
(differently between DTC0 and secondary DTCs) also appears to result in
PMOVSR reading as zero even when an overflow remains asserted. Thus
rather than resetting the PMU to a nice clean state, we can currently
end up with screaming spurious interrupts from secondary DTCs which we
can neither see nor clear. This behaviour is of course not documented.

Resetting PMCR to disable the interrupt output but enable the PMU itself
seems to at least make the PMOVSR_CLR write work as expected on DTC0
(although it looks like writing to PMCR twice has actually been having
some hidden side-effect of clearing any pending overflows there).
Unfortunately this still does not seem to help secondary DTCs, but going
beyond PMU scope and additionally resetting DTC_CTL does seems to make
everything work out, and superficially looks sensible. Therefore pile
that onto the house of empirical cards too, until I can check with the
hardware team whether there's actually any proper recommended way of
recovering from an arbitrary PMU state after an oops/kexec/whatever.

Fixes: 0ba64770a2f2 ("perf: Add Arm CMN-600 PMU driver")
Reported-by: Geoff Blake <blakgeof@amazon.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
This supersedes the previous shutdown/IRQ patches, now that I've
finally managed to make *some* sense of what's really going on. If
anyone's interested, this is the contrivance I used for testing:

https://gitlab.arm.com/linux-arm/linux-rm/-/commit/d8f1035c5bc510516d6e4f0b7bf0b875a749daf7
---
 drivers/perf/arm-cmn.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 144cc08d9e04..81fe01171e33 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1899,7 +1899,10 @@ static int arm_cmn_init_dtc(struct arm_cmn *cmn, struct arm_cmn_node *dn, int id
 	if (dtc->irq < 0)
 		return dtc->irq;
 
-	writel_relaxed(0, dtc->base + CMN_DT_PMCR);
+	if (idx == 0)
+		writel_relaxed(0, dtc->base + CMN_DT_DTC_CTL);
+
+	writel_relaxed(CMN_DT_PMCR_PMU_EN, dtc->base + CMN_DT_PMCR);
 	writel_relaxed(0x1ff, dtc->base + CMN_DT_PMOVSR_CLR);
 	writel_relaxed(CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR);
 
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH] perf/arm-cmn: Fix DTC reset

[Geoff Blake]

Ran this patch on an AWS C6g.metal and unfortunately still see the 
spurious IRQs trigger quickly (within 10 tries) when using the following 
flow:

perf stat -a -e arm_cmn_0/event=0x5,type=0x5/ -- sleep 600
kexec -e 

Adding in the simple shutdown routine, I have run over 100 of the 
above cycles and the spurious IRQs haven't triggered.  I think we still 
need both for now.

-Geoff  

On Thu, 6 Apr 2023, Robin Murphy wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> It turns out that my naive DTC reset logic fails to work as intended,
> since clearing PMCR.PMU_EN appears to result in writes to PMOVSR_CLR
> being ignored, while some hard-to-characterise combination of conditions
> (differently between DTC0 and secondary DTCs) also appears to result in
> PMOVSR reading as zero even when an overflow remains asserted. Thus
> rather than resetting the PMU to a nice clean state, we can currently
> end up with screaming spurious interrupts from secondary DTCs which we
> can neither see nor clear. This behaviour is of course not documented.
> 
> Resetting PMCR to disable the interrupt output but enable the PMU itself
> seems to at least make the PMOVSR_CLR write work as expected on DTC0
> (although it looks like writing to PMCR twice has actually been having
> some hidden side-effect of clearing any pending overflows there).
> Unfortunately this still does not seem to help secondary DTCs, but going
> beyond PMU scope and additionally resetting DTC_CTL does seems to make
> everything work out, and superficially looks sensible. Therefore pile
> that onto the house of empirical cards too, until I can check with the
> hardware team whether there's actually any proper recommended way of
> recovering from an arbitrary PMU state after an oops/kexec/whatever.
> 
> Fixes: 0ba64770a2f2 ("perf: Add Arm CMN-600 PMU driver")
> Reported-by: Geoff Blake <blakgeof@amazon.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> This supersedes the previous shutdown/IRQ patches, now that I've
> finally managed to make *some* sense of what's really going on. If
> anyone's interested, this is the contrivance I used for testing:
> 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/d8f1035c5bc510516d6e4f0b7bf0b875a749daf7
> ---
>  drivers/perf/arm-cmn.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 144cc08d9e04..81fe01171e33 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1899,7 +1899,10 @@ static int arm_cmn_init_dtc(struct arm_cmn *cmn, struct arm_cmn_node *dn, int id
>         if (dtc->irq < 0)
>                 return dtc->irq;
> 
> -       writel_relaxed(0, dtc->base + CMN_DT_PMCR);
> +       if (idx == 0)
> +               writel_relaxed(0, dtc->base + CMN_DT_DTC_CTL);
> +
> +       writel_relaxed(CMN_DT_PMCR_PMU_EN, dtc->base + CMN_DT_PMCR);
>         writel_relaxed(0x1ff, dtc->base + CMN_DT_PMOVSR_CLR);
>         writel_relaxed(CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR);
> 
> --
> 2.39.2.101.g768bb238c484.dirty
> 
> 

Re: [PATCH] perf/arm-cmn: Fix DTC reset

[Will Deacon]

Hi Geoff,

On Thu, Apr 06, 2023 at 04:25:39PM -0500, Geoff Blake wrote:
> Ran this patch on an AWS C6g.metal and unfortunately still see the 
> spurious IRQs trigger quickly (within 10 tries) when using the following 
> flow:
> 
> perf stat -a -e arm_cmn_0/event=0x5,type=0x5/ -- sleep 600
> kexec -e 
> 
> Adding in the simple shutdown routine, I have run over 100 of the 
> above cycles and the spurious IRQs haven't triggered.  I think we still 
> need both for now.

Does that mean you're happy for me to queue this change as-is?

Will

RE: [PATCH] perf/arm-cmn: Fix DTC reset

[Geoff Blake]

Sorry for the confusion Will, but no, the patch does not solve the issue, 
the spurious interrupt storm still exists.  The patch needs the 
additional shutdown routine Robin had in his older set.

Geoff

Re: [PATCH] perf/arm-cmn: Fix DTC reset

[Robin Murphy]

On 2023-04-06 22:25, Geoff Blake wrote:
> Ran this patch on an AWS C6g.metal and unfortunately still see the
> spurious IRQs trigger quickly (within 10 tries) when using the following
> flow:
> 
> perf stat -a -e arm_cmn_0/event=0x5,type=0x5/ -- sleep 600
> kexec -e
> 
> Adding in the simple shutdown routine, I have run over 100 of the
> above cycles and the spurious IRQs haven't triggered.  I think we still
> need both for now.

There is no "need both" - if this patch doesn't work to reset the PMU as 
intended then we still need a better patch that does. After yet more 
trying, I still cannot reproduce your results, but I do suspect this 
patch isn't as good as it initially seemed.

I got my hands on a C6g.metal instance, and I'm building the mainline 
version of arm-cmn.c from my cmn-dev branch (including the two other 
pending fixes that I've sent recently) against the 5.15.0-1031-aws 
kernel that it came with, as a standalone module with a trivial 
makefile. Even running "stress -m 60" in the background, as the most 
effective thing I've found so far, that hnf_pocq_reqs_recvd event takes 
well over 8 minutes to overflow, so I have failed to achieve the 
necessary timing to kexec at just the right point for the residual 
interconnect traffic to add up and overflow the event during the handful 
of seconds that the kexec takes. For completeness, I have managed to run 
the perf stat/kexec, then run stress for 10 minutes under the new 
kernel, *then* finally load the module to achieve the right conditions, 
but that's so utterly contrived and long-winded that I don't really have 
the patience to do it more than the twice that I already did.

What I can do instantly and reliably is reproduce equivalent conditions 
with my (now even more stripped-down) remove hack[1] and a simple 
rmmod/insmod (with a few seconds in between for good measure), leading 
to demonstrable latent overflows on all 4 DTCs every time. The existing 
code does seem to manage to reset DTC0 such that its interrupt (IRQ 27) 
does not fire, consistent with what I've observed on other machines, 
while I see the secondary DTCs (IRQs 28, 29 and 30) each fire 100000 
times spuriously and get disabled. With this patch on top[2], that 
consistently does not happen over 100 unload/reload cycles.

Given that you say the same write to clear DTC_CTL, but a few seconds 
earlier in the form of the shutdown hook, does seem to work, I have 
still been wary of some kind of weird timing issue all along, but the 
fact that I was getting such consistent behaviour even on C6g seemed to 
be pointing away from that :/

The closest I've got so far is by leaving this even more involved test 
loop (with real PMU programming in between) running overnight:

for i in {1..10000}; do sudo insmod arm-cmn.ko && sudo perf stat -e 
arm_cmn_0/eventid=5,type=5/ sleep 1 && sudo rmmod arm-cmn && sleep 4; done

and now coming back to find /proc/interrupts saying this:

  27:          1          0          0...
  28:          1          0          0...
  29:          2          0          0...
  30:          1          0          0...

I've quite often seen a single IRQ firing earlier than expected (not 
necessarily spuriously), so I still need to check what's up with that - 
it may be that writing to the counters doesn't always take either. 
However, the single extra incidence of IRQ 29 which has happened at some 
point after I went home is more of a smoking gun:

[84581.790043] WARNING: CPU: 0 PID: 0 at /home/ubuntu/arm-cmn.c:1828 
arm_cmn_handle_irq+0x148/0x1cc [arm_cmn]

So something still snuck through reset, but it *was* at least visible 
and clearable by the time the IRQ was enabled. Interestingly the other 
warning for !dtc->cycles did not fire at the same time, even though the 
hack normally overflows PMCCNTR before PMEVCNTR(0). I'll keep digging...

Thanks,
Robin.

[1] 
https://gitlab.arm.com/linux-arm/linux-rm/-/commit/d3bdc783411fd71d5948ce7c7e7fa6cc6b388b6c
[2] 
https://gitlab.arm.com/linux-arm/linux-rm/-/commit/c742e8be1a783430151828ed27287ad3d61ff9d1

> 
> -Geoff
> 
> On Thu, 6 Apr 2023, Robin Murphy wrote:
> 
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> It turns out that my naive DTC reset logic fails to work as intended,
>> since clearing PMCR.PMU_EN appears to result in writes to PMOVSR_CLR
>> being ignored, while some hard-to-characterise combination of conditions
>> (differently between DTC0 and secondary DTCs) also appears to result in
>> PMOVSR reading as zero even when an overflow remains asserted. Thus
>> rather than resetting the PMU to a nice clean state, we can currently
>> end up with screaming spurious interrupts from secondary DTCs which we
>> can neither see nor clear. This behaviour is of course not documented.
>>
>> Resetting PMCR to disable the interrupt output but enable the PMU itself
>> seems to at least make the PMOVSR_CLR write work as expected on DTC0
>> (although it looks like writing to PMCR twice has actually been having
>> some hidden side-effect of clearing any pending overflows there).
>> Unfortunately this still does not seem to help secondary DTCs, but going
>> beyond PMU scope and additionally resetting DTC_CTL does seems to make
>> everything work out, and superficially looks sensible. Therefore pile
>> that onto the house of empirical cards too, until I can check with the
>> hardware team whether there's actually any proper recommended way of
>> recovering from an arbitrary PMU state after an oops/kexec/whatever.
>>
>> Fixes: 0ba64770a2f2 ("perf: Add Arm CMN-600 PMU driver")
>> Reported-by: Geoff Blake <blakgeof@amazon.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> This supersedes the previous shutdown/IRQ patches, now that I've
>> finally managed to make *some* sense of what's really going on. If
>> anyone's interested, this is the contrivance I used for testing:
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/d8f1035c5bc510516d6e4f0b7bf0b875a749daf7
>> ---
>>   drivers/perf/arm-cmn.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index 144cc08d9e04..81fe01171e33 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -1899,7 +1899,10 @@ static int arm_cmn_init_dtc(struct arm_cmn *cmn, struct arm_cmn_node *dn, int id
>>          if (dtc->irq < 0)
>>                  return dtc->irq;
>>
>> -       writel_relaxed(0, dtc->base + CMN_DT_PMCR);
>> +       if (idx == 0)
>> +               writel_relaxed(0, dtc->base + CMN_DT_DTC_CTL);
>> +
>> +       writel_relaxed(CMN_DT_PMCR_PMU_EN, dtc->base + CMN_DT_PMCR);
>>          writel_relaxed(0x1ff, dtc->base + CMN_DT_PMOVSR_CLR);
>>          writel_relaxed(CMN_DT_PMCR_OVFL_INTR_EN, dtc->base + CMN_DT_PMCR);
>>
>> --
>> 2.39.2.101.g768bb238c484.dirty
>>
>>

Re: [PATCH] perf/arm-cmn: Fix DTC reset

[Robin Murphy]

On 14/04/2023 3:19 pm, Robin Murphy wrote:
> On 2023-04-06 22:25, Geoff Blake wrote:
>> Ran this patch on an AWS C6g.metal and unfortunately still see the
>> spurious IRQs trigger quickly (within 10 tries) when using the following
>> flow:
>>
>> perf stat -a -e arm_cmn_0/event=0x5,type=0x5/ -- sleep 600
>> kexec -e
>>
>> Adding in the simple shutdown routine, I have run over 100 of the
>> above cycles and the spurious IRQs haven't triggered.  I think we still
>> need both for now.
> 
> There is no "need both" - if this patch doesn't work to reset the PMU as 
> intended then we still need a better patch that does. After yet more 
> trying, I still cannot reproduce your results, but I do suspect this 
> patch isn't as good as it initially seemed.
> 
> I got my hands on a C6g.metal instance, and I'm building the mainline 
> version of arm-cmn.c from my cmn-dev branch (including the two other 
> pending fixes that I've sent recently) against the 5.15.0-1031-aws 
> kernel that it came with, as a standalone module with a trivial 
> makefile. Even running "stress -m 60" in the background, as the most 
> effective thing I've found so far, that hnf_pocq_reqs_recvd event takes 
> well over 8 minutes to overflow, so I have failed to achieve the 
> necessary timing to kexec at just the right point for the residual 
> interconnect traffic to add up and overflow the event during the handful 
> of seconds that the kexec takes. For completeness, I have managed to run 
> the perf stat/kexec, then run stress for 10 minutes under the new 
> kernel, *then* finally load the module to achieve the right conditions, 
> but that's so utterly contrived and long-winded that I don't really have 
> the patience to do it more than the twice that I already did.
> 
> What I can do instantly and reliably is reproduce equivalent conditions 
> with my (now even more stripped-down) remove hack[1] and a simple 
> rmmod/insmod (with a few seconds in between for good measure), leading 
> to demonstrable latent overflows on all 4 DTCs every time. The existing 
> code does seem to manage to reset DTC0 such that its interrupt (IRQ 27) 
> does not fire, consistent with what I've observed on other machines, 
> while I see the secondary DTCs (IRQs 28, 29 and 30) each fire 100000 
> times spuriously and get disabled. With this patch on top[2], that 
> consistently does not happen over 100 unload/reload cycles.
> 
> Given that you say the same write to clear DTC_CTL, but a few seconds 
> earlier in the form of the shutdown hook, does seem to work, I have 
> still been wary of some kind of weird timing issue all along, but the 
> fact that I was getting such consistent behaviour even on C6g seemed to 
> be pointing away from that :/
> 
> The closest I've got so far is by leaving this even more involved test 
> loop (with real PMU programming in between) running overnight:
> 
> for i in {1..10000}; do sudo insmod arm-cmn.ko && sudo perf stat -e 
> arm_cmn_0/eventid=5,type=5/ sleep 1 && sudo rmmod arm-cmn && sleep 4; done
> 
> and now coming back to find /proc/interrupts saying this:
> 
>   27:          1          0          0...
>   28:          1          0          0...
>   29:          2          0          0...
>   30:          1          0          0...
> 
> I've quite often seen a single IRQ firing earlier than expected (not 
> necessarily spuriously), so I still need to check what's up with that - 
> it may be that writing to the counters doesn't always take either. 
> However, the single extra incidence of IRQ 29 which has happened at some 
> point after I went home is more of a smoking gun:
> 
> [84581.790043] WARNING: CPU: 0 PID: 0 at /home/ubuntu/arm-cmn.c:1828 
> arm_cmn_handle_irq+0x148/0x1cc [arm_cmn]
> 
> So something still snuck through reset, but it *was* at least visible 
> and clearable by the time the IRQ was enabled. Interestingly the other 
> warning for !dtc->cycles did not fire at the same time, even though the 
> hack normally overflows PMCCNTR before PMEVCNTR(0). I'll keep digging...

I realise I neglected to follow up on this - where I got to was adding 
an extra read back of CMN_DTC_CTL after the write, tweaking my remove 
hack to generate overflows even more reliably for good measure, and that 
then ran for ~56,000 test cycles (until my time on the instance ran out) 
without any stray interrupts at all. However I wasn't keen on posting 
that as a v2 without any better justification than it being a "add 
random things to change the timing a bit" bodge. Since we're in the 
merge window now, I'll see if I can get a better answer by -rc1.

Thanks,
Robin.