2018-05-23 00:19:31

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()


Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
also run in tasklet context as the channel event callback.

With CONFIG_PROVE_LOCKING=y in the latest mainline, or
old kernels that don't have commit f71b74bca637 ("irq/softirqs: Use lockdep
to assert IRQs are disabled/enabled"), it turns out can we trigger a warning
at the beginning of __local_bh_enable_ip(), because the upper layer
irq code can call hv_compose_msi_msg() with local irqs disabled.

Let's fix the warning by switching to local_irq_save()/restore(). This
is not an issue because hv_pci_onchannelcallback() is not slow, and it
not a hot path.

Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
Signed-off-by: Dexuan Cui <[email protected]>
Cc: <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: K. Y. Srinivasan <[email protected]>
---

A trimmed version of the warning is:

IRQs not enabled as expected
WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip+0xb0/0xe0
Call Trace:
hv_compose_msi_msg+0x209/0x462 [pci_hyperv]
irq_chip_compose_msi_msg+0x41/0x50
msi_domain_activate+0x1a/0x40
__irq_domain_activate_irq+0x59/0x90
irq_domain_activate_irq+0x25/0x40
__setup_irq+0x3ec/0x730
request_threaded_irq+0xfa/0x1a0
mlx4_init_eq_table+0x3c3/0x5f0 [mlx4_core]
mlx4_setup_hca+0x1db/0x750 [mlx4_core]
mlx4_load_one+0xad2/0x13b0 [mlx4_core]
mlx4_init_one+0x578/0x710 [mlx4_core]
local_pci_probe+0x1e/0x50
work_for_cpu_fn+0x10/0x20
process_one_work+0x1d4/0x5a0
worker_thread+0x1cb/0x3d0
kthread+0xf5/0x130

drivers/pci/host/pci-hyperv.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 50cdefe..ad6a64d 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1051,6 +1051,7 @@ static u32 hv_compose_msi_req_v2(
*/
static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
{
+ unsigned long flags;
struct irq_cfg *cfg = irqd_cfg(data);
struct hv_pcibus_device *hbus;
struct hv_pci_dev *hpdev;
@@ -1148,14 +1149,14 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
* the channel callback directly when channel->target_cpu is
* the current CPU. When the higher level interrupt code
* calls us with interrupt enabled, let's add the
- * local_bh_disable()/enable() to avoid race.
+ * local_irq_save()/restore() to avoid race.
*/
- local_bh_disable();
+ local_irq_save(flags);

if (hbus->hdev->channel->target_cpu == smp_processor_id())
hv_pci_onchannelcallback(hbus);

- local_bh_enable();
+ local_irq_restore(flags);

if (hpdev->state == hv_pcichild_ejecting) {
dev_err_once(&hbus->hdev->device,
--
2.7.4



2018-05-25 19:53:36

by Haiyang Zhang

[permalink] [raw]
Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()



> -----Original Message-----
> From: Dexuan Cui
> Sent: Tuesday, May 22, 2018 8:18 PM
> To: Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> <[email protected]>; [email protected]; KY Srinivasan
> <[email protected]>; Stephen Hemminger <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> Haiyang Zhang <[email protected]>; [email protected];
> [email protected]
> Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>
>
> Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
> uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can also
> run in tasklet context as the channel event callback.
>
> With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs are
> disabled/enabled"), it turns out can we trigger a warning at the beginning of
> __local_bh_enable_ip(), because the upper layer irq code can call
> hv_compose_msi_msg() with local irqs disabled.
>
> Let's fix the warning by switching to local_irq_save()/restore(). This is not an
> issue because hv_pci_onchannelcallback() is not slow, and it not a hot path.
>
> Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
> Signed-off-by: Dexuan Cui <[email protected]>
> Cc: <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: K. Y. Srinivasan <[email protected]>
> ---

Reviewed-by: Haiyang Zhang <[email protected]>

Thanks you.


2018-06-07 00:20:47

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

> From: Haiyang Zhang
> Sent: Friday, May 25, 2018 12:52
> To: Dexuan Cui <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; KY Srinivasan <[email protected]>; Stephen
> Hemminger <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>
> > From: Dexuan Cui
> > Sent: Tuesday, May 22, 2018 8:18 PM
> > To: Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > <[email protected]>; [email protected]; KY Srinivasan
> > <[email protected]>; Stephen Hemminger <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > Haiyang Zhang <[email protected]>; [email protected];
> > [email protected]
> > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> >
> >
> > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> hv_compose_msi_msg()")
> > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> also
> > run in tasklet context as the channel event callback.
> >
> > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs
> are
> > disabled/enabled"), it turns out can we trigger a warning at the beginning of
> > __local_bh_enable_ip(), because the upper layer irq code can call
> > hv_compose_msi_msg() with local irqs disabled.
> >
> > Let's fix the warning by switching to local_irq_save()/restore(). This is not an
> > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path.
> >
> > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
> > Signed-off-by: Dexuan Cui <[email protected]>
> > Cc: <[email protected]>
> > Cc: Stephen Hemminger <[email protected]>
> > Cc: K. Y. Srinivasan <[email protected]>
> > ---
>
> Reviewed-by: Haiyang Zhang <[email protected]>
>
> Thanks you.

Hi Lorenzo,

Can I have your reply to this patch?

-- Dexuan

2018-06-13 20:34:09

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

> From: Dexuan Cui
> Sent: Wednesday, June 6, 2018 17:15
> To: Haiyang Zhang <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; KY Srinivasan <[email protected]>; Stephen
> Hemminger <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>
> > From: Haiyang Zhang
> > Sent: Friday, May 25, 2018 12:52
> > To: Dexuan Cui <[email protected]>; Lorenzo Pieralisi
> > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; KY Srinivasan <[email protected]>; Stephen
> > Hemminger <[email protected]>; [email protected];
> > [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> >
> > > From: Dexuan Cui
> > > Sent: Tuesday, May 22, 2018 8:18 PM
> > > To: Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > <[email protected]>; [email protected]; KY Srinivasan
> > > <[email protected]>; Stephen Hemminger <[email protected]>;
> > > [email protected]; [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > Haiyang Zhang <[email protected]>; [email protected];
> > > [email protected]
> > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > hv_compose_msi_msg()
> > >
> > >
> > > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > hv_compose_msi_msg()")
> > > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> > also
> > > run in tasklet context as the channel event callback.
> > >
> > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs
> > are
> > > disabled/enabled"), it turns out can we trigger a warning at the beginning
> of
> > > __local_bh_enable_ip(), because the upper layer irq code can call
> > > hv_compose_msi_msg() with local irqs disabled.
> > >
> > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an
> > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path.
> > >
> > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> hv_compose_msi_msg()")
> > > Signed-off-by: Dexuan Cui <[email protected]>
> > > Cc: <[email protected]>
> > > Cc: Stephen Hemminger <[email protected]>
> > > Cc: K. Y. Srinivasan <[email protected]>
> > > ---
> >
> > Reviewed-by: Haiyang Zhang <[email protected]>
> >
> > Thanks you.
>
> Hi Lorenzo,
>
> Can I have your reply to this patch?
>
> -- Dexuan

It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
I guess Lorenzo may be on vacation.

@Bjorn, can this patch go through your tree?
Should I resubmit it?

Thanks,
-- Dexuan

2018-06-13 22:16:10

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

On Wed, Jun 13, 2018 at 08:32:13PM +0000, Dexuan Cui wrote:
> > From: Dexuan Cui
> > Sent: Wednesday, June 6, 2018 17:15
> > To: Haiyang Zhang <[email protected]>; Lorenzo Pieralisi
> > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; KY Srinivasan <[email protected]>; Stephen
> > Hemminger <[email protected]>; [email protected];
> > [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> >
> > > From: Haiyang Zhang
> > > Sent: Friday, May 25, 2018 12:52
> > > To: Dexuan Cui <[email protected]>; Lorenzo Pieralisi
> > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > [email protected]; KY Srinivasan <[email protected]>; Stephen
> > > Hemminger <[email protected]>; [email protected];
> > > [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > hv_compose_msi_msg()
> > >
> > > > From: Dexuan Cui
> > > > Sent: Tuesday, May 22, 2018 8:18 PM
> > > > To: Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > > <[email protected]>; [email protected]; KY Srinivasan
> > > > <[email protected]>; Stephen Hemminger <[email protected]>;
> > > > [email protected]; [email protected]; [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > Haiyang Zhang <[email protected]>; [email protected];
> > > > [email protected]
> > > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > > hv_compose_msi_msg()
> > > >
> > > >
> > > > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > > hv_compose_msi_msg()")
> > > > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> > > also
> > > > run in tasklet context as the channel event callback.
> > > >
> > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs
> > > are
> > > > disabled/enabled"), it turns out can we trigger a warning at the beginning
> > of
> > > > __local_bh_enable_ip(), because the upper layer irq code can call
> > > > hv_compose_msi_msg() with local irqs disabled.
> > > >
> > > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an
> > > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path.
> > > >
> > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > hv_compose_msi_msg()")
> > > > Signed-off-by: Dexuan Cui <[email protected]>
> > > > Cc: <[email protected]>
> > > > Cc: Stephen Hemminger <[email protected]>
> > > > Cc: K. Y. Srinivasan <[email protected]>
> > > > ---
> > >
> > > Reviewed-by: Haiyang Zhang <[email protected]>
> > >
> > > Thanks you.
> >
> > Hi Lorenzo,
> >
> > Can I have your reply to this patch?
> >
> > -- Dexuan
>
> It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> I guess Lorenzo may be on vacation.
>
> @Bjorn, can this patch go through your tree?
> Should I resubmit it?

No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
he'll pick this up as he catches up.

You might, however, fix the commit log:

This is not an issue because hv_pci_onchannelcallback() is not slow,
and it not a hot path.

This has at least one typo (I think you mean "and *is* not a hot
path").

I also don't understand the sentence as a whole because the
hv_pci_onchannelcallback() comment says it's called whenever the host
sends a packet to this channel, and that *does* sound like a hot path.

I also don't understand the "hv_pci_onchannelcallback() is not slow"
part. In other words, you're saying hv_pci_onchannelcallback() is
fast and it's not a hot path. And apparently this has something to do
with the difference between local_bh_disable() and local_irq_save()?

Bjorn

2018-06-13 22:51:01

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

> From: Bjorn Helgaas <[email protected]>
> Sent: Wednesday, June 13, 2018 15:15
> > ...
> > It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> > I guess Lorenzo may be on vacation.
> >
> > @Bjorn, can this patch go through your tree?
> > Should I resubmit it?
>
> No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
> he'll pick this up as he catches up.
OK, I see. Thanks!

> You might, however, fix the commit log:
>
> This is not an issue because hv_pci_onchannelcallback() is not slow,
> and it not a hot path.
>
> This has at least one typo (I think you mean "and *is* not a hot
> path").
Sorry -- yes, it's a typo. I hope Lorenzo can help to fix this, or I can
resubmit it if Lorenzo or you want me to do it.

> I also don't understand the sentence as a whole because the
> hv_pci_onchannelcallback() comment says it's called whenever the host
> sends a packet to this channel, and that *does* sound like a hot path.
Sorry for not making it clear.
The host only sends a packet into the channel of the guest when there
is a change of device configuration (i.e. hot add or remove a device), or
the host is responding to the guest's request.

The change of device configuration is only triggered on-demand by the
administrator on the host, and the guest's requests are one-off when
the device is probed.

So IMO the callback is not a hot path.

> I also don't understand the "hv_pci_onchannelcallback() is not slow"
> part. In other words, you're saying hv_pci_onchannelcallback() is
> fast and it's not a hot path. And apparently this has something to do
> with the difference between local_bh_disable() and local_irq_save()?
>
> Bjorn
Actually in my original internal version of the patch, I did use
local_irq_save/restore().

hv_pci_onchannelcallback() itself runs fast, but here since it's in a
loop (i.e. the while (!try_wait_for_completion(&comp.comp_pkt.host_event)
loop), IIRC I was asked if I really need local_irq_save/restore(),
and I answered "not really", so later I switched to local_bh_disable()/enable().

However, recently I found that if we enable CONFIG_PROVE_LOCKING=y,
the local_bh_enable() can trigger a warning because the function
hv_compose_msi_msg() can be called with local IRQs disabled (BTW,
hv_compose_msi_msg() can also be called with local IRQS enabled in
another code path):

IRQs not enabled as expected
WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip

Despite the warning, the code itself can still work correctly, but IMO we'd
better switch back to local_irq_save/restore(), and hence I made the patch.

I hope the explanation sounds reasonable. :-)

Thanks,
-- Dexuan


2018-06-29 10:01:10

by Lorenzo Pieralisi

[permalink] [raw]
Subject: Re: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote:
> > From: Bjorn Helgaas <[email protected]>
> > Sent: Wednesday, June 13, 2018 15:15
> > > ...
> > > It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> > > I guess Lorenzo may be on vacation.
> > >
> > > @Bjorn, can this patch go through your tree?
> > > Should I resubmit it?
> >
> > No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
> > he'll pick this up as he catches up.
> OK, I see. Thanks!
>
> > You might, however, fix the commit log:
> >
> > This is not an issue because hv_pci_onchannelcallback() is not slow,
> > and it not a hot path.
> >
> > This has at least one typo (I think you mean "and *is* not a hot
> > path").
> Sorry -- yes, it's a typo. I hope Lorenzo can help to fix this, or I can
> resubmit it if Lorenzo or you want me to do it.
>
> > I also don't understand the sentence as a whole because the
> > hv_pci_onchannelcallback() comment says it's called whenever the host
> > sends a packet to this channel, and that *does* sound like a hot path.
> Sorry for not making it clear.
> The host only sends a packet into the channel of the guest when there
> is a change of device configuration (i.e. hot add or remove a device), or
> the host is responding to the guest's request.
>
> The change of device configuration is only triggered on-demand by the
> administrator on the host, and the guest's requests are one-off when
> the device is probed.
>
> So IMO the callback is not a hot path.
>
> > I also don't understand the "hv_pci_onchannelcallback() is not slow"
> > part. In other words, you're saying hv_pci_onchannelcallback() is
> > fast and it's not a hot path. And apparently this has something to do
> > with the difference between local_bh_disable() and local_irq_save()?
> >
> > Bjorn
> Actually in my original internal version of the patch, I did use
> local_irq_save/restore().
>
> hv_pci_onchannelcallback() itself runs fast, but here since it's in a
> loop (i.e. the while (!try_wait_for_completion(&comp.comp_pkt.host_event)
> loop), IIRC I was asked if I really need local_irq_save/restore(),
> and I answered "not really", so later I switched to local_bh_disable()/enable().
>
> However, recently I found that if we enable CONFIG_PROVE_LOCKING=y,
> the local_bh_enable() can trigger a warning because the function
> hv_compose_msi_msg() can be called with local IRQs disabled (BTW,
> hv_compose_msi_msg() can also be called with local IRQS enabled in
> another code path):
>
> IRQs not enabled as expected
> WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip
>
> Despite the warning, the code itself can still work correctly, but IMO we'd
> better switch back to local_irq_save/restore(), and hence I made the patch.
>
> I hope the explanation sounds reasonable. :-)

Sorry for the delay in replying. I need to understand if you are
preventing a spurious lockdep warning or you are fixing a kernel
bug. From your commit log, I assume the former option but I do
not think that's what you are really doing.

Apart from the commit log typos fixes I would like a log that
explains *why* this is not a kernel bug fix rather than a harmless
lockdep warning prevention.

Thanks,
Lorenzo

2018-06-30 18:14:56

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

> From: Lorenzo Pieralisi <[email protected]>
> Sent: Friday, June 29, 2018 02:39
> To: Dexuan Cui <[email protected]>
> On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote:
> > > From: Bjorn Helgaas <[email protected]>
> > > Sent: Wednesday, June 13, 2018 15:15
> > > > ...
> > > > It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> > > > I guess Lorenzo may be on vacation.
> > > >
> > > > @Bjorn, can this patch go through your tree?
> > > > Should I resubmit it?
> > >
> > > No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
> > > he'll pick this up as he catches up.
> > OK, I see. Thanks!
> >
> > > You might, however, fix the commit log:
> > >
> > > This is not an issue because hv_pci_onchannelcallback() is not slow,
> > > and it not a hot path.
> > >
> > > This has at least one typo (I think you mean "and *is* not a hot
> > > path").
> > Sorry -- yes, it's a typo. I hope Lorenzo can help to fix this, or I can
> > resubmit it if Lorenzo or you want me to do it.
> >
> > > I also don't understand the sentence as a whole because the
> > > hv_pci_onchannelcallback() comment says it's called whenever the host
> > > sends a packet to this channel, and that *does* sound like a hot path.
> > Sorry for not making it clear.
> > The host only sends a packet into the channel of the guest when there
> > is a change of device configuration (i.e. hot add or remove a device), or
> > the host is responding to the guest's request.
> >
> > The change of device configuration is only triggered on-demand by the
> > administrator on the host, and the guest's requests are one-off when
> > the device is probed.
> >
> > So IMO the callback is not a hot path.
> >
> > > I also don't understand the "hv_pci_onchannelcallback() is not slow"
> > > part. In other words, you're saying hv_pci_onchannelcallback() is
> > > fast and it's not a hot path. And apparently this has something to do
> > > with the difference between local_bh_disable() and local_irq_save()?
> > >
> > > Bjorn
> > Actually in my original internal version of the patch, I did use
> > local_irq_save/restore().
> >
> > hv_pci_onchannelcallback() itself runs fast, but here since it's in a
> > loop (i.e. the while (!try_wait_for_completion(&comp.comp_pkt.host_event)
> > loop), IIRC I was asked if I really need local_irq_save/restore(),
> > and I answered "not really", so later I switched to
> local_bh_disable()/enable().
> >
> > However, recently I found that if we enable CONFIG_PROVE_LOCKING=y,
> > the local_bh_enable() can trigger a warning because the function
> > hv_compose_msi_msg() can be called with local IRQs disabled (BTW,
> > hv_compose_msi_msg() can also be called with local IRQS enabled in
> > another code path):
> >
> > IRQs not enabled as expected
> > WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip
> >
> > Despite the warning, the code itself can still work correctly, but IMO we'd
> > better switch back to local_irq_save/restore(), and hence I made the patch.
> >
> > I hope the explanation sounds reasonable. :-)
>
> Sorry for the delay in replying. I need to understand if you are
> preventing a spurious lockdep warning or you are fixing a kernel
> bug. From your commit log, I assume the former option but I do
> not think that's what you are really doing.

Now my understanding is:
1) When hv_compose_msi_msg() is called with local irq ENABLED by the upper
level irq code, the current code is good and the lockdep warning is not triggered.

2) When hv_compose_msi_msg() is called with local irq DISABLED by the upper
level irq code, the current code *is* buggg: local_bh_enable() can potentially call
do_softirq(), which is not supposed to run when local irq is DISABLED.
I think the lockdep warning is triggered for this reason.

In summary, now I realized the warning is not spurious, and here at the first
place I should not use local_bh_disable()/enable(), which are not supposed to
be used when local irq can be DISABLED.

> Apart from the commit log typos fixes I would like a log that
> explains *why* this is not a kernel bug fix rather than a harmless
> lockdep warning prevention.
>
> Lorenzo

Now I realized there *is* a bug.
I'm going to send a v2 with a new changelog, though the changed code
will remain the same.

Thanks,
-- Dexuan