[1/2] ixgbe: release lock for the duration of ixgbe_suspend_close()

Message ID 20180503035931.22439-2-pasha.tatashin@oracle.com
State Changes Requested
Delegated to: Jeff Kirsher
Headers show
Series
  • multi-threading device shutdown
Related show

Commit Message

Pavel Tatashin May 3, 2018, 3:59 a.m.
Currently, during device_shutdown() ixgbe holds rtnl_lock for the duration
of lengthy ixgbe_close_suspend(). On machines with multiple ixgbe cards
this lock prevents scaling if device_shutdown() function is multi-threaded.

It is not necessary to hold this lock during ixgbe_close_suspend()
as it is not held when ixgbe_close() is called also during shutdown but for
kexec case.

Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Pavel Tatashin May 3, 2018, 1:30 p.m. | #1
Hi Alex,

> I'm not a fan of dropping the mutex while we go through
> ixgbe_close_suspend. I'm concerned it will result in us having a
> number of races on shutdown.

I would agree, but ixgbe_close_suspend() is already called without this
mutex from ixgbe_close(). This path is executed also during machine
shutdown but when kexec'ed. So, it is either an existing bug or there are
no races. But, because ixgbe_close() is called from the userland, and a
little earlier than ixgbe_shutdown() I think this means there are no races.


> If anything, I think we would need to find a replacement for the mutex
> that can operate at the per-netdev level if you are wanting to
> parallelize this.

Yes, if lock is necessary, it can be replaced in this place (and added to
ixgbe_close())  with something scalable.

Thank you,
Pavel
Bowers, AndrewX May 15, 2018, 10:53 p.m. | #2
> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> Behalf Of Pavel Tatashin
> Sent: Wednesday, May 2, 2018 9:00 PM
> To: pasha.tatashin@oracle.com; steven.sistare@oracle.com;
> daniel.m.jordan@oracle.com; linux-kernel@vger.kernel.org; Kirsher, Jeffrey
> T <jeffrey.t.kirsher@intel.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; gregkh@linuxfoundation.org
> Subject: [Intel-wired-lan] [PATCH 1/2] ixgbe: release lock for the duration of
> ixgbe_suspend_close()
> 
> Currently, during device_shutdown() ixgbe holds rtnl_lock for the duration of
> lengthy ixgbe_close_suspend(). On machines with multiple ixgbe cards this
> lock prevents scaling if device_shutdown() function is multi-threaded.
> 
> It is not necessary to hold this lock during ixgbe_close_suspend() as it is not
> held when ixgbe_close() is called also during shutdown but for kexec case.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index afadba99f7b8..e7875b58854b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6748,8 +6748,15 @@  static int __ixgbe_shutdown(struct pci_dev *pdev, bool *enable_wake)
 	rtnl_lock();
 	netif_device_detach(netdev);
 
-	if (netif_running(netdev))
+	if (netif_running(netdev)) {
+		/* Suspend takes a long time, device_shutdown may be
+		 * parallelized this function, so drop lock for the
+		 * duration of this call.
+		 */
+		rtnl_unlock();
 		ixgbe_close_suspend(adapter);
+		rtnl_lock();
+	}
 
 	ixgbe_clear_interrupt_scheme(adapter);
 	rtnl_unlock();