Message-ID: <4a845e01d9ceff2531f165f4918f72beb064a799.camel@siemens.com>
Date: Mon, 17 Nov 2025 14:59:28 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "paulmck@...nel.org" <paulmck@...nel.org>
CC: "mingo@...nel.org" <mingo@...nel.org>, "rostedt@...dmis.org"
	<rostedt@...dmis.org>, "peterz@...radead.org" <peterz@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "torvalds@...ux-foundation.org"
	<torvalds@...ux-foundation.org>, "bigeasy@...utronix.de"
	<bigeasy@...utronix.de>, "joel@...lfernandes.org" <joel@...lfernandes.org>,
	"will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH 6/9] lockdep: Introduce wait-type checks

Hi Paul,

On Mon, 2025-11-17 at 06:37 -0800, Paul E. McKenney wrote:
> > > > [ BUG: Invalid wait context ]
> > > > 6.18.0-rc1+git476309497a53 #1 Tainted: G           O       
> > > > -----------------------------
> > > > some-user-space-process/1251 is trying to lock:
> > > > ffff000005cbc548 (&counter->events_list_lock){....}-{3:3}, at: counter_push_event+0x68/0x430 [counter]
> > > > other info that might help us debug this:
> > > > context-{2:2}
> > > > no locks held by some-user-space-process/1251.
> > > > stack backtrace:
> > > > CPU: 0 UID: 0 PID: 1251 Comm: some-user-space-process Tainted: G           O        6.18.0-rc1+git476309497a53 #1 PREEMPT 
> > > > Call trace:
> > > >   show_stack+0x20/0x38 (C)
> > > >   dump_stack_lvl+0x8c/0xd0
> > > >   dump_stack+0x18/0x28
> > > >   __lock_acquire+0x91c/0x1f78
> > > >   lock_acquire+0x1c4/0x338
> > > >   _raw_spin_lock_irqsave+0x68/0x98
> > > >   counter_push_event+0x68/0x430 [counter]
> > > >   interrupt_cnt_isr+0x40/0x78 [interrupt_cnt]
> > > >   __handle_irq_event_percpu+0xa4/0x398
> > > >   handle_irq_event+0x54/0xb8
> > > >   handle_simple_irq+0xe4/0x128
> > > >   handle_irq_desc+0x48/0x68
> > > >   generic_handle_domain_irq+0x24/0x38
> > > >   gpio_irq_handler+0xa0/0x140
> > > >   handle_irq_desc+0x48/0x68
> > > >   generic_handle_domain_irq+0x24/0x38
> > > >   gic_handle_irq+0x54/0x128
> > > >   call_on_irq_stack+0x30/0x70
> > > >   do_interrupt_handler+0x88/0x98
> > > >   el0_interrupt+0x5c/0x270
> > > >   __el0_irq_handler_common+0x18/0x28
> > > >   el0t_64_irq_handler+0x10/0x20
> > > >   el0t_64_irq+0x198/0x1a0
> > > > 
> > > > This is not PREEMPT_RT, but still CONFIG_PROVE_RAW_LOCK_NESTING=y config...
> > > > The relevant logic in the current v6.18-rcX is still largely unchanged
> > > > from the Subject commit and...
> > > > 
> > > The validation is not limited to PREEMPT_RT enabled.
> > > …
> > > > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > > > index 32406ef0d6a2d..4a9abf8bd9d15 100644
> > > > > --- a/kernel/locking/lockdep.c
> > > > > +++ b/kernel/locking/lockdep.c
> > > …
> > > > > +	 * Set appropriate wait type for the context; for IRQs we have to take
> > > > > +	 * into account force_irqthread as that is implied by PREEMPT_RT.
> > > > > +	 */
> > > > > +	if (curr->hardirq_context) {
> > > > > +		/*
> > > > > +		 * Check if force_irqthreads will run us threaded.
> > > > > +		 */
> > > > > +		if (curr->hardirq_threaded)
> > > > > +			curr_inner = LD_WAIT_CONFIG;
> > > > > +		else
> > > > > +			curr_inner = LD_WAIT_SPIN;
> > > > 
> > > > ... even though it's "if (curr->hardirq_threaded || curr->irq_config)" in the
> > > > meanwhile, it is unclear to me, why is it conditional at all and not always
> > > > "LD_WAIT_CONFIG" in hardirq context?
> > > > 
> > > > With !PREEMPT_RT but CONFIG_PROVE_RAW_LOCK_NESTING=y we always end up
> > > > with LD_WAIT_SPIN here and it always warns if we take "spinlock_t" in hardirq
> > > > context (as in the splat above). But the latter should be legal under all
> > > > circumstances, isn't it?
> > > 
> > > Nope. The validation is performed always. The architectures which don't
> > > support PREEMPT_RT are excluded for $reasons that is why
> > > PROVE_RAW_LOCK_NESTING remains an option there.
> > > 
> > > A spinlock_t is a sleeping lock in a PREEMPT_RT context and the
> > > interrupt hander are threaded. In this case a spinlock_t is okay.
> > > However, if a spinlock_t is used in a non-forced-threaded environment
> > > (such as in a primary handler with IRQF_NO_THREAD) then this handler
> > > will not be threaded. Therefore spinlock_t is not a valid class.
> > 
> > I'm not in PREEMPT_RT, but now CONFIG_PROVE_RAW_LOCK_NESTING=y is forced
> > for ARM64. And as I'm not in PREEMPT_RT, it's allowed to take spinlock_t
> > in an IRQ, no matter what (that's what happens in the above splat, I
> > believe).
> > 
> > So does it mean, that LOCKDEP is not usable any longer on ARM64
> > (for !PREEMPT_RT kernels)?
> 
> LOCKDEP is quite usable, and in this case it is just doing its job by
> preventing people from inadvertently breaking RT.  Which arm64 really
> does support, as you can see by searching for "select ARCH_SUPPORTS_RT"
> in arch/arm64/Kconfig.  So even arm64-specific code must follow the
> PREEMPT_RT rules, which lockdep is enforcing.
> 
> So how to fix this?  One option is to use a raw_spinlock_t instead of a
> spinlock_t.  This works because a raw_spinlock_t remains a spinlock in RT,
> but it also restricts what you can do while holding the lock.
> 
> Does that help?

Mmm. Not yet... Does this mean we are not allowed to use "spinlock_t" in
IRQs any longer? Where has this been announced? This probably means that
the majority of IRQ handlers were already converted tree-wide?

This also would mean that spin_lock_irqsave() shall not exist at all
so that we are forced to use raw_spin_lock_irqsave()?

The current documentation says [1]:
"Use raw_spinlock_t only in real critical core code, low-level interrupt
handling and places where disabling preemption or interrupts is required,
for example, to safely access hardware state."

So does the counter_push_event() constitute such a code?

[1] https://docs.kernel.org/locking/locktypes.html#raw-spinlock-t-and-spinlock-t

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Powered by blists - more mailing lists