Message ID | 51EA3B03.7010302@huawei.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, Jul 20, 2013 at 03:23:47PM +0800, dingtianhong wrote: >the slave_xxx_netpoll may 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... > >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? > >Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > >--- >drivers/net/bonding/bond_main.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 07f257d4..5eb75ef 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -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, g > struct 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; > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/7/20 18:38, Veaceslav Falico wrote: > On Sat, Jul 20, 2013 at 03:23:47PM +0800, dingtianhong wrote: >> the slave_xxx_netpoll may 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. >> >> 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? > yes, this patch has remove bond_netpoll_cleanup(), and change _bond_netpoll_cleanup() to bond_netpoll_cleanup(), rtnl lock is enough here. >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> >> --- >> drivers/net/bonding/bond_main.c | 15 +++------------ >> 1 file changed, 3 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 07f257d4..5eb75ef 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -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, g >> struct 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; >> } >> > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm waiting for a reposting of this entire series with the requested changes made to patch #1. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 07f257d4..5eb75ef 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -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, g struct 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; }
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. Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- drivers/net/bonding/bond_main.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)