Re: RE: [PATCH 1/2] hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-04-04 01:22:08 [+0000], Michael Kelley wrote:
> Nit: For historical consistency, use "Drivers: hv: vmbus:" as the prefix for the
> patch "Subject:" line.  Same in Patch 2 of this series.

okay.

> Also, any reason not to have copied linux-kernel@vger.kernel.org? I know this
> is pretty much just a Hyper-V thing, but I would have liked to see what the
> Sashiko AI did with these two patches. :-)

Can do.

> > lockdep_hardirq_threaded() is supposed to be used within IRQ core code
> > and not within drivers. It is not obvious from within the driver, that
> > this is the only interrupt service routing and that it is not shared
> > handler.
> 
> I presume you meant "routine". And what do you mean by "the only interrupt
> service routine"? And why is the lack of obviousness relevant here? I don't have
> deep expertise in lockdep, but evidently there's some conclusion to reach and it
> would have helped me to have it spelled out.

You use lockdep_hardirq_threaded() which marks the hardirq as a thread
for lockdep purposes. The reason is that the IRQ core will force-thread
the handler and the whole routine will be threaded. It does invoke the
function and the very begin and nothing else will be done.

vmbus_isr() is using it before __vmbus_isr() is invoked and there can be
other functions/ ISRs that are invoked afterwards which would be wrongly
recognized.
To make it more concrete: sysvec_hyperv_callback() invokes mshv_handler
and vmbus_handler. Would vmbus_handler() be invoked first then it will
"spill" this lockdep attribute into mshv_handler(). Nested locks in
mshv_handler() would not be visible then.
This override limits it only to __vmbus_isr() while the RT case above is
using vmbus_irqd_wake().

> > 
> > Replace lockdep_hardirq_threaded() with a lockdep annotation limiting
> > threaded context on PREEMPT_RT to __vmbus_isr().
> 
> Again, I'm not clear what "limiting threaded context" means. But see my
> additional comment further down.

Does the explanation above make it clear?

> > 
> > Fixes: f8e6343b7a89c ("Drivers: hv: vmbus: Use kthread for vmbus interrupts on PREEMPT_RT")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  drivers/hv/vmbus_drv.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index bc4fc1951ae1c..e44275370ac2a 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1407,8 +1407,11 @@ void vmbus_isr(void)
> >  	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >  		vmbus_irqd_wake();
> >  	} else {
> > -		lockdep_hardirq_threaded();
> 
> I see two similar occurrences of LD_WAIT_CONFIG in the kernel:
> __kfree_rcu_sheaf() and adjacent to printk_legacy_allow_spinlock_enter().
> Both occurrences have a multi-line comment explaining the "why". I'd like
> to see a similar comment here so that drive-by readers of the code have 
> some idea of what's going on. My suggestion is something like this:
> 
>    vmbus_isr() always runs at hard IRQ level -- the interrupt is not threaded. It
>    calls __vmbus_isr() here, which may obtain the spinlock_t sched_lock for
>    a VMBus channel in vmbus_chan_sched(). If CONFIG_PROVE_LOCKING=y,
>    lockdep complains because obtaining spinlock_t's is not permitted at hard
>    IRQ level in PREEMPT_RT configurations. However, the PREEMPT_RT path
>    is handled separately above, so there's actually not a problem. Tell
>    lockdep that acquiring the spinlock_t is valid by temporarily raising
>    the wait-type to LD_WAIT_CONFIG using the "fake" lock vmbus_map.
>    If lockdep is not enabled, the acquire & release of the fake lock are no-ops,
>    so performance is not impacted.
> 
> Please review my suggested text and revise as appropriate, as I'm far
> from an expert on any of this. The above is based on what I've learned
> just now from a bit of research.

What about?
  vmbus_isr is never force-threaded and always invoked at hard IRQ level.
  __vmbus_isr() below can acquire a spinlock_t which becomes a sleeping
  lock and must not be acquired in this context. Therefore on PREEMPT_RT
  this will be threaded via vmbus_irqd_wake(). On non-PREEMPT the
  annotation lets lockdep know that acquiring a spinlock_t is not an
  issue.

> And thanks for jumping in and making all this better ....

so I've been looking at __vmbus_isr() and vmbus_chan_sched() seems fine.
vmbus_message_sched() on the handler invokes hv_stimer0_isr() which
should not be done in the thread but in the hardirq. Is this the only
timer in the system or some legacy thingy that is no longer used?

And there is a lot of tasklet involved which mandates a
local_bh_disable()/enable() in run_vmbus_irqd() around the function or
everything gets pushed into ksoftirqd.

> Michael

Sebastian