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

From: Sebastian Andrzej Siewior <[email protected]> Sent: Wednesday, July 15, 2026 7:33 AM
> 
> 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 [email protected]? 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?

Yes, this makes sense. Thanks for the help.

> 
> > >
> > > Fixes: f8e6343b7a89c ("Drivers: hv: vmbus: Use kthread for vmbus interrupts on PREEMPT_RT")
> > > Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> > > ---
> > >  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.

Yes, I'm good with this.

> 
> > 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?

The stimer0 is not legacy, but having the stimer0 interrupt delivered as
a VMBus message *is* legacy. The legacy mechanism is used when running
on really old versions of Hyper-V. Modern versions of Hyper-V deliver stimer0
interrupts via a separate stimer0-specific hardirq. This linkage between stimer0
and VMBus creates separation problems in that it should conceptually be
possible to run on Hyper-V with stimer0 without using VMBus or vmbus_isr().

I will do some research on the history and supported versions of Hyper-V.
It might be possible to drop Linux support for running on these old versions
of Hyper-V, and remove this legacy mechanism for delivering stimer0
interrupts. That would be a nice cleanup of some historical baggage.

> 
> 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.

OK.

Michael