[PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[Alex Williamson]

When running a Windows 2012 R2 guest with a pair of VFs assigned
through vfio-pci, we run into a problem trying to hot-unplug those VFs
after the PF has unregistered the netdev.  This is a common scenario
if the PF is unbound from the driver while VFs are active.  In the
case of igb, the resulting guest behavior differs slightly between the
Microsoft provided and Intel add-on guest drivers.  With the Microsoft
driver, the guest seems to stumble through ejecting both VFs, but
takes longer than normal to do so.  With the Intel drivers, only one
VF is unplugged, but Device Manager still shows it as present.  The
second VF is non-functional but also still shown in Device Manager.
At this point, the guest is in such a state that it will not cleanly
shutdown.  With ixgbe VFs, both the Microsoft and Intel drivers take
on this latter behavior.

For both, I've found that disabling SR-IOV before unregistering the PF
netdev device allows the hot-unplug to proceed without interruption or
further ill behavior in the guest.  This is true regardless of which
driver is used.  I don't fully understand what dependency is broken
by unregistering the netdev prior to disabling SR-IOV, but I also
don't see the benefit in delaying SR-IOV teardown in this call path.
It could potentially be moved even earlier, but I'll let those more
familiar with the hardware and code make that determination.  In any
case, the VM behavior is substantially improved by this slight
re-ordering.

I don't have an i40e for testing, but it already appears to disable
SR-IOV much earlier in the unbind path, so I wouldn't expect to find
similar issues.  Thanks,

Alex

---

Alex Williamson (2):
      igb: Teardown SR-IOV before unregister_netdev()
      ixgbe: Teardown SR-IOV before unregister_netdev()


 drivers/net/ethernet/intel/igb/igb_main.c     |    8 ++++----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

[PATCH 1/2] igb: Teardown SR-IOV before unregister_netdev()

[Alex Williamson]

When the .remove() callback for a PF is called, SR-IOV support for the
device is disabled, which requires unbinding and removing the VFs.
The VFs may be in-use either by the host kernel or userspace, such as
assigned to a VM through vfio-pci.  In this latter case, the VFs may
be removed either by shutting down the VM or hot-unplugging the
devices from the VM.  Unfortunately in the case of a Windows 2012 R2
guest, hot-unplug is broken due to the ordering of the PF driver
teardown.  Disabling SR-IOV prior to unregister_netdev() avoids this
issue.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 517746f..606a7ae 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2805,14 +2805,14 @@ static void igb_remove(struct pci_dev *pdev)
 	 */
 	igb_release_hw_control(adapter);
 
-	unregister_netdev(netdev);
-
-	igb_clear_interrupt_scheme(adapter);
-
 #ifdef CONFIG_PCI_IOV
 	igb_disable_sriov(pdev);
 #endif
 
+	unregister_netdev(netdev);
+
+	igb_clear_interrupt_scheme(adapter);
+
 	pci_iounmap(pdev, hw->hw_addr);
 	if (hw->flash_address)
 		iounmap(hw->flash_address);

[PATCH 2/2] ixgbe: Teardown SR-IOV before unregister_netdev()

[Alex Williamson]

When the .remove() callback for a PF is called, SR-IOV support for the
device is disabled, which requires unbinding and removing the VFs.
The VFs may be in-use either by the host kernel or userspace, such as
assigned to a VM through vfio-pci.  In this latter case, the VFs may
be removed either by shutting down the VM or hot-unplugging the
devices from the VM.  Unfortunately in the case of a Windows 2012 R2
guest, hot-unplug is broken due to the ordering of the PF driver
teardown.  Disabling SR-IOV prior to unregister_netdev() avoids this
issue.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index f775123..e27813c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9035,12 +9035,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
 	/* remove the added san mac */
 	ixgbe_del_sanmac_netdev(netdev);
 
-	if (netdev->reg_state == NETREG_REGISTERED)
-		unregister_netdev(netdev);
-
 #ifdef CONFIG_PCI_IOV
 	ixgbe_disable_sriov(adapter);
 #endif
+	if (netdev->reg_state == NETREG_REGISTERED)
+		unregister_netdev(netdev);
+
 	ixgbe_clear_interrupt_scheme(adapter);
 
 	ixgbe_release_hw_control(adapter);

RE: [Intel-wired-lan] [PATCH 1/2] igb: Teardown SR-IOV before unregister_netdev()

[Williams, Mitch A]

ACK

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Alex Williamson
> Sent: Monday, July 27, 2015 4:19 PM
> To: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH 1/2] igb: Teardown SR-IOV before
> unregister_netdev()
> 
> When the .remove() callback for a PF is called, SR-IOV support for the
> device is disabled, which requires unbinding and removing the VFs.
> The VFs may be in-use either by the host kernel or userspace, such as
> assigned to a VM through vfio-pci.  In this latter case, the VFs may
> be removed either by shutting down the VM or hot-unplugging the
> devices from the VM.  Unfortunately in the case of a Windows 2012 R2
> guest, hot-unplug is broken due to the ordering of the PF driver
> teardown.  Disabling SR-IOV prior to unregister_netdev() avoids this
> issue.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index 517746f..606a7ae 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2805,14 +2805,14 @@ static void igb_remove(struct pci_dev *pdev)
>  	 */
>  	igb_release_hw_control(adapter);
> 
> -	unregister_netdev(netdev);
> -
> -	igb_clear_interrupt_scheme(adapter);
> -
>  #ifdef CONFIG_PCI_IOV
>  	igb_disable_sriov(pdev);
>  #endif
> 
> +	unregister_netdev(netdev);
> +
> +	igb_clear_interrupt_scheme(adapter);
> +
>  	pci_iounmap(pdev, hw->hw_addr);
>  	if (hw->flash_address)
>  		iounmap(hw->flash_address);
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

RE: [Intel-wired-lan] [PATCH 2/2] ixgbe: Teardown SR-IOV before unregister_netdev()

[Williams, Mitch A]

ACK

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Alex Williamson
> Sent: Monday, July 27, 2015 4:19 PM
> To: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH 2/2] ixgbe: Teardown SR-IOV before
> unregister_netdev()
> 
> When the .remove() callback for a PF is called, SR-IOV support for the
> device is disabled, which requires unbinding and removing the VFs.
> The VFs may be in-use either by the host kernel or userspace, such as
> assigned to a VM through vfio-pci.  In this latter case, the VFs may
> be removed either by shutting down the VM or hot-unplugging the
> devices from the VM.  Unfortunately in the case of a Windows 2012 R2
> guest, hot-unplug is broken due to the ordering of the PF driver
> teardown.  Disabling SR-IOV prior to unregister_netdev() avoids this
> issue.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index f775123..e27813c 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -9035,12 +9035,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
>  	/* remove the added san mac */
>  	ixgbe_del_sanmac_netdev(netdev);
> 
> -	if (netdev->reg_state == NETREG_REGISTERED)
> -		unregister_netdev(netdev);
> -
>  #ifdef CONFIG_PCI_IOV
>  	ixgbe_disable_sriov(adapter);
>  #endif
> +	if (netdev->reg_state == NETREG_REGISTERED)
> +		unregister_netdev(netdev);
> +
>  	ixgbe_clear_interrupt_scheme(adapter);
> 
>  	ixgbe_release_hw_control(adapter);
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[David Miller]

From: Alex Williamson <alex.williamson@redhat.com>
Date: Mon, 27 Jul 2015 17:18:28 -0600

> When running a Windows 2012 R2 guest with a pair of VFs assigned
> through vfio-pci, we run into a problem trying to hot-unplug those VFs
> after the PF has unregistered the netdev.  This is a common scenario
> if the PF is unbound from the driver while VFs are active.  In the
> case of igb, the resulting guest behavior differs slightly between the
> Microsoft provided and Intel add-on guest drivers.  With the Microsoft
> driver, the guest seems to stumble through ejecting both VFs, but
> takes longer than normal to do so.  With the Intel drivers, only one
> VF is unplugged, but Device Manager still shows it as present.  The
> second VF is non-functional but also still shown in Device Manager.
> At this point, the guest is in such a state that it will not cleanly
> shutdown.  With ixgbe VFs, both the Microsoft and Intel drivers take
> on this latter behavior.
> 
> For both, I've found that disabling SR-IOV before unregistering the PF
> netdev device allows the hot-unplug to proceed without interruption or
> further ill behavior in the guest.  This is true regardless of which
> driver is used.  I don't fully understand what dependency is broken
> by unregistering the netdev prior to disabling SR-IOV, but I also
> don't see the benefit in delaying SR-IOV teardown in this call path.
> It could potentially be moved even earlier, but I'll let those more
> familiar with the hardware and code make that determination.  In any
> case, the VM behavior is substantially improved by this slight
> re-ordering.
> 
> I don't have an i40e for testing, but it already appears to disable
> SR-IOV much earlier in the unbind path, so I wouldn't expect to find
> similar issues.  Thanks,

Patch #2 does not apply cleanly, please respin this series against
my 'net' GIT tree, thanks.

Re: [PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[Alex Williamson]

On Wed, 2015-07-29 at 12:16 -0700, David Miller wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> Date: Mon, 27 Jul 2015 17:18:28 -0600
> 
> > When running a Windows 2012 R2 guest with a pair of VFs assigned
> > through vfio-pci, we run into a problem trying to hot-unplug those VFs
> > after the PF has unregistered the netdev.  This is a common scenario
> > if the PF is unbound from the driver while VFs are active.  In the
> > case of igb, the resulting guest behavior differs slightly between the
> > Microsoft provided and Intel add-on guest drivers.  With the Microsoft
> > driver, the guest seems to stumble through ejecting both VFs, but
> > takes longer than normal to do so.  With the Intel drivers, only one
> > VF is unplugged, but Device Manager still shows it as present.  The
> > second VF is non-functional but also still shown in Device Manager.
> > At this point, the guest is in such a state that it will not cleanly
> > shutdown.  With ixgbe VFs, both the Microsoft and Intel drivers take
> > on this latter behavior.
> > 
> > For both, I've found that disabling SR-IOV before unregistering the PF
> > netdev device allows the hot-unplug to proceed without interruption or
> > further ill behavior in the guest.  This is true regardless of which
> > driver is used.  I don't fully understand what dependency is broken
> > by unregistering the netdev prior to disabling SR-IOV, but I also
> > don't see the benefit in delaying SR-IOV teardown in this call path.
> > It could potentially be moved even earlier, but I'll let those more
> > familiar with the hardware and code make that determination.  In any
> > case, the VM behavior is substantially improved by this slight
> > re-ordering.
> > 
> > I don't have an i40e for testing, but it already appears to disable
> > SR-IOV much earlier in the unbind path, so I wouldn't expect to find
> > similar issues.  Thanks,
> 
> Patch #2 does not apply cleanly, please respin this series against
> my 'net' GIT tree, thanks.

I expect that's because of this patch that's in Jeff's dev-queue branch:

http://git.kernel.org/cgit/linux/kernel/git/jkirsher/next-queue.git/commit/?h=dev-queue&id=ddf766a812a13eca1116b5905e902184904266f9

I based these patches off that branch, assuming they'd take the same
route and avoid the merge conflict.  If you'd rather take these, I'll be
happy to respin.  Apologies for not noting the base branch in the
series.  Thanks,

Alex

Re: [PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[David Miller]

From: Alex Williamson <alex.williamson@redhat.com>
Date: Wed, 29 Jul 2015 13:33:07 -0600

> I expect that's because of this patch that's in Jeff's dev-queue branch:
> 
> http://git.kernel.org/cgit/linux/kernel/git/jkirsher/next-queue.git/commit/?h=dev-queue&id=ddf766a812a13eca1116b5905e902184904266f9
> 
> I based these patches off that branch, assuming they'd take the same
> route and avoid the merge conflict.  If you'd rather take these, I'll be
> happy to respin.  Apologies for not noting the base branch in the
> series.  Thanks,

No, that's fine, this would normally go via Jeff's tree anyways.

I just didn't see him take it so I assumed that it should go via me.

Re: [PATCH 0/2] igb/ixgbe: Fix ordering of SR-IOV teardown

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On Wed, 2015-07-29 at 14:31 -0700, David Miller wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> Date: Wed, 29 Jul 2015 13:33:07 -0600
> 
> > I expect that's because of this patch that's in Jeff's dev-queue branch:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/jkirsher/next-queue.git/commit/?h=dev-queue&id=ddf766a812a13eca1116b5905e902184904266f9
> > 
> > I based these patches off that branch, assuming they'd take the same
> > route and avoid the merge conflict.  If you'd rather take these, I'll be
> > happy to respin.  Apologies for not noting the base branch in the
> > series.  Thanks,
> 
> No, that's fine, this would normally go via Jeff's tree anyways.
> 
> I just didn't see him take it so I assumed that it should go via me.

Sorry, was on vacation last week and cell coverage was spotty where I
was at.  I have picked up the series.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [Intel-wired-lan] [PATCH 1/2] igb: Teardown SR-IOV before unregister_netdev()

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Alex Williamson
> Sent: Monday, July 27, 2015 4:19 PM
> To: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH 1/2] igb: Teardown SR-IOV before
> unregister_netdev()
> 
> When the .remove() callback for a PF is called, SR-IOV support for the
> device is disabled, which requires unbinding and removing the VFs.
> The VFs may be in-use either by the host kernel or userspace, such as
> assigned to a VM through vfio-pci.  In this latter case, the VFs may
> be removed either by shutting down the VM or hot-unplugging the
> devices from the VM.  Unfortunately in the case of a Windows 2012 R2
> guest, hot-unplug is broken due to the ordering of the PF driver
> teardown.  Disabling SR-IOV prior to unregister_netdev() avoids this
> issue.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>