diff mbox

upgrade to 3.8.1 : BUG Scheduling while atomic in bonding driver:

Message ID CAE3RKNnrDDh1Y_4+NAKuG3-A9qaC-mPXr7e9vefrNHHUUTE=SA@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Veaceslav Falico March 7, 2013, 2:59 p.m. UTC 
On Thu, Mar 7, 2013 at 10:50 AM, Michael Wang
<wangyun@linux.vnet.ibm.com> wrote:
> On 03/07/2013 04:50 PM, Linda Walsh wrote:
>>
>> I am *not* seeing the bug in 3.8.2 with the 2nd patch applied (in
>> addition to the first)...
>
> So that means bond lock is the reason, nice, but this is really not a
> good fix if we just unlock it...

You're right, we can't unlock bond->lock while in bond_for_each_slave(),
however I think we don't even need that bond_update_speed_duplex() in
bond_miimon_commit() - cause the speed/duplex on interface state change
were already updated via NETDEV_UP/CHANGE event in
bond_slave_netdev_event() - and if not, it's not our fault, but the
driver's that he didn't call the notifiers.

So, maybe something like this would work (any feedback welcome, it's
definitely not the first time we hit the wall with sleeping in
bond_update_speed_duplex):

Subject: [PATCH] bonding: don't call update_speed_duplex() under spinlocks

bond_update_speed_duplex() might sleep while calling underlying slave's
routines. Move it out of atomic context in bond_enslave() and remove it
from bond_miimon_commit() - it was introduced by commit 546add79, however
when the slave interfaces go up/change state it's their responsability to
fire NETDEV_UP/NETDEV_CHANGE events so that bonding can properly update
their speed.

---
 drivers/net/bonding/bond_main.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

 	new_slave->last_arp_rx = jiffies -
@@ -1798,8 +1800,6 @@ int bond_enslave(struct net_device *bond_dev,
struct net_device *slave_dev)
 		new_slave->link == BOND_LINK_DOWN ? "DOWN" :
 			(new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));

-	bond_update_speed_duplex(new_slave);
-
 	if (USES_PRIMARY(bond->params.mode) && bond->params.primary[0]) {
 		/* if there is a primary slave, remember it */
 		if (strcmp(bond->params.primary, new_slave->dev->name) == 0) {
@@ -2373,8 +2373,6 @@ static void bond_miimon_commit(struct bonding *bond)
 				bond_set_backup_slave(slave);
 			}

-			bond_update_speed_duplex(slave);
-
 			pr_info("%s: link status definitely up for interface %s, %u Mbps
%s duplex.\n",
 				bond->dev->name, slave->dev->name,
 				slave->speed, slave->duplex ? "full" : "half");
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7bd068a..c63a33c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1746,6 +1746,8 @@  int bond_enslave(struct net_device *bond_dev,
struct net_device *slave_dev)

 	bond_compute_features(bond);

+	bond_update_speed_duplex(new_slave);
+
 	read_lock(&bond->lock);