* [PATCH 1/4] bonding: don't call slave_xxx_netpoll under spinlocks @ 2013-07-20 7:23 Ding Tianhong 2013-07-20 10:38 ` [1/4] " Veaceslav Falico 2013-07-22 22:48 ` [PATCH 1/4] " David Miller 0 siblings, 2 replies; 4+ messages in thread From: Ding Tianhong @ 2013-07-20 7:23 UTC (permalink / raw) To: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev 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(-) 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; } -- 1.8.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [1/4] bonding: don't call slave_xxx_netpoll under spinlocks 2013-07-20 7:23 [PATCH 1/4] bonding: don't call slave_xxx_netpoll under spinlocks Ding Tianhong @ 2013-07-20 10:38 ` Veaceslav Falico 2013-07-22 0:40 ` Ding Tianhong 2013-07-22 22:48 ` [PATCH 1/4] " David Miller 1 sibling, 1 reply; 4+ messages in thread From: Veaceslav Falico @ 2013-07-20 10:38 UTC (permalink / raw) To: dingtianhong; +Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev 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; > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [1/4] bonding: don't call slave_xxx_netpoll under spinlocks 2013-07-20 10:38 ` [1/4] " Veaceslav Falico @ 2013-07-22 0:40 ` Ding Tianhong 0 siblings, 0 replies; 4+ messages in thread From: Ding Tianhong @ 2013-07-22 0:40 UTC (permalink / raw) To: Veaceslav Falico; +Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev 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; >> } >> > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/4] bonding: don't call slave_xxx_netpoll under spinlocks 2013-07-20 7:23 [PATCH 1/4] bonding: don't call slave_xxx_netpoll under spinlocks Ding Tianhong 2013-07-20 10:38 ` [1/4] " Veaceslav Falico @ 2013-07-22 22:48 ` David Miller 1 sibling, 0 replies; 4+ messages in thread From: David Miller @ 2013-07-22 22:48 UTC (permalink / raw) To: dingtianhong; +Cc: fubar, andy, netdev I'm waiting for a reposting of this entire series with the requested changes made to patch #1. Thanks. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-07-22 22:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-20 7:23 [PATCH 1/4] bonding: don't call slave_xxx_netpoll under spinlocks Ding Tianhong 2013-07-20 10:38 ` [1/4] " Veaceslav Falico 2013-07-22 0:40 ` Ding Tianhong 2013-07-22 22:48 ` [PATCH 1/4] " David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).