IssueTracker

Submitted again, as new issue here: https://issuetracker.google.com/issues/198449947

What I found most interesting was that just enabling the ARM64_PSEUDO_NMI compile time option but not the runtime option gave some performance improvement.

Maybe we could enable the Kconfig option for now and wait awhile before applying the rest and turning on the runtime option?

What I found most interesting was that just enabling the ARM64_PSEUDO_NMI compile time option but not the runtime option gave some performance improvement.

Is there any explanation for that? It would be good to understand why it gives an improvement so that we don't inadvertently disable some cpu vulnerability mitigation that we need enabled. If it truly is an improvement then I wonder why whatever the improvement is isn't always available.

If it truly is an improvement then I wonder why whatever the improvement is isn't always available.

It does seem weird. I think the only difference here is that it adds some static branches which I guess evaluate to a nop unless the command line parameter is provided? So the only way we'd get a performance improvement is if somehow these extra nops caused a speedup due to some weird interactions, maybe with cache lines?

I could certainly understand if irqchip.gicv3_pseudo_nmi=1 somehow caused a performance improvement. It seems plausible that masking using a different mechanism could be faster in some systems and slower in others just like how on some systems it might be faster to write "y = x * 3" and on others "y = x << 2 + 1". ...but that's not what you're seeing here...

camera_test_min_buffers_0.log is the full log from the camera test suite with crrev/c/3168489 applied.
camera_test_min_buffers_n.log is the full log for the run without the CL applied.

I'm looking at seeing if I can send out a v8 of Sumit's series. Maybe we can drum up support, or (if not) I guess we could think about landing FROMLIST while we wait...

Cool! There's also something [1] from MediaTek, though I haven't gone through it and am not sure how they might be connected.

[1] https://lore.kernel.org/linux-arm-kernel/20220903093415.15850-1-lecopzer.chen@mediatek.com/

Cool! There's also something [1] from MediaTek, though I haven't gone through it and am not sure how they might be connected.

Ah, thanks for the pointer! Looks like Mediatek's patch series is a continuation of the perf-based hard lockup detector. I believe that would work in tandem with the ability to get NMI backtraces. If we actually managed to upstream the buddy lockup detector in b/172213097 then we wouldn't need Mediatek's series. Similarly, if Mediatek's series landed then we could probably skip upstreaming the buddy detector.

There's also b/191948966 regarding the partitioned PMU interrupts not usable for NMIs. Not sure if that was ever solved upstream. I took a look at it back when, but it was quite involved. IIRC the NMI code in the GIC driver has since been rewritten. I can dig up whatever changes I have if you're interested.

There's also b/191948966 regarding the partitioned PMU interrupts not usable for NMIs

I presume the issues that were preventing this are fixed upstream, since the Mediatek patch series seems to re-land the previously-reverted "arm64: Enable perf events based hard lockup detector"

In any case, I've posted this at: https://lore.kernel.org/r/20230419225604.21204-1-dianders@chromium.org

...now we can see what people think! ;-) I included a summary in the cover letter trying to reconcile the various related series...

On Juniper running mainline, just enabling pseudo-NMIs causes a bad lockup:

[    3.047997] ================================
[    3.047998] WARNING: inconsistent lock state
[    3.048000] 6.3.0-next-20230428-14437-g7728edc2096a-dirty #628 Not tainted
[    3.048002] --------------------------------
[    3.048003] inconsistent {INITIAL USE} -> {IN-NMI} usage.
[    3.048005] swapper/4/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
[    3.048009] ffffff80ff52c358 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested+0x2c/0x50
[    3.048027] {INITIAL USE} state was registered at:
[    3.048028]   lock_acquire+0x1f4/0x340
[    3.048034]   _raw_spin_lock_nested+0x4c/0x70
[    3.048039]   raw_spin_rq_lock_nested+0x2c/0x50
[    3.048043]   rq_attach_root+0x48/0x240
[    3.048045]   sched_init+0x4d8/0x5e8
[    3.048051]   start_kernel+0x410/0x908
[    3.048054]   __primary_switched+0xbc/0xd0
[    3.048059] irq event stamp: 5004
[    3.048060] hardirqs last  enabled at (5003): [<ffffffeb96a45164>] psci_cpu_suspend_enter+0x19c/0x1f0
[    3.048066] hardirqs last disabled at (5004): [<ffffffeb96a45164>] psci_cpu_suspend_enter+0x19c/0x1f0
[    3.048070] softirqs last  enabled at (4844): [<ffffffeb960107ec>] __do_softirq+0x43c/0x548
[    3.048074] softirqs last disabled at (4833): [<ffffffeb96017a48>] ____do_softirq+0x18/0x30
[    3.048078] 
[    3.048078] other info that might help us debug this:
[    3.048079]  Possible unsafe locking scenario:
[    3.048079] 
[    3.048080]        CPU0
[    3.048080]        ----
[    3.048081]   lock(&rq->__lock);
[    3.048083]   <Interrupt>
[    3.048084]     lock(&rq->__lock);
[    3.048086] 
[    3.048086]  *** DEADLOCK ***
[    3.048086] 
[    3.048086] 1 lock held by swapper/4/0:
[    3.048088]  #0: ffffffeb9792f000 (rcu_read_lock){....}-{1:2}, at: cpu_pm_notify+0x8/0x140
[    3.048097] 
[    3.048097] stack backtrace:
[    3.048099] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 6.3.0-next-20230428-14437-g7728edc2096a-dirty #628 cfa3ce05a32b511c7b1da71d960a9262f60f029b
[    3.048103] Hardware name: Google juniper sku16 board (DT)
[    3.048105] Call trace:
[    3.048106]  dump_backtrace+0xa0/0x100
[    3.048109]  show_stack+0x20/0x38
[    3.048112]  dump_stack_lvl+0xdc/0x148
[    3.048116]  dump_stack+0x18/0x28
[    3.048118]  print_usage_bug.part.0+0x290/0x358
[    3.048122]  lock_acquire+0x2f4/0x340
[    3.048125]  _raw_spin_lock_nested+0x4c/0x70
[    3.048127]  raw_spin_rq_lock_nested+0x2c/0x50
[    3.048131]  sched_ttwu_pending+0x78/0x248
[    3.048133]  __flush_smp_call_function_queue+0xe4/0x2e8
[    3.048139]  generic_smp_call_function_single_interrupt+0x1c/0x30
[    3.048142]  ipi_handler+0x224/0x240
[    3.048147]  handle_percpu_devid_irq+0x94/0x158
[    3.048151]  generic_handle_domain_nmi+0x44/0x70
[    3.048156]  __gic_handle_nmi.constprop.0+0x58/0xb0
[    3.048161]  gic_handle_irq+0x284/0x2c8
[    3.048164]  call_on_irq_stack+0x24/0x58
[    3.048167]  do_interrupt_handler+0x88/0x98
[    3.048171]  el1_interrupt+0xb0/0xe8
[    3.048174]  el1h_64_irq_handler+0x18/0x28
[    3.048177]  el1h_64_irq+0x7c/0x80
[    3.048179]  gic_cpu_sys_reg_init+0x108/0x2b8
[    3.048183]  gic_cpu_pm_notifier+0x78/0xa8
[    3.048186]  notifier_call_chain+0xac/0x188
[    3.048189]  raw_notifier_call_chain+0x20/0x38
[    3.048191]  cpu_pm_notify+0x68/0x140
[    3.048194]  cpu_pm_exit+0x1c/0x30
[    3.048196]  psci_enter_idle_state+0x40/0x78
[    3.048200]  cpuidle_enter_state+0xe8/0x5a0
[    3.048203]  cpuidle_enter+0x40/0x60
[    3.048208]  do_idle+0x284/0x308
[    3.048210]  cpu_startup_entry+0x2c/0x40
[    3.048213]  secondary_start_kernel+0x160/0x1c8
[    3.048217]  __secondary_switched+0xb8/0xc0
[    3.048373] hub 1-1:1.0: USB hub found
[    3.048859] ------------[ cut here ]------------
[    3.048861] WARNING: CPU: 4 PID: 93 at arch/arm64/include/asm/irqflags.h:70 _raw_spin_lock_irq+0xb4/0xe8
[    3.048867] Modules linked in:
[    3.048870] CPU: 4 PID: 93 Comm: kworker/4:2 Not tainted 6.3.0-next-20230428-14437-g7728edc2096a-dirty #628 cfa3ce05a32b511c7b1da71d960a9262f60f029b
[    3.048874] Hardware name: Google juniper sku16 board (DT)
[    3.048877] Workqueue: usb_hub_wq hub_event
[    3.048882] pstate: 204000c5 (nzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    3.048885] pc : _raw_spin_lock_irq+0xb4/0xe8
[    3.048888] lr : __hrtimer_run_queues+0x1d0/0x3f0
[    3.048891] sp : ffffffc008023e50
[    3.048893] pmr_save: 000000f0
[    3.048894] x29: ffffffc008023e50 x28: ffffff80ff51d998 x27: ffffffeb97826d5c
[    3.048898] x26: ffffff80ff51d080 x25: ffffffeb9744d008 x24: 0000000000000001
[    3.048902] x23: 0000000000000000 x22: ffffff80ff51d110 x21: ffffffeb97823b60
[    3.048906] x20: ffffffeb9618db30 x19: ffffff80ff51d080 x18: ffffffffffffffff
[    3.048910] x17: ffffff95680ca000 x16: ffffffc008020000 x15: ffffffffffffffff
[    3.048913] x14: 0000000000000001 x13: fffffffffff88807 x12: ffffffffffffffff
[    3.048917] x11: 0000000000000040 x10: ffffff80c0013d58 x9 : ffffffeb9618db30
[    3.048921] x8 : 00000000a0b1b200 x7 : 7fffffffffffffff x6 : 00000000a0b1b200
[    3.048924] x5 : 00000000a0b1b200 x4 : ffffff80ff51d998 x3 : 00000000a0c0f440
[    3.048928] x2 : 00000000000000a0 x1 : 00000000000000f0 x0 : 0000000000000001
[    3.048931] Call trace:
[    3.048932]  _raw_spin_lock_irq+0xb4/0xe8
[    3.048935]  __hrtimer_run_queues+0x1d0/0x3f0
[    3.048937]  hrtimer_interrupt+0xf0/0x258
[    3.048940]  arch_timer_handler_phys+0x34/0x58
[    3.048944]  handle_percpu_devid_irq+0x94/0x158
[    3.048947]  generic_handle_domain_nmi+0x44/0x70
[    3.048951]  __gic_handle_nmi.constprop.0+0x58/0xb0
[    3.048954]  gic_handle_irq+0x284/0x2c8
[    3.048956]  call_on_irq_stack+0x24/0x58
[    3.048959]  do_interrupt_handler+0x88/0x98
[    3.048962]  el1_interrupt+0xb0/0xe8
[    3.048965]  el1h_64_irq_handler+0x18/0x28
[    3.048968]  el1h_64_irq+0x7c/0x80
[    3.048970]  vprintk_emit+0x2b0/0x3e0
[    3.048974]  dev_vprintk_emit+0xe0/0x1b0
[    3.048979]  dev_printk_emit+0x60/0x90
[    3.048982]  __dev_printk+0x44/0x98
[    3.048985]  _dev_info+0x68/0x98
[    3.048988]  hub_probe+0x13c/0xa18
[    3.048990]  usb_probe_interface+0xdc/0x290
[    3.048995]  really_probe+0x150/0x2c0
[    3.048998]  __driver_probe_device+0x80/0x140
[    3.049001]  driver_probe_device+0xe0/0x170
[    3.049003]  __device_attach_driver+0xc0/0x148
[    3.049005]  bus_for_each_drv+0x88/0xf0
[    3.049009]  __device_attach+0xa4/0x198
[    3.049012]  device_initial_probe+0x1c/0x30
[    3.049014]  bus_probe_device+0xb4/0xc0
[    3.049016]  device_add+0x5a8/0x778
[    3.049019]  usb_set_configuration+0x50c/0x8d0
[    3.049022]  usb_generic_driver_probe+0x78/0xd0
[    3.049026]  usb_probe_device+0x88/0x108
[    3.049029]  really_probe+0x150/0x2c0
[    3.049031]  __driver_probe_device+0x80/0x140
[    3.049033]  driver_probe_device+0xe0/0x170
[    3.049035]  __device_attach_driver+0xc0/0x148
[    3.049037]  bus_for_each_drv+0x88/0xf0
[    3.049041]  __device_attach+0xa4/0x198
[    3.049043]  device_initial_probe+0x1c/0x30
[    3.049045]  bus_probe_device+0xb4/0xc0
[    3.049047]  device_add+0x5a8/0x778
[    3.049050]  usb_new_device+0x24c/0x4b8
[    3.049052]  hub_event+0xedc/0x15f8
[    3.049054]  process_one_work+0x2c0/0x5b8
[    3.049059]  worker_thread+0x7c/0x458
[    3.049063]  kthread+0x128/0x138
[    3.049066]  ret_from_fork+0x10/0x20
[    3.049070] irq event stamp: 1860
[    3.049071] hardirqs last  enabled at (1859): [<ffffffeb96db5d18>] _raw_spin_unlock_irq+0x38/0xb0
[    3.049074] hardirqs last disabled at (1860): [<ffffffeb96dacd48>] __schedule+0x4c8/0xd30
[    3.049078] softirqs last  enabled at (1730): [<ffffffeb960107ec>] __do_softirq+0x43c/0x548
[    3.049081] softirqs last disabled at (1725): [<ffffffeb96017a48>] ____do_softirq+0x18/0x30
[    3.049084] ---[ end trace 0000000000000000 ]---

On Juniper running mainline, just enabling pseudo-NMIs causes a bad lockup:

That's super weird. So far it's been working fine for me on trogdor, but I'm not on kernel-next. I'll try to take a peek at this tomorrow or early next week and see if anything makes sense.

FWIW, I tried putting 6.3.0-next-20230505 on a trogdor device (even compiled with KASAN and lockdep) and I didn't see the same problem.

I then tried on kukui-kodama and I reproduced the same problem you did. Problem also reproduces on Linus's tree (v6.1-45701-g7163a2111f6c). I went back to vanilla kernel 5.15 and I still see the problem, so it's not new. Then I went back to vanilla v5.13 (plus crrev.com/c/3522894 and crrev.com/c/3522895 to make it compile) and I reproduced it there too. v5.13 is the first place kodama was supported upstream.

...I then tried chromeos-5.10 and tried turning on the option. It's also broken there on kukui.

Unless we really think this used to work, bisect doesn't seem like it'll help.

I've been poking at this a bunch, but it's pretty unfamiliar code to me. While researching, I found that Marc seems to have fixed a bunch of similar-sounding issues in the past, so adding him to CC on the off chance that he wants to offer any insight or has ideas of things to try. ;-)

As far as I can tell, the problem here is that somehow the system has decided that the IPI handler for calling a function on another CPU should be running as a NMI. That's obviously not right.

FWIW:

• CONFIG_ARM64_DEBUG_PRIORITY_MASKING=y didn't show anything.
• I don't know if it matters, but I did notice one difference between kukui (where things are broken) and trogdor (where things work):

kukui (doesn't work):

[    0.000000] GICv3: DOUG: has_group0(): 1, gic_dist_security_disabled(): 0

trogdor (works)

[    0.000000] GICv3: DOUG: has_group0(): 0, gic_dist_security_disabled(): 0

Could the "group0" difference be what's mattering here? I tried hacking has_group0() to false on kukui and kukui could boot, but pseudo-NMI didn't work...

FWIW, kevin (rk3999) matches kukui for at least those two properties, but works:

[    0.000000] GICv3: DOUG: has_group0(): 1, gic_dist_security_disabled(): 0

...so at least it's not just some generic problem with that config. Back to digging into what's different with kukui.

OK, so it looks as if somehow the GIC is losing state when CPUs go idle and that's messing things up. Evidence:

• I can "fix" things by just commenting out the cpu-idle-states in the device tree and then I don't get the crash and I also can use pseudo-NMI.
• I dropped into kgdb the moment I first got the unexpected pseudo-NMI. When I did, I found that some of the processor's GIC configuration had been reset to 0.

It looks like the GIC already gets involved when CPUs come out of idle, so something seems like it's wrong with gic_cpu_sys_reg_init(). Maybe it's somehow not re-initting things properly. I'll keep digging...

OK, so comment#27 seems to be correct and I have a fix. It looks like old versions of firmware didn't save/restore this register properly. See https://github.com/ARM-software/tf-issues/issues/464. I'm still trying to figure out exactly why kevin isn't having a problem because the patch that fixes this in ATF doesn't seem to be in kevin's firmware branch...

I ran out of time to today to do full testing, but I believe https://crrev.com/c/4519877 fixes us. I'll think more about it and possibly post it upstream tomorrow, hopefully.

Maybe on Rockchip the GIC never loses power, and thus retains its register state?

This really looks like a firmware issue. On RK3399, ATF is in charge of save/restoring the GIC state across PM events.

Hacking the kernel to save/restore priorities (or any other GIC distributor state) for that is unlikely to result in something that really works, as the kernel running in non-secure cannot configure the state of secure interrupts.

I have pushed back on this in the past, and will continue to do so.

A quick skim through ATF seems to indicate that MediaTek ATF is not saving/restoring the state of the redistributor (gicv3_rdistif_save() and gicv3_rdistif_init_restore()) are never called.

That alone would screw up all SGIs and PPIs. Good stuff... :-/

MTK saves/restores the state of the redistributor using their own implementation. (mt_gic_rdistif_save() and mt_gic_rdistif_restore())

@jason-ch, could you ask MTK GIC owner to take a look at this issue ?

FWIW: I've forked the ATF issue to the (also public) b/281831288

Hacking the kernel to save/restore priorities (or any other GIC distributor state) for that is unlikely to result in something that really works [...]

Marc: it sounds as if the Mediatek ATF is saving / restoring most of the GIC state, but just not these priorities. Presumably that means that it's configuring any state of secure interrupts that are important and we're just missing the priorities. Given that the kernel was the one that sets these priorities in the first place, it must have access to them, right? Would you object to a quirk to workaround this Mediatek issue? The problem is that even if we fix the Mediatek ATF implementation, anyone running with old firmware would continue to have this issue. In the very least, recovery images would need to continue to work and those always boot from read-only (non-updatable) firmware.

I'd also be interested if anyone has suggestions for how we should detect this issue. The kernel needs to be able to detect this so that it can either enable the workaround or disable pseudo-NMI. Certainly we could go the "dt" route, but it feels more reliable if we could query ATF directly. Maybe some PSCI call that tells us that we're on Mediatek today, and then we query some other function that currently returns 0. Then, when ATF is fixed we make that function return 1? I'll do a little digging to see if I see anything.

[I don't know which bug to follow anymore]

The minute I open the door to one such thing, I'll end-up with 20 variation of the same stuff, such as this horror:

https://lore.kernel.org/r/20230214233426.2994501-1-f.fainelli@gmail.com

So no, I'm not taking save/restore code for anything that is the responsibility of ATF and for which the architecture provides the adequate level of support (and when it doesn't, I'm happy to oblige, such as the collection replay on GIC500).

As for filtering the enablement, SOC_ID would be one way out, but nobody implements it. I'm not entertaining SOC-specific SMC calls, as if the vendor can be bothered to implement such a call, they can also implement SOC_ID. If you want to disable it for a whole SoC family, that's fine by me.

But it seems to me that you know exactly what platform you're running on (each board family has a separate kernel?), and you get to construct the command-line that enables it. Why not avoiding on the affected platforms, since they are unusable with this feature?

[I don't know which bug to follow anymore]

I was hoping to move the MTK issue in general to the other bug, but oh well. Then this bug could remain about enabling pseudo-NMIs in general, which is what it was originally about. :-P

So no, I'm not taking save/restore code for anything that is the responsibility of ATF and for which the architecture provides the adequate level of support

OK, fair enough. I'll focus on just disabling pseudo-NMIs if I detect that they're broken.

As for filtering the enablement, SOC_ID would be one way out, but nobody implements it. I'm not entertaining SOC-specific SMC calls, as if the vendor can be bothered to implement such a call, they can also implement SOC_ID. If you want to disable it for a whole SoC family, that's fine by me.

After spending a little time on it, I think maybe just using DT and avoiding the SMC calls is the right way to go, but I'll see if I still agree when I get the full prototype, and then I guess see what folks on the list say.

But it seems to me that you know exactly what platform you're running on (each board family has a separate kernel?), and you get to construct the command-line that enables it. Why not avoiding on the affected platforms, since they are unusable with this feature?

The idea would be to actually fix the firmware and enable this feature. Then we'd have one build that needs to run on devices that have the old (broken) firmware and the new fixed firmware, so we'd need to detect. Technically we could teach the new firmware to add the kernel command line argument, but that feels a little awkward. It could also cause problem in the future if we ever wanted to make a universal arm64 build that worked on a variety of arm64 boards since we don't want to have to teach every architecture's bootloader to add irqchip.gicv3_pseudo_nmi=1

FWIW: I've posted patches for the kernel to disable pseudo-NMI support on the affected devices. See b/281831288#comment13 for details.

Mediatek-related quirks have landed, as can be seen above. I put a plan for getting Mediatek Chromebooks fixed in b/281831288#comment40

Plan for getting this rolled out:

1. Land upstream fixes (still need +2):

5.15:

• https://crrev.com/c/4575505 - BACKPORT: irqchip/gic-v3: Fix priority mask handling

5.10:

• https://crrev.com/c/4575725 - BACKPORT: irqchip/gic-v3: Fix priority mask handling
• https://crrev.com/c/4575724 - UPSTREAM: irqchip/gic-v3: Refactor ISB + EOIR at ack time
• https://crrev.com/c/4575723 - UPSTREAM: irqchip/gic-v3: Ensure pseudo-NMIs have an ISB between ack and handling

2. Figure out how to get the pseudo-NMI backtrace series landed upstream. I'll plan to send out a v9 soon and see if I can drum up any more interest.

3. Re-confirm Chen-Yu's tests earlier that show that turning on pseudo-NMI isn't causing any big performance regressions on all of our hardware.

4. Turn on in arm64 config for ChromeOS 5.10, 5.15, and 6.1 and add the magic kernel command line argument to our build scripts.

Then, I think we're done.

I've sent out v9 of this patch series:

https://lore.kernel.org/r/20230601213440.2488667-1-dianders@chromium.org

We also landed all the upstream fixes for pseudo-NMI in chromeos-5.10 and chromeos-5.15. chromeos-6.1 already has them.

Updated plan:

• Keep track of Mediatek in b/281831288
• Figure out if there is anything else actually broken with pseudo NMI. See my ping to Mark Rutland.
• Figure out how to get the pseudo-NMI backtrace series landed upstream.
• Re-confirm Chen-Yu's tests earlier that show that turning on pseudo-NMI isn't causing any big performance regressions on all of our hardware.
• Turn on in arm64 config for ChromeOS 5.10, 5.15, and 6.1 and add the magic kernel command line argument to our build scripts.

Then, I think we're done.

OK, I ran hackbench on trogdor based somewhat on Chen-Yu's methods in comment#5. I didn't bother forcing to "performance" mode. My tests were run against the 5.15 kernel and the top of my tree can be found at https://crrev.com/c/4704892.

I've attached my hacky script that I used. On my machine, the 3 branches (230719-nmi-test-trogdor-config-disabled, 230719-nmi-test-trogdor-config-enabled, and 230719-nmi-test-trogdor-forced-on) represent how many of the CLs in the chain are applied. The "disable" is just before turning on the config. The "enabled" is after turning on the config but without the command line argument. The "forced on" is where I just force the command line argument on in the kernel.

In order to rule out any changes like thermal pressure changing over the course of the test, I did my usual interleaving of the tests (test all 3 configs, then test all 3 configs again, etc). Raw results are attached.

Here are the results sorted by the config:

$ grep -A2 "230719-nmi-test-trogdor-config-disabled" /b/tip/tmp/230719-pnmi | grep mean
min=24.4, max=25.0, mean=24.6
min=23.8, max=26.5, mean=24.5
min=23.9, max=24.7, mean=24.2
min=24.3, max=24.8, mean=24.5
min=23.9, max=24.4, mean=24.2

$ grep -A2 "230719-nmi-test-trogdor-config-enabled" /b/tip/tmp/230719-pnmi | grep mean
min=23.8, max=24.6, mean=24.1
min=23.9, max=28.3, mean=24.6
min=24.0, max=25.5, mean=24.3
min=23.7, max=24.9, mean=24.2
min=23.7, max=24.9, mean=24.1

$ grep -A2 "230719-nmi-test-trogdor-forced-on" /b/tip/tmp/230719-pnmi | grep mean
min=25.0, max=25.8, mean=25.4
min=25.2, max=26.0, mean=25.5
min=24.8, max=25.6, mean=25.3
min=24.7, max=25.9, mean=25.2
min=25.1, max=25.6, mean=25.4

From my tests, you can see that turning on the config really didn't do too much. However, actually enabling pseudoNMI did cause a slight regression in hackbench (lower numbers are better) where it gave ~4% worse results. I would believe that this is probably OK for a microbenchmark. I'll try to double-confirm against ToT Linux and also see if any real-world benchmarks are affected.

I did 3 loops against vanilla kernel 6.4 (with my patches applied) using the same methodology.

$ grep -A2 config-disabled /b/tip/tmp/230720-pnmi-mainline | grep mean
min=25.0, max=26.9, mean=25.5
min=25.1, max=27.4, mean=25.6
min=24.9, max=26.9, mean=25.4
$ grep -A2 config-enabled /b/tip/tmp/230720-pnmi-mainline | grep mean
min=24.8, max=29.3, mean=25.5
min=24.8, max=25.5, mean=25.1
min=25.0, max=26.8, mean=25.4
$ grep -A2 forced-on /b/tip/tmp/230720-pnmi-mainline | grep mean
min=25.9, max=27.6, mean=26.4
min=25.8, max=26.5, mean=26.1
min=26.0, max=26.5, mean=26.2

Baseline results were slower by about 4% and then, just like against chromeos-5.15, we lost another 4% enabling pseudo-NMI. The baseline results might be slower due to other config options. Since this was a vanilla kernel I built it with the fallback config instead of the normal ChromeOS config.

I also did 3 loops against a 5.15 kernel but turned off "auto FDO" by adding "-kernel_afdo -kern_arm_afdo" to my USE flags. In that case baseline got even worse and we again got ~4% worse when we fully enabled pseudo-NMI.

$ grep -A2 config-disabled /b/tip/tmp/230720-pnmi-noafdo | grep mean
min=26.6, max=27.4, mean=27.1
min=26.6, max=27.8, mean=27.2
min=26.8, max=27.7, mean=27.2
$ grep -A2 config-enabled /b/tip/tmp/230720-pnmi-noafdo | grep mean
min=27.1, max=28.2, mean=27.6
min=27.0, max=28.1, mean=27.4
min=26.8, max=29.5, mean=27.5
$ grep -A2 forced /b/tip/tmp/230720-pnmi-noafdo | grep mean
min=27.9, max=29.2, mean=28.5
min=28.3, max=29.0, mean=28.6
min=28.1, max=29.4, mean=28.5

I managed to get speedometer running with a similar test and it made no measurable difference. ...so I think the 4% microbenchmark hackbench regression here is not something to be overly concerned about. While speedometer isn't the end-all be-all of benchmarks, I think we can be confident that the real-world effect of enabling this won't be large.

Hacky script and results attached. It looks like there was one failure to run the test at the very end, but otherwise data is pretty complete. To make the data easy to visualize, I graphed it in a spreadsheet and also took the mean of the data. The mean of the data after 25 runs was nearly identical. The graph also makes it easy to see that there's no real difference.

FWIW: these landed upstream (horay!). I refreshed the picks against 5.15 and 6.1, but they can't quite land yet. The bots found that when you enable CONFIG_ARM64_ACPI_PARKING_PROTOCOL (which we don't normally enable) that they don't compile in our tree. That's because the patch uses arch_smp_send_reschedule() which doesn't exist in our tree. It came from commit 4c8c3c7f70a6 ("treewide: Trace IPIs sent via smp_send_reschedule()"). That made me wonder why we're using arch_smp_send_reschedule() directly instead of smp_send_reschedule(), so I asked Mark. If we can change upstream to smp_send_reschedule() then that would be cleanest, IMO. Then we can just pick the set and not worry about adding the tracing to our tree.

For now, juggling:

bugjuggler: wait 1 week

Also: an update on plans. As of now, I probably won't try to backport these to 5.10. Specifically, the current first patch of the series, irqchip/gic-v3: Enable support for SGIs to act as NMIs now relies on commit 6abbd6988971 ("irqchip/gic, gic-v3: Make SGIs use handle_percpu_devid_irq()") which doesn't exist in our 5.10. We could backport it, but unless we're actually going to roll firmware out for the various Mediatek boards then it's probably not worth it. Mediatek boards will get this feature when they move to 5.15+.

Running next on MediaTek devices without firmware fixed, I get the following warnings, one per CPU core.

prepare_percpu_nmi called for a non-NMI interrupt: irq 4
WARNING: CPU: 7 PID: 0 at kernel/irq/manage.c:2742 prepare_percpu_nmi+0x19c/0x1c0
Modules linked in:
CPU: 7 PID: 0 Comm: swapper/7 Tainted: G        W          6.6.0-rc3-next-20230928-08558-g358342cb3833 #31 7514c590cecd3e60aced4f5aa68fe1342e34a074
Hardware name: Google Hayato rev1 (DT)
pstate: 604001c9 (nZCv dAIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : prepare_percpu_nmi+0x19c/0x1c0
lr : prepare_percpu_nmi+0x19c/0x1c0
sp : ffffffc080477d90
pmr_save: 000000f0
x29: ffffffc080477d90 x28: 0000000000000000 x27: 0000000000000000
x26: 0000000000000000 x25: ffffffe09e2ae000 x24: ffffffe09e2ae428
x23: ffffffe09f411f80 x22: 1ffffff81008efba x21: 0000000000000004
x20: 0000000000000001 x19: ffffff80c0074000 x18: 0000000000000000
x17: 000000040044ffff x16: 005000f2b5593519 x15: 0000000000000000
x14: 1ffffff81008ef0e x13: 0000000041b58ab3 x12: ffffffbc13e4b389
x11: 1ffffffc13e4b388 x10: ffffffbc13e4b388 x9 : ffffffe09c5c0cc4
x8 : 00000043ec1b4c78 x7 : ffffffe09f259c40 x6 : 0000000000000001
x5 : ffffffe09f259c40 x4 : 0000000000000000 x3 : dfffffc000000000
x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff80c0af0000
Call trace:
 prepare_percpu_nmi+0x19c/0x1c0
 ipi_setup.isra.0+0x44/0xc8
 secondary_start_kernel+0x148/0x2b0
 __secondary_switched+0xb8/0xc0
irq event stamp: 0
hardirqs last  enabled at (0): [<0000000000000000>] 0x0
hardirqs last disabled at (0): [<ffffffe09c4d9900>] copy_process+0xcd0/0x2880
softirqs last  enabled at (0): [<ffffffe09c4d9900>] copy_process+0xcd0/0x2880
softirqs last disabled at (0): [<0000000000000000>] 0x0
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

Maybe we could do something about this?

BTW, I see other teams are backporting pseudo-NMI for their uses, and I see some latency / performance regression measurements in b/283977855, b/276997882, b/276317977.

Running next on MediaTek devices without firmware fixed, I get the following warnings, one per CPU core.

Ah, dang. It didn't do this before I rejiggered all the code for Mark's feedback, but it clearly does now. I'll send up a patch ASAP.

BTW, I see other teams are backporting pseudo-NMI for their uses, and I see some latency / performance regression measurements in

Yeah, I was aware of some of these. We should certainly keep an eye on this, but I think we know that some codepaths will be slower with pseudo-NMI. IMO we should mitigate these as much as possible and then enable it anyway. The extra debuggability is worth it. I'm happy to have a discussion about this if people disagree, though. I'm also planning to try to talk with other kernel teams at Google about this in early November.

Running next on MediaTek devices without firmware fixed, I get the following warnings, one per CPU core.

Fix posted. See https://lore.kernel.org/r/20231002094526.1.Ie8f760213053e3d11592f892b30912dbac6b8b48@changeid

I'm also planning to try to talk with other kernel teams at Google about this in early November.

It would make a great topic for Kernel Exchange.

It would make a great topic for Kernel Exchange.

Yup, that's the plan and what I was alluding to. I've already submitted a lightning talk to hopefully get people talking about it.

Still working on stuff upstream. Hopefully we'll get things all resolved sooner, but schedule a nag for 2 weeks just in case:

bugjuggler: wait 2 weeks

Fixes have landed upstream and I've refreshed the picks to v6.1 and v5.15.