[PATCH 0/2] hv: vmbus: Small cleanups

[PATCH 0/2] hv: vmbus: Small cleanups

[Sebastian Andrzej Siewior]

Replacing the lockdep_hardirq_threaded() annotation which does not
belong in drivers and removing the vmbus_irq_initialized which seems
redundant.

Sebastian Andrzej Siewior (2):
  hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation
  hv: vmbus: Remove vmbus_irq_initialized

 drivers/hv/vmbus_drv.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

-- 
2.53.0

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

[Sebastian Andrzej Siewior]

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.

Replace lockdep_hardirq_threaded() with a lockdep annotation limiting
threaded context on PREEMPT_RT to __vmbus_isr().

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();
+		static DEFINE_WAIT_OVERRIDE_MAP(vmbus_map, LD_WAIT_CONFIG);
+
+		lock_map_acquire_try(&vmbus_map);
 		__vmbus_isr();
+		lock_map_release(&vmbus_map);
 	}
 }
 EXPORT_SYMBOL_FOR_MODULES(vmbus_isr, "mshv_vtl");
-- 
2.53.0

[PATCH 2/2] hv: vmbus: Remove vmbus_irq_initialized

[Sebastian Andrzej Siewior]

vmbus_irq_initialized is only true if the registration of the per-CPU
threads succeeded. If it failed, the whole registration aborts and the
vmbus_exit() path is never called.

Remove vmbus_irq_initialized.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hv/vmbus_drv.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index e44275370ac2a..7417841cd1f70 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1392,8 +1392,6 @@ static void run_vmbus_irqd(unsigned int cpu)
 	__vmbus_isr();
 }
 
-static bool vmbus_irq_initialized;
-
 static struct smp_hotplug_thread vmbus_irq_threads = {
 	.store                  = &vmbus_irqd,
 	.setup			= vmbus_irqd_setup,
@@ -1513,11 +1511,10 @@ static int vmbus_bus_init(void)
 	 * the VMbus interrupt handler.
 	 */
 
-	if (IS_ENABLED(CONFIG_PREEMPT_RT) && !vmbus_irq_initialized) {
+	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
 		ret = smpboot_register_percpu_thread(&vmbus_irq_threads);
 		if (ret)
 			goto err_kthread;
-		vmbus_irq_initialized = true;
 	}
 
 	if (vmbus_irq == -1) {
@@ -1561,10 +1558,8 @@ static int vmbus_bus_init(void)
 	else
 		free_percpu_irq(vmbus_irq, &vmbus_evt);
 err_setup:
-	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
 		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
-		vmbus_irq_initialized = false;
-	}
 err_kthread:
 	bus_unregister(&hv_bus);
 	return ret;
@@ -3033,10 +3028,9 @@ static void __exit vmbus_exit(void)
 		hv_remove_vmbus_handler();
 	else
 		free_percpu_irq(vmbus_irq, &vmbus_evt);
-	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
 		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
-		vmbus_irq_initialized = false;
-	}
+
 	for_each_online_cpu(cpu) {
 		struct hv_per_cpu_context *hv_cpu
 			= per_cpu_ptr(hv_context.cpu_context, cpu);
-- 
2.53.0

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

[Michael Kelley]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Sent: Wednesday, April 1, 2026 8:15 AM
> 

Nit: For historical consistency, use "Drivers: hv: vmbus:" as the prefix for the
patch "Subject:" line.  Same in Patch 2 of this series.

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

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

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

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

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

Michael

> +		static DEFINE_WAIT_OVERRIDE_MAP(vmbus_map, LD_WAIT_CONFIG);
> +
> +		lock_map_acquire_try(&vmbus_map);
>  		__vmbus_isr();
> +		lock_map_release(&vmbus_map);
>  	}
>  }
>  EXPORT_SYMBOL_FOR_MODULES(vmbus_isr, "mshv_vtl");
> --
> 2.53.0

RE: [PATCH 2/2] hv: vmbus: Remove vmbus_irq_initialized

[Michael Kelley]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Sent: Wednesday, April 1, 2026 8:15 AM
> 
> vmbus_irq_initialized is only true if the registration of the per-CPU
> threads succeeded. If it failed, the whole registration aborts and the
> vmbus_exit() path is never called.
> 
> Remove vmbus_irq_initialized.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Michael Kelley <mhklinux@outlook.com>
But see comment about the patch Subject prefix from Patch 1 of this series.

> ---
>  drivers/hv/vmbus_drv.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index e44275370ac2a..7417841cd1f70 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1392,8 +1392,6 @@ static void run_vmbus_irqd(unsigned int cpu)
>  	__vmbus_isr();
>  }
> 
> -static bool vmbus_irq_initialized;
> -
>  static struct smp_hotplug_thread vmbus_irq_threads = {
>  	.store                  = &vmbus_irqd,
>  	.setup			= vmbus_irqd_setup,
> @@ -1513,11 +1511,10 @@ static int vmbus_bus_init(void)
>  	 * the VMbus interrupt handler.
>  	 */
> 
> -	if (IS_ENABLED(CONFIG_PREEMPT_RT) && !vmbus_irq_initialized) {
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
>  		ret = smpboot_register_percpu_thread(&vmbus_irq_threads);
>  		if (ret)
>  			goto err_kthread;
> -		vmbus_irq_initialized = true;
>  	}
> 
>  	if (vmbus_irq == -1) {
> @@ -1561,10 +1558,8 @@ static int vmbus_bus_init(void)
>  	else
>  		free_percpu_irq(vmbus_irq, &vmbus_evt);
>  err_setup:
> -	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
> -		vmbus_irq_initialized = false;
> -	}
>  err_kthread:
>  	bus_unregister(&hv_bus);
>  	return ret;
> @@ -3033,10 +3028,9 @@ static void __exit vmbus_exit(void)
>  		hv_remove_vmbus_handler();
>  	else
>  		free_percpu_irq(vmbus_irq, &vmbus_evt);
> -	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
> -		vmbus_irq_initialized = false;
> -	}
> +
>  	for_each_online_cpu(cpu) {
>  		struct hv_per_cpu_context *hv_cpu
>  			= per_cpu_ptr(hv_context.cpu_context, cpu);
> --
> 2.53.0

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

[Sebastian Andrzej Siewior]

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