Re: [PATCH] perf/arm-cmn: Fix DTC reset
From: Robin Murphy
Date: Fri Apr 14 2023 - 10:19:45 EST
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@xxxxxxxxxx>
Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
---
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