bonding: patchset for rcu use in bonding

PATCH In review Ding Tianhong 2 participants 9 messages Oct 21, 2013 – Oct 22, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2013-10-21 09:02 UTC #0 lore raw

cover bonding: patchset for rcu use in bonding

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(-)

Signature · 1 line

1.8.2.1

Veaceslav Falico

2013-10-21 09:18 UTC #1 lore raw

Quoted 4 lines · Ding Tianhong

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.

Quoted 18 lines

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(-)
Signature · 1 line
1.8.2.1

Ding Tianhong

author

2013-10-21 09:28 UTC #2 lore raw

Quoted 6 lines · Veaceslav Falico

Quoted 4 lines · Ding Tianhong
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.

Quoted 8 lines

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

Quoted 20 lines

Quoted 18 lines
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(-)
Signature · 1 line
1.8.2.1

Veaceslav Falico

2013-10-21 09:40 UTC #3 lore raw

Quoted 10 lines · Ding Tianhong

Quoted 9 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.

Quoted 8 lines

Quoted 5 lines
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()?

Quoted 25 lines

Ding
Quoted 20 lines
Quoted 18 lines
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

Ding Tianhong

author

2013-10-21 12:32 UTC #4 lore raw

Quoted 14 lines · Veaceslav Falico

Quoted 13 lines · Ding Tianhong
Quoted 12 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

Quoted 28 lines

Quoted 25 lines
Ding
Quoted 20 lines
Quoted 18 lines
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(-)
Signature · 1 line
1.8.2.1
.

Veaceslav Falico

2013-10-21 12:46 UTC #5 lore raw

Quoted 21 lines · Ding Tianhong

Quoted 20 lines · Veaceslav Falico
Quoted 18 lines · Ding Tianhong
Quoted 6 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.
Quoted 8 lines
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?

Quoted 7 lines

Quoted 2 lines
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.

Quoted 34 lines

Ding
Quoted 28 lines
Quoted 25 lines
Ding
Quoted 20 lines
Quoted 18 lines
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
.

Veaceslav Falico

2013-10-21 13:25 UTC #6 lore raw

Quoted 30 lines · Veaceslav Falico

Quoted 21 lines · Ding Tianhong
Quoted 20 lines · Veaceslav Falico
Quoted 18 lines · Ding Tianhong
Quoted 6 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.
Quoted 8 lines
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?
Quoted 7 lines
Quoted 2 lines
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.

Quoted 4 lines

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).

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.

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.

Quoted 39 lines

Quoted 35 lines
Ding
Quoted 28 lines
Quoted 25 lines
Ding
Quoted 20 lines
Quoted 18 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Veaceslav Falico

2013-10-21 13:35 UTC #7 lore raw

Quoted 31 lines · Veaceslav Falico

Quoted 30 lines · Veaceslav Falico
Quoted 28 lines · Ding Tianhong
Quoted 14 lines · Veaceslav Falico
Quoted 13 lines · Ding Tianhong
Quoted 12 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 4 lines
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:

drivers/net/bonding/bond_main.c +1 -1

index d90734f..b3923e1 100644
@@ -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.
 	 */

Ding Tianhong

author

2013-10-22 02:16 UTC #8 lore raw

In reply to Veaceslav Falico

Quoted 40 lines · Veaceslav Falico

Quoted 30 lines · Veaceslav Falico
Quoted 21 lines · Ding Tianhong
Quoted 20 lines · Veaceslav Falico
Quoted 18 lines · Ding Tianhong
Quoted 6 lines · Veaceslav Falico
Quoted 4 lines · Ding Tianhong
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.
Quoted 8 lines
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?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 4 lines
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().

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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().

Quoted 61 lines

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.
Quoted 31 lines
Quoted 31 lines
Ding
Quoted 26 lines
Quoted 25 lines
Ding
Quoted 20 lines
Quoted 18 lines
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(-)
Signature · 1 line
1.8.2.1
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
.
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
.