* [PATCH net-next 0/5] bonding: patchset for rcu use in bonding @ 2013-10-21 8:58 Ding Tianhong 2013-10-21 9:13 ` Veaceslav Falico 0 siblings, 1 reply; 9+ messages in thread From: Ding Tianhong @ 2013-10-21 8:58 UTC (permalink / raw) To: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Veaceslav Falico, Netdev Hi: The Patch Set will remove the invalid lock for bond work queue and replace it with rtnl lock, as read lock for bond could not protect slave list any more. Ding Tianhong (5): bonding: remove bond read lock for bond_mii_monitor() bonding: remove bond read lock for bond_alb_monitor() bonding: remove bond read lock for bond_loadbalance_arp_mon() bonding: remove bond read lock for bond_activebackup_arp_mon() bonding: remove bond read lock for bond_3ad_state_machine_handler() drivers/net/bonding/bond_3ad.c | 9 ++-- drivers/net/bonding/bond_alb.c | 20 ++------ drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- 3 files changed, 40 insertions(+), 89 deletions(-) -- 1.8.2.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 8:58 [PATCH net-next 0/5] bonding: patchset for rcu use in bonding Ding Tianhong @ 2013-10-21 9:13 ` Veaceslav Falico 2013-10-21 9:27 ` Ding Tianhong 0 siblings, 1 reply; 9+ messages in thread From: Veaceslav Falico @ 2013-10-21 9:13 UTC (permalink / raw) To: Ding Tianhong Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >Hi: > >The Patch Set will remove the invalid lock for bond work queue and replace it >with rtnl lock, as read lock for bond could not protect slave list any more. rtnl lock is a lot more expensive than bond lock, and not only for bond, but for all the networking stack. Why is the bond->lock invalid? It correctly protects slaves from being modified concurrently. I don't see the point in this patchset. > >Ding Tianhong (5): > bonding: remove bond read lock for bond_mii_monitor() > bonding: remove bond read lock for bond_alb_monitor() > bonding: remove bond read lock for bond_loadbalance_arp_mon() > bonding: remove bond read lock for bond_activebackup_arp_mon() > bonding: remove bond read lock for bond_3ad_state_machine_handler() > > drivers/net/bonding/bond_3ad.c | 9 ++-- > drivers/net/bonding/bond_alb.c | 20 ++------ > drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- > 3 files changed, 40 insertions(+), 89 deletions(-) > >-- >1.8.2.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 9:13 ` Veaceslav Falico @ 2013-10-21 9:27 ` Ding Tianhong 2013-10-21 9:35 ` Veaceslav Falico 0 siblings, 1 reply; 9+ messages in thread From: Ding Tianhong @ 2013-10-21 9:27 UTC (permalink / raw) To: Veaceslav Falico Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On 2013/10/21 17:13, Veaceslav Falico wrote: > On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >> Hi: >> >> The Patch Set will remove the invalid lock for bond work queue and replace it >> with rtnl lock, as read lock for bond could not protect slave list any more. > > rtnl lock is a lot more expensive than bond lock, and not only for bond, > but for all the networking stack. > > Why is the bond->lock invalid? It correctly protects slaves from being > modified concurrently. > > I don't see the point in this patchset. > yes, rtnl lock is a big lock, but I think bond->lock could not protect bond_for_each_slave any more, am I miss something? Ding >> >> Ding Tianhong (5): >> bonding: remove bond read lock for bond_mii_monitor() >> bonding: remove bond read lock for bond_alb_monitor() >> bonding: remove bond read lock for bond_loadbalance_arp_mon() >> bonding: remove bond read lock for bond_activebackup_arp_mon() >> bonding: remove bond read lock for bond_3ad_state_machine_handler() >> >> drivers/net/bonding/bond_3ad.c | 9 ++-- >> drivers/net/bonding/bond_alb.c | 20 ++------ >> drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >> 3 files changed, 40 insertions(+), 89 deletions(-) >> >> -- >> 1.8.2.1 >> >> >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 9:27 ` Ding Tianhong @ 2013-10-21 9:35 ` Veaceslav Falico 2013-10-21 12:32 ` Ding Tianhong 0 siblings, 1 reply; 9+ messages in thread From: Veaceslav Falico @ 2013-10-21 9:35 UTC (permalink / raw) To: Ding Tianhong Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >On 2013/10/21 17:13, Veaceslav Falico wrote: >> On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>> Hi: >>> >>> The Patch Set will remove the invalid lock for bond work queue and replace it >>> with rtnl lock, as read lock for bond could not protect slave list any more. >> >> rtnl lock is a lot more expensive than bond lock, and not only for bond, >> but for all the networking stack. >> >> Why is the bond->lock invalid? It correctly protects slaves from being >> modified concurrently. >> >> I don't see the point in this patchset. >> > >yes, rtnl lock is a big lock, but I think bond->lock could not protect >bond_for_each_slave any more, am I miss something? Why can't it protect bond_for_each_slave()? > >Ding > >>> >>> Ding Tianhong (5): >>> bonding: remove bond read lock for bond_mii_monitor() >>> bonding: remove bond read lock for bond_alb_monitor() >>> bonding: remove bond read lock for bond_loadbalance_arp_mon() >>> bonding: remove bond read lock for bond_activebackup_arp_mon() >>> bonding: remove bond read lock for bond_3ad_state_machine_handler() >>> >>> drivers/net/bonding/bond_3ad.c | 9 ++-- >>> drivers/net/bonding/bond_alb.c | 20 ++------ >>> drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >>> 3 files changed, 40 insertions(+), 89 deletions(-) >>> >>> -- >>> 1.8.2.1 >>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 9:35 ` Veaceslav Falico @ 2013-10-21 12:32 ` Ding Tianhong 2013-10-21 12:41 ` Veaceslav Falico 0 siblings, 1 reply; 9+ messages in thread From: Ding Tianhong @ 2013-10-21 12:32 UTC (permalink / raw) To: Veaceslav Falico Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On 2013/10/21 17:35, Veaceslav Falico wrote: > On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >> On 2013/10/21 17:13, Veaceslav Falico wrote: >>> On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>>> Hi: >>>> >>>> The Patch Set will remove the invalid lock for bond work queue and replace it >>>> with rtnl lock, as read lock for bond could not protect slave list any more. >>> >>> rtnl lock is a lot more expensive than bond lock, and not only for bond, >>> but for all the networking stack. >>> >>> Why is the bond->lock invalid? It correctly protects slaves from being >>> modified concurrently. >>> >>> I don't see the point in this patchset. >>> >> >> yes, rtnl lock is a big lock, but I think bond->lock could not protect >> bond_for_each_slave any more, am I miss something? > > Why can't it protect bond_for_each_slave()? > bond_master_upper_dev_link() and bond_upper_dev_unlink() was only in rtnl lock, bond_for_each_slave may changed while loop in bond read lock, but it sees that nothing serious will happen yet. Maybe I miss something. Ding >> >> Ding >> >>>> >>>> Ding Tianhong (5): >>>> bonding: remove bond read lock for bond_mii_monitor() >>>> bonding: remove bond read lock for bond_alb_monitor() >>>> bonding: remove bond read lock for bond_loadbalance_arp_mon() >>>> bonding: remove bond read lock for bond_activebackup_arp_mon() >>>> bonding: remove bond read lock for bond_3ad_state_machine_handler() >>>> >>>> drivers/net/bonding/bond_3ad.c | 9 ++-- >>>> drivers/net/bonding/bond_alb.c | 20 ++------ >>>> drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >>>> 3 files changed, 40 insertions(+), 89 deletions(-) >>>> >>>> -- >>>> 1.8.2.1 >>>> >>>> >>>> >>> >>> >> >> > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 12:32 ` Ding Tianhong @ 2013-10-21 12:41 ` Veaceslav Falico 2013-10-21 13:21 ` Veaceslav Falico 0 siblings, 1 reply; 9+ messages in thread From: Veaceslav Falico @ 2013-10-21 12:41 UTC (permalink / raw) To: Ding Tianhong Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On Mon, Oct 21, 2013 at 08:32:11PM +0800, Ding Tianhong wrote: >On 2013/10/21 17:35, Veaceslav Falico wrote: >> On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >>> On 2013/10/21 17:13, Veaceslav Falico wrote: >>>> On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>>>> Hi: >>>>> >>>>> The Patch Set will remove the invalid lock for bond work queue and replace it >>>>> with rtnl lock, as read lock for bond could not protect slave list any more. >>>> >>>> rtnl lock is a lot more expensive than bond lock, and not only for bond, >>>> but for all the networking stack. >>>> >>>> Why is the bond->lock invalid? It correctly protects slaves from being >>>> modified concurrently. >>>> >>>> I don't see the point in this patchset. >>>> >>> >>> yes, rtnl lock is a big lock, but I think bond->lock could not protect >>> bond_for_each_slave any more, am I miss something? >> >> Why can't it protect bond_for_each_slave()? >> > >bond_master_upper_dev_link() and bond_upper_dev_unlink() was only in rtnl lock, >bond_for_each_slave may changed while loop in bond read lock, but it sees that >nothing serious will happen yet. >Maybe I miss something. Even if it is unsafe to use bond_for_each_slave() while holding bond->lock - it means that we must protect the list by locking the bond_upper_dev_(un)link() via bond->lock, but not by removing bond->lock from everywhere where it is now. And I'm not that sure if it's safe or not. > >Ding > > >>> >>> Ding >>> >>>>> >>>>> Ding Tianhong (5): >>>>> bonding: remove bond read lock for bond_mii_monitor() >>>>> bonding: remove bond read lock for bond_alb_monitor() >>>>> bonding: remove bond read lock for bond_loadbalance_arp_mon() >>>>> bonding: remove bond read lock for bond_activebackup_arp_mon() >>>>> bonding: remove bond read lock for bond_3ad_state_machine_handler() >>>>> >>>>> drivers/net/bonding/bond_3ad.c | 9 ++-- >>>>> drivers/net/bonding/bond_alb.c | 20 ++------ >>>>> drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >>>>> 3 files changed, 40 insertions(+), 89 deletions(-) >>>>> >>>>> -- >>>>> 1.8.2.1 >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> . >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 12:41 ` Veaceslav Falico @ 2013-10-21 13:21 ` Veaceslav Falico 2013-10-21 13:31 ` Veaceslav Falico 2013-10-22 2:16 ` Ding Tianhong 0 siblings, 2 replies; 9+ messages in thread From: Veaceslav Falico @ 2013-10-21 13:21 UTC (permalink / raw) To: Ding Tianhong Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On Mon, Oct 21, 2013 at 02:41:44PM +0200, Veaceslav Falico wrote: >On Mon, Oct 21, 2013 at 08:32:11PM +0800, Ding Tianhong wrote: >>On 2013/10/21 17:35, Veaceslav Falico wrote: >>>On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >>>>On 2013/10/21 17:13, Veaceslav Falico wrote: >>>>>On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>>>>>Hi: >>>>>> >>>>>>The Patch Set will remove the invalid lock for bond work queue and replace it >>>>>>with rtnl lock, as read lock for bond could not protect slave list any more. >>>>> >>>>>rtnl lock is a lot more expensive than bond lock, and not only for bond, >>>>>but for all the networking stack. >>>>> >>>>>Why is the bond->lock invalid? It correctly protects slaves from being >>>>>modified concurrently. >>>>> >>>>>I don't see the point in this patchset. >>>>> >>>> >>>>yes, rtnl lock is a big lock, but I think bond->lock could not protect >>>>bond_for_each_slave any more, am I miss something? >>> >>>Why can't it protect bond_for_each_slave()? >>> >> >>bond_master_upper_dev_link() and bond_upper_dev_unlink() was only in rtnl lock, >>bond_for_each_slave may changed while loop in bond read lock, but it sees that >>nothing serious will happen yet. >>Maybe I miss something. > >Even if it is unsafe to use bond_for_each_slave() while holding bond->lock >- it means that we must protect the list by locking the >bond_upper_dev_(un)link() via bond->lock, but not by removing bond->lock >from everywhere where it is now. And I'm not that sure if it's safe or not. I've quickly looked over the code - yes, theoretically we could race between bond_for_each_slave() that is not rtnl-protected and bond_upper_dev_(un)link(). Your patchset, also, doesn't 'replace' bond->lock with rtnl_lock(), cause everywhere the rtnl_lock() is already present, it just moves it around, while removing the bond->lock. The commit message is wrong and says actually nothing why is it done, how is it done, why it's safe to do so and what do we get in the end. For every patch, and the cover letter is also not an exception. I'd suggest you either: 1) add bond->lock around bond_upper_dev_(un)link() (GFP_ATOMIC might be needed). or 2) add ASSERT_RTNL() to bond_for_each_slave() macro, catch all the offenders and remove the bond->lock from them. Also, I'm not that sure that it's safe to do so - cause one of the slaves (not the slave list) might change, and we might have race conditions there. or 3) move bond_for_each_slave() to bond_for_each_slave_rcu() where appropriate. And in any case write specific commit messages - bonding's code is really old and full of locks that were placed for some reason (and the reason might have gone away long ago, too), so it's really hard to say if the change is safe or not. I'd personally go for either 3) (preferred), or 1). Sorry, I'm a bit tired of going in-depth on your patches. Start either doing patches with commit messages that *prove* me that you're right (I'll, obviously, verify it - but at least I'll know what you're doing, and won't have to figure it out from code), or I'll start explicitly NAKing them. Sorry again, but I don't really have time for that. I didn't have time to review your last patchset (RCUifying the remaining transmit path), and now I can understand nothing from their commit description, except that you've changed bond_for_each_slave() to bond_for_each_slave_rcu() and bond->lock to rcu_read_lock(). I'm not saying that they're wrong, just that they're really hard to understand. So, please, start writing commit messages. > >> >>Ding >> >> >>>> >>>>Ding >>>> >>>>>> >>>>>>Ding Tianhong (5): >>>>>> bonding: remove bond read lock for bond_mii_monitor() >>>>>> bonding: remove bond read lock for bond_alb_monitor() >>>>>> bonding: remove bond read lock for bond_loadbalance_arp_mon() >>>>>> bonding: remove bond read lock for bond_activebackup_arp_mon() >>>>>> bonding: remove bond read lock for bond_3ad_state_machine_handler() >>>>>> >>>>>>drivers/net/bonding/bond_3ad.c | 9 ++-- >>>>>>drivers/net/bonding/bond_alb.c | 20 ++------ >>>>>>drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >>>>>>3 files changed, 40 insertions(+), 89 deletions(-) >>>>>> >>>>>>-- >>>>>>1.8.2.1 >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> >>>. >>> >> >> >-- >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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 13:21 ` Veaceslav Falico @ 2013-10-21 13:31 ` Veaceslav Falico 2013-10-22 2:16 ` Ding Tianhong 1 sibling, 0 replies; 9+ messages in thread From: Veaceslav Falico @ 2013-10-21 13:31 UTC (permalink / raw) To: Ding Tianhong Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On Mon, Oct 21, 2013 at 03:21:36PM +0200, Veaceslav Falico wrote: >On Mon, Oct 21, 2013 at 02:41:44PM +0200, Veaceslav Falico wrote: >>On Mon, Oct 21, 2013 at 08:32:11PM +0800, Ding Tianhong wrote: >>>On 2013/10/21 17:35, Veaceslav Falico wrote: >>>>On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >>>>>On 2013/10/21 17:13, Veaceslav Falico wrote: >>>>>>On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>>>>>>Hi: >>>>>>> >>>>>>>The Patch Set will remove the invalid lock for bond work queue and replace it >>>>>>>with rtnl lock, as read lock for bond could not protect slave list any more. >>>>>> >>>>>>rtnl lock is a lot more expensive than bond lock, and not only for bond, >>>>>>but for all the networking stack. >>>>>> >>>>>>Why is the bond->lock invalid? It correctly protects slaves from being >>>>>>modified concurrently. >>>>>> >>>>>>I don't see the point in this patchset. >>>>>> >>>>> >>>>>yes, rtnl lock is a big lock, but I think bond->lock could not protect >>>>>bond_for_each_slave any more, am I miss something? >>>> >>>>Why can't it protect bond_for_each_slave()? >>>> >>> >>>bond_master_upper_dev_link() and bond_upper_dev_unlink() was only in rtnl lock, >>>bond_for_each_slave may changed while loop in bond read lock, but it sees that >>>nothing serious will happen yet. >>>Maybe I miss something. >> >>Even if it is unsafe to use bond_for_each_slave() while holding bond->lock >>- it means that we must protect the list by locking the >>bond_upper_dev_(un)link() via bond->lock, but not by removing bond->lock >>from everywhere where it is now. And I'm not that sure if it's safe or not. > >I've quickly looked over the code - yes, theoretically we could race >between bond_for_each_slave() that is not rtnl-protected and >bond_upper_dev_(un)link(). For this race, btw, it's enough to apply the following patch, and we're good (we don't care if we add a slave whilst bond_for_each_slave()) - untested patch: diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d90734f..b3923e1 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1737,10 +1737,10 @@ static int __bond_release_one(struct net_device *bond_dev, unblock_netpoll_tx(); return -EINVAL; } + bond_upper_dev_unlink(bond_dev, slave_dev); write_unlock_bh(&bond->lock); - bond_upper_dev_unlink(bond_dev, slave_dev); /* unregister rx_handler early so bond_handle_frame wouldn't be called * for this slave anymore. */ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/5] bonding: patchset for rcu use in bonding 2013-10-21 13:21 ` Veaceslav Falico 2013-10-21 13:31 ` Veaceslav Falico @ 2013-10-22 2:16 ` Ding Tianhong 1 sibling, 0 replies; 9+ messages in thread From: Ding Tianhong @ 2013-10-22 2:16 UTC (permalink / raw) To: Veaceslav Falico Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Nikolay Aleksandrov, Netdev On 2013/10/21 21:21, Veaceslav Falico wrote: > On Mon, Oct 21, 2013 at 02:41:44PM +0200, Veaceslav Falico wrote: >> On Mon, Oct 21, 2013 at 08:32:11PM +0800, Ding Tianhong wrote: >>> On 2013/10/21 17:35, Veaceslav Falico wrote: >>>> On Mon, Oct 21, 2013 at 05:27:51PM +0800, Ding Tianhong wrote: >>>>> On 2013/10/21 17:13, Veaceslav Falico wrote: >>>>>> On Mon, Oct 21, 2013 at 04:58:36PM +0800, Ding Tianhong wrote: >>>>>>> Hi: >>>>>>> >>>>>>> The Patch Set will remove the invalid lock for bond work queue and replace it >>>>>>> with rtnl lock, as read lock for bond could not protect slave list any more. >>>>>> >>>>>> rtnl lock is a lot more expensive than bond lock, and not only for bond, >>>>>> but for all the networking stack. >>>>>> >>>>>> Why is the bond->lock invalid? It correctly protects slaves from being >>>>>> modified concurrently. >>>>>> >>>>>> I don't see the point in this patchset. >>>>>> >>>>> >>>>> yes, rtnl lock is a big lock, but I think bond->lock could not protect >>>>> bond_for_each_slave any more, am I miss something? >>>> >>>> Why can't it protect bond_for_each_slave()? >>>> >>> >>> bond_master_upper_dev_link() and bond_upper_dev_unlink() was only in rtnl lock, >>> bond_for_each_slave may changed while loop in bond read lock, but it sees that >>> nothing serious will happen yet. >>> Maybe I miss something. >> >> Even if it is unsafe to use bond_for_each_slave() while holding bond->lock >> - it means that we must protect the list by locking the >> bond_upper_dev_(un)link() via bond->lock, but not by removing bond->lock >> from everywhere where it is now. And I'm not that sure if it's safe or not. > > I've quickly looked over the code - yes, theoretically we could race > between bond_for_each_slave() that is not rtnl-protected and > bond_upper_dev_(un)link(). > > Your patchset, also, doesn't 'replace' bond->lock with rtnl_lock(), cause > everywhere the rtnl_lock() is already present, it just moves it around, > while removing the bond->lock. > > The commit message is wrong and says actually nothing why is it done, how > is it done, why it's safe to do so and what do we get in the end. For every > patch, and the cover letter is also not an exception. > It is my fault, too lazy to describe the detail for the patch and occurs so many missmatch, I'll fix it later. > I'd suggest you either: > > 1) add bond->lock around bond_upper_dev_(un)link() (GFP_ATOMIC might be needed). > bond_upper_dev_(un)link() will call call_netdevice_notifiers(), it is not safe to call it in read lock, call_netdevice_notifiers() may sleep or schedule, and sometimes call_netdevice_notifiers() will read bond lock again. > or > > 2) add ASSERT_RTNL() to bond_for_each_slave() macro, catch all the offenders > and remove the bond->lock from them. Also, I'm not that sure that it's safe > to do so - cause one of the slaves (not the slave list) might change, and > we might have race conditions there. > yes, it is not a good idea, as your meaning, it is a big cost, but monitor is a slow path, I think the cost is acceptable,whatever we should find a better way. > or > > 3) move bond_for_each_slave() to bond_for_each_slave_rcu() where appropriate. > > And in any case write specific commit messages - bonding's code is really > old and full of locks that were placed for some reason (and the reason > might have gone away long ago, too), so it's really hard to say if the > change is safe or not. above all, the 3) is the wise idea, rtnl or rcu, every one is enough for bond_for_each_slave(). > > I'd personally go for either 3) (preferred), or 1). > > Sorry, I'm a bit tired of going in-depth on your patches. Start either > doing patches with commit messages that *prove* me that you're right (I'll, > obviously, verify it - but at least I'll know what you're doing, and won't > have to figure it out from code), or I'll start explicitly NAKing them. > > Sorry again, but I don't really have time for that. I didn't have time to > review your last patchset (RCUifying the remaining transmit path), and now > I can understand nothing from their commit description, except that you've > changed bond_for_each_slave() to bond_for_each_slave_rcu() and bond->lock > to rcu_read_lock(). I'm not saying that they're wrong, just that they're > really hard to understand. > > So, please, start writing commit messages. > >> >>> >>> Ding >>> >>> >>>>> >>>>> Ding >>>>> >>>>>>> >>>>>>> Ding Tianhong (5): >>>>>>> bonding: remove bond read lock for bond_mii_monitor() >>>>>>> bonding: remove bond read lock for bond_alb_monitor() >>>>>>> bonding: remove bond read lock for bond_loadbalance_arp_mon() >>>>>>> bonding: remove bond read lock for bond_activebackup_arp_mon() >>>>>>> bonding: remove bond read lock for bond_3ad_state_machine_handler() >>>>>>> >>>>>>> drivers/net/bonding/bond_3ad.c | 9 ++-- >>>>>>> drivers/net/bonding/bond_alb.c | 20 ++------ >>>>>>> drivers/net/bonding/bond_main.c | 100 +++++++++++++--------------------------- >>>>>>> 3 files changed, 40 insertions(+), 89 deletions(-) >>>>>>> >>>>>>> -- >>>>>>> 1.8.2.1 >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> . >>>> >>> >>> >> -- >> 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-10-22 2:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-21 8:58 [PATCH net-next 0/5] bonding: patchset for rcu use in bonding Ding Tianhong 2013-10-21 9:13 ` Veaceslav Falico 2013-10-21 9:27 ` Ding Tianhong 2013-10-21 9:35 ` Veaceslav Falico 2013-10-21 12:32 ` Ding Tianhong 2013-10-21 12:41 ` Veaceslav Falico 2013-10-21 13:21 ` Veaceslav Falico 2013-10-21 13:31 ` Veaceslav Falico 2013-10-22 2:16 ` Ding Tianhong
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).