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

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

[Dexuan Cui]

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=3Dy 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 warnin=
g
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 <decui@microsoft.com>
Cc: <stable@vger.kernel.org>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: K. Y. Srinivasan <kys@microsoft.com>
---

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+0xb=
0/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 =3D 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 *dat=
a, 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);
=20
 		if (hbus->hdev->channel->target_cpu =3D=3D smp_processor_id())
 			hv_pci_onchannelcallback(hbus);
=20
-		local_bh_enable();
+		local_irq_restore(flags);
=20
 		if (hpdev->state =3D=3D hv_pcichild_ejecting) {
 			dev_err_once(&hbus->hdev->device,
--=20
2.7.4

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

[Haiyang Zhang]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Tuesday, May 22, 2018 8:18 PM
> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com;
> marcelo.cerri@canonical.com
> Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>=20
>=20
> 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.
>=20
> With CONFIG_PROVE_LOCKING=3Dy 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 beginnin=
g of
> __local_bh_enable_ip(), because the upper layer irq code can call
> hv_compose_msi_msg() with local irqs disabled.
>=20
> 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 pa=
th.
>=20
> Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()"=
)
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Cc: <stable@vger.kernel.org>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Cc: K. Y. Srinivasan <kys@microsoft.com>
> ---

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Thanks you.

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

[Dexuan Cui]

> From: Haiyang Zhang
> Sent: Friday, May 25, 2018 12:52
> To: Dexuan Cui <decui@microsoft.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>;
> linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> vkuznets@redhat.com; marcelo.cerri@canonical.com
> Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>=20
> > From: Dexuan Cui
> > Sent: Tuesday, May 22, 2018 8:18 PM
> > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan
> > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.or=
g;
> > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com;
> > marcelo.cerri@canonical.com
> > 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() ca=
n
> also
> > run in tasklet context as the channel event callback.
> >
> > With CONFIG_PROVE_LOCKING=3Dy in the latest mainline, or old kernels th=
at
> > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IR=
Qs
> are
> > disabled/enabled"), it turns out can we trigger a warning at the beginn=
ing 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 <decui@microsoft.com>
> > Cc: <stable@vger.kernel.org>
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Cc: K. Y. Srinivasan <kys@microsoft.com>
> > ---
>=20
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
>=20
> Thanks you.

Hi Lorenzo,

Can I have your reply to this patch?

-- Dexuan

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

[Dexuan Cui]

> From: Dexuan Cui
> Sent: Wednesday, June 6, 2018 17:15
> To: Haiyang Zhang <haiyangz@microsoft.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>;
> linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> vkuznets@redhat.com; marcelo.cerri@canonical.com
> Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>=20
> > From: Haiyang Zhang
> > Sent: Friday, May 25, 2018 12:52
> > To: Dexuan Cui <decui@microsoft.com>; Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> > apw@canonical.com; jasowang@redhat.com
> > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.or=
g;
> > vkuznets@redhat.com; marcelo.cerri@canonical.com
> > 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 <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan
> > > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.=
org;
> > > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com;
> > > marcelo.cerri@canonical.com
> > > 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=3Dy 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 begi=
nning
> 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(). Thi=
s is not an
> > > issue because hv_pci_onchannelcallback() is not slow, and it not a ho=
t path.
> > >
> > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> hv_compose_msi_msg()")
> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > Cc: <stable@vger.kernel.org>
> > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > Cc: K. Y. Srinivasan <kys@microsoft.com>
> > > ---
> >
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > Thanks you.
>=20
> Hi Lorenzo,
>=20
> Can I have your reply to this patch?
>=20
> -- Dexuan

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

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

Thanks,
-- Dexuan

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

[Bjorn Helgaas]

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 <haiyangz@microsoft.com>; Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> > apw@canonical.com; jasowang@redhat.com
> > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > vkuznets@redhat.com; marcelo.cerri@canonical.com
> > 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 <decui@microsoft.com>; Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> > > apw@canonical.com; jasowang@redhat.com
> > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > > vkuznets@redhat.com; marcelo.cerri@canonical.com
> > > 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 <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> > > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan
> > > > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > > > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com;
> > > > marcelo.cerri@canonical.com
> > > > 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 <decui@microsoft.com>
> > > > Cc: <stable@vger.kernel.org>
> > > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > > Cc: K. Y. Srinivasan <kys@microsoft.com>
> > > > ---
> > >
> > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > >
> > > 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

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

[Dexuan Cui]

> From: Bjorn Helgaas <helgaas@kernel.org>
> 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?
>=20
> 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!
=20
> You might, however, fix the commit log:
>=20
>   This is not an issue because hv_pci_onchannelcallback() is not slow,
>   and it not a hot path.
>=20
> 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.
=20
> 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()?
>=20
> Bjorn
Actually in my original internal version of the patch, I did use=20
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()/enab=
le().

However, recently I found that if we enable CONFIG_PROVE_LOCKING=3Dy,
the local_bh_enable() can trigger a warning because the function=20
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

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

[Lorenzo Pieralisi]

On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote:
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > 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

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

[Dexuan Cui]

> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Friday, June 29, 2018 02:39
> To: Dexuan Cui <decui@microsoft.com>
> On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote:
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > 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 ca=
n
> > 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 d=
o
> > > 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_even=
t)
> > 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=3Dy,
> > 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 pa=
tch.
> >
> > I hope the explanation sounds reasonable. :-)
>=20
> 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 tri=
ggered.

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 potentia=
lly call
do_softirq(), which is not supposed to run when local irq is DISABLED.=20
I think the lockdep warning is triggered for this reason.

In summary, now I realized the warning is not spurious, and here at the fir=
st
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.
>=20
> 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.=20

Thanks,
-- Dexuan