bonding: don't call slave_xxx_netpoll under spinlocks
Subsystems (3)
PATCH 1/4 bonding: don't call slave_xxx_netpoll under spinlocks
the slave_xxx_netpoll may sleep, so it should't be called under spinlocks.
the slave point of the bonding will not be changed outside rtnl lock, so rtnl lock is enough here.
--- drivers/net/bonding/bond_main.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
drivers/net/bonding/bond_main.c +3 -12
@@ -1249,8 +1249,9 @@ static void bond_poll_controller(struct net_device *bond_dev) { } -static void __bond_netpoll_cleanup (struct bonding *bond) +static void bond_netpoll_cleanup (struct net_device *bond_dev) { +struct bonding *bond =netdev_priv (bond_dev);struct slave *slave;int i;
@@ -1258,14 +1259,6 @@ static void __bond_netpoll_cleanup(struct bonding *bond)if (IS_UP (slave->dev))slave_disable_netpoll (slave); } -static void bond_netpoll_cleanup (struct net_device *bond_dev) -{ -struct bonding *bond =netdev_priv (bond_dev); - -read_lock (&bond->lock); -__bond_netpoll_cleanup (bond); -read_unlock (&bond->lock); -}static int bond_netpoll_setup (struct net_device *dev,struct netpoll_info *ni,gfp_t gfp) {
@@ -1273,15 +1266,13 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, gstruct slave *slave;int i, err =0 ; -read_lock (&bond->lock);bond_for_each_slave (bond, slave, i) { err =slave_enable_netpoll (slave);if (err) { -__bond_netpoll_cleanup (bond); +bond_netpoll_cleanup (dev);break ; } } -read_unlock (&bond->lock);return err; }
Signature · 1 line
1.8.0
Quoted 1 line · dingtianhong
the
slave_xxx_netpollmay sleep, so it should't be called under spinlocks.
I don't really see how it may sleep, it was specifically changed to not sleep actually. However, see below...
Quoted 3 lines
the slave point of the bonding will not be changed outside rtnl lock, so rtnl lock is enough here.
Yep, as far as I see there's really no need to take the lock, both the slave list and the netpoll part are always protected by rtnl lock, unless I'm missing something, and indeed .ndo_netpoll_setup() is always called under rtnl.
BTW, bond_netpoll_cleanup() has the same problem - maybe you could check if we can remove the bond->lock from there also and update the patch?
Quoted 54 lines
Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 15 +++------------1 file changed, 3 insertions(+), 12 deletions(-)drivers/net/bonding/bond_main.c +3
-12@@ -1249,8 +1249,9 @@ static void bond_poll_controller(struct net_device *bond_dev) { } -static void __bond_netpoll_cleanup (struct bonding *bond) +static void bond_netpoll_cleanup (struct net_device *bond_dev) { +struct bonding *bond =netdev_priv (bond_dev);struct slave *slave;int i;@@ -1258,14 +1259,6 @@ static void __bond_netpoll_cleanup(struct bonding *bond)if (IS_UP (slave->dev))slave_disable_netpoll (slave); } -static void bond_netpoll_cleanup (struct net_device *bond_dev) -{ -struct bonding *bond =netdev_priv (bond_dev); - -read_lock (&bond->lock); -__bond_netpoll_cleanup (bond); -read_unlock (&bond->lock); -}static int bond_netpoll_setup (struct net_device *dev,struct netpoll_info *ni,gfp_t gfp) {@@ -1273,15 +1266,13 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, gstruct slave *slave;int i, err =0 ; -read_lock (&bond->lock);bond_for_each_slave (bond, slave, i) { err =slave_enable_netpoll (slave);if (err) { -__bond_netpoll_cleanup (bond); +bond_netpoll_cleanup (dev);break ; } } -read_unlock (&bond->lock);return err; }
Quoted 6 lines · Veaceslav Falico
Quoted 1 line · dingtianhong
the
slave_xxx_netpollmay sleep, so it should't be called under spinlocks.I don't really see how it may sleep, it was specifically changed to not sleep actually. However, see below...
I think the synchronize_rcu_bh() in slave disable_netpoll will sched and speed,so spinlock should not used here.
Quoted 4 lines
Quoted 3 lines
the slave point of the bonding will not be changed outside rtnl lock, so rtnl lock is enough here.
Quoted 8 lines
Yep, as far as I see there's really no need to take the lock, both the slave list and the netpoll part are always protected by rtnl lock, unless I'm missing something, and indeed .ndo_netpoll_setup() is always called under rtnl.
BTW,
bond_netpoll_cleanup()has the same problem - maybe you could check if we can remove thebond->lockfrom there also and update the patch?
yes, this patch has remove bond_netpoll_cleanup(), and change _bond_netpoll_cleanup() to bond_netpoll_cleanup(), rtnl lock is enough here.
Quoted 57 lines
Quoted 44 lines
Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 15 +++------------1 file changed, 3 insertions(+), 12 deletions(-)drivers/net/bonding/bond_main.c +2
-10@@ -1249,8 +1249,9 @@ static void bond_poll_controller(struct net_device *bond_dev) { } -static void __bond_netpoll_cleanup (struct bonding *bond) +static void bond_netpoll_cleanup (struct net_device *bond_dev) { +struct bonding *bond =netdev_priv (bond_dev);struct slave *slave;int i;@@ -1258,14 +1259,6 @@ static void __bond_netpoll_cleanup(struct bonding *bond)if (IS_UP (slave->dev))slave_disable_netpoll (slave); } -static void bond_netpoll_cleanup (struct net_device *bond_dev) -{ -struct bonding *bond =netdev_priv (bond_dev); - -read_lock (&bond->lock); -__bond_netpoll_cleanup (bond); -read_unlock (&bond->lock); -}static int bond_netpoll_setup (struct net_device *dev,struct netpoll_info *ni,gfp_t gfp) {@@ -1273,15 +1266,13 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, gstruct slave *slave;int i, err =0 ; -read_lock (&bond->lock);bond_for_each_slave (bond, slave, i) { err =slave_enable_netpoll (slave);drivers/net/bonding/bond_main.c static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, g · line 1278
Quoted 10 lines
drivers/net/bonding/bond_main.c +1
-2if (err) { -__bond_netpoll_cleanup (bond); +bond_netpoll_cleanup (dev);break ; } } -read_unlock (&bond->lock);return err; }.
David Miller
maintainerI'm waiting for a reposting of this entire series with the requested changes made to patch #1.
Thanks.