On 6/6/25 18:58, Peter Zijlstra wrote:
On Fri, Jun 06, 2025 at 05:57:23PM +0800, Pavel Tikhomirov wrote:
This is intended to easily detect irq spinlock self-deadlocks like:
spin_lock_irq(A);
spin_lock_irq(B);
spin_unlock_irq(B);
IRQ {
spin_lock(A); <- deadlocks
spin_unlock(A);
}
spin_unlock_irq(A);
Recently we saw this kind of deadlock on our partner's node:
PID: 408 TASK: ffff8eee0870ca00 CPU: 36 COMMAND: "kworker/36:1H"
#0 [fffffe3861831e60] crash_nmi_callback at ffffffff97269e31
#1 [fffffe3861831e68] nmi_handle at ffffffff972300bb
#2 [fffffe3861831eb0] default_do_nmi at ffffffff97e9e000
#3 [fffffe3861831ed0] exc_nmi at ffffffff97e9e211
#4 [fffffe3861831ef0] end_repeat_nmi at ffffffff98001639
[exception RIP: native_queued_spin_lock_slowpath+638]
RIP: ffffffff97eb31ae RSP: ffffb1c8cd2a4d40 RFLAGS: 00000046
RAX: 0000000000000000 RBX: ffff8f2dffb34780 RCX: 0000000000940000
RDX: 000000000000002a RSI: 0000000000ac0000 RDI: ffff8eaed4eb81c0
RBP: ffff8eaed4eb81c0 R8: 0000000000000000 R9: ffff8f2dffaf3438
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000024 R14: 0000000000000000 R15: ffffd1c8bfb24b80
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
--- <NMI exception stack> ---
#5 [ffffb1c8cd2a4d40] native_queued_spin_lock_slowpath at ffffffff97eb31ae
#6 [ffffb1c8cd2a4d60] _raw_spin_lock_irqsave at ffffffff97eb2730
#7 [ffffb1c8cd2a4d70] __wake_up at ffffffff9737c02d
#8 [ffffb1c8cd2a4da0] sbitmap_queue_wake_up at ffffffff9786c74d
#9 [ffffb1c8cd2a4dc8] sbitmap_queue_clear at ffffffff9786cc97
--- <IRQ stack> ---
[exception RIP: _raw_spin_unlock_irq+20]
RIP: ffffffff97eb2e84 RSP: ffffb1c8cd90fd18 RFLAGS: 00000283
RAX: 0000000000000001 RBX: ffff8eafb68efb40 RCX: 0000000000000001
RDX: 0000000000000008 RSI: 0000000000000061 RDI: ffff8eafb06c3c70
RBP: ffff8eee7af43000 R8: ffff8eaed4eb81c8 R9: ffff8eaed4eb81c8
R10: 0000000000000008 R11: 0000000000000008 R12: 0000000000000000
R13: ffff8eafb06c3bd0 R14: ffff8eafb06c3bc0 R15: ffff8eaed4eb81c0
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
Luckily it was already fixed in mainstream by:
commit b313a8c83551 ("block: Fix lockdep warning in blk_mq_mark_tag_wait")
Currently if we are unlucky we may miss such a deadlock on our testing
system as it is racy and it depends on the specific interrupt handler
appearing at the right place and at the right time. So this patch tries
to detect the problem despite the absence of the interrupt.
If we see spin_lock_irq under interrupts already disabled we can assume
that it has paired spin_unlock_irq which would reenable interrupts where
they should not be reenabled. So we report a warning for it.
Same thing on spin_unlock_irq even if we were lucky and there was no
deadlock let's report if interrupts were enabled.
Let's make this functionality catch one problem and then be disabled, to
prevent from spamming kernel log with warnings. Also let's add sysctl
kernel.debug_spin_lock_irq_with_disabled_interrupts to reenable it if
needed. Also let's add a by default enabled configuration option
DEBUG_SPINLOCK_IRQ_WITH_DISABLED_INTERRUPTS_BY_DEFAULT, in case we will
need this on boot.
Yes Lockdep can detect that, if it sees both the interrupt stack and the
regular stack where we can get into interrupt with spinlock held. But
with this approach we can detect the problem even without ever getting
into interrupt stack. And also this functionality seems to be more
lightweight then Lockdep as it does not need to maintain lock dependency
graph.
So why do we need DEBUG_SPINLOCK code, that's injected into every single
callsite, if lockdep can already detect this?
Hello Peter,
Thank you for your reply!
We are trying to figure out a way to improve the detection of similar
cases in future, in our tests and even in some cases in production. I
see that in mainstream this issue (commit b313a8c83551) was detected by
Lockdep and fixed in the next release, that is very fast. But, sadly, we
didn't catch it in our QA testing cycles (including runs with debug
kernel with Lockdep), and it led to repeated complete nodes hang in
production of one of our partners.
1. This patch, arguably, is less performance consuming than Lockdep (I
don't have performance results, but my motivations is: we don't need to
save stacks and manage lock topology tree on each lock, thus saving
memory and some cpu cycles). Because Lockdep makes tests slow, we don't
run all tests with it, and hopefully we would be able to run all tests
with this small check, and it will improve detection probability.
2. If on a production node we have a reproduce of such a deadlock on
spinlock and we can't easily find the real spinlock owner in crashdump,
we can probably enable this small feature there and catch the "guilty"
stack directly on next reproduce. Running Lockdep there will definitely
be a no go. The suggested checks are really quite small and lightweight
and we think even release kernels can have those checks compiled-in
(with static key disabled by default surely) and those checks can be
enabled without extra reboot on affected nodes and with very small
performance footprint.
3. Lockdep generates false positives, from my experience roughly <10% of
what it reports is an actual thing, but maybe I'm just unlucky (or not
understanding something). And then Lockdep detects a problem there is no
way to search for the next error (AFAIU lock dependency tree gets a
cycle), you have to reboot, if I don't miss something. So we have to fix
even false positives if we want to get possible new detections later on
the same test. (Or maybe disable Lockdep tracing with lockdep_off, but
that can skip a real problem nearby.)
4. If lock in interrupt code path will not be triggered in test Lockdep
will not detect the deadlock. But with this check we only need to
trigger the non-interrupt code path to detect the problem (having
enabled interrupt brefore spin_unlock_irq* is always a bad sign) and
possible deadlock. So we again improve probability of detecting such
cases by a small fraction.
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
