Patchwork 2.6.27.18: bnx2/tg3: BUG: "scheduling while atomic" trying to ifenslave a second interface to my bond

login
register
mail settings
Submitter Jay Vosburgh
Date April 15, 2009, 1:12 a.m.
Message ID <11276.1239757967@death.nxdomain.ibm.com>
Download mbox | patch
Permalink /patch/25954/
State RFC
Delegated to: David Miller
Headers show

Comments

Jay Vosburgh - April 15, 2009, 1:12 a.m.
Paul Smith <paul@mad-scientist.net> wrote:
[...]
>As soon as I try to ifenslave the second interface, Badness Ensues:
>
>        # ifenslave bond0 eth0
>        ------------[ cut here ]------------
>        WARNING: at linux/kernel/sched.c:4303 local_bh_enable_ip+0x2c/0xc0()
>        Modules linked in: rng_core dock scsi_mod libata ata_piix zlib_inflate bnx2 ipmi_msghandler ipmi_si ipmi_devintf bonding
>        Pid: 1552, comm: ifenslave Not tainted 2.6.27.18-WR3.0bg_small #1
>        
>        Call Trace:
>         [<ffffffff8023be34>] warn_on_slowpath+0x64/0xb0
>         [<ffffffff8028654a>] get_page_from_freelist+0x30a/0x640
>         [<ffffffff8041497a>] __dev_get_by_name+0x9a/0xc0
>         [<ffffffff80419a66>] dev_ethtool+0xd46/0x11c0
>         [<ffffffff8027fc7a>] find_get_page+0x9a/0xe0
>         [<ffffffff802800c3>] find_lock_page+0x23/0x80
>         [<ffffffff8024233c>] local_bh_enable_ip+0x2c/0xc0
>         [<ffffffffa00ad780>] bond_alb_set_mac_address+0x2a0/0x2f0 [bonding]
[...]

	I think I know what's going on.  I believe this patch will
resolve things, but I won't be able to test it until tomorrow.  If you
want to test this, great; if you want to wait, that's fine too.




	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
--
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
David Miller - April 15, 2009, 3:23 a.m.
From: Jay Vosburgh <fubar@us.ibm.com>
Date: Tue, 14 Apr 2009 18:12:47 -0700

> 	I think I know what's going on.  I believe this patch will
> resolve things, but I won't be able to test it until tomorrow.  If you
> want to test this, great; if you want to wait, that's fine too.

Jay, thanks for working on this.

Let me know when you have a final version of this fix for me
to include.
--
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
Paul Smith - April 15, 2009, 5:29 a.m.
On Tue, 2009-04-14 at 18:12 -0700, Jay Vosburgh wrote:
> 	I think I know what's going on.  I believe this patch will
> resolve things, but I won't be able to test it until tomorrow.  If you
> want to test this, great; if you want to wait, that's fine too.

I tested this; it works great.  All my systems came up fine with this
change applied.  Thanks!

> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 8dc6fbb..b22467a 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -1708,10 +1708,8 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
>   * Called with RTNL
>   */
>  int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
> -	__releases(&bond->curr_slave_lock)
> -	__releases(&bond->lock)
>  	__acquires(&bond->lock)
> -	__acquires(&bond->curr_slave_lock)
> +	__releases(&bond->lock)
>  {
>  	struct bonding *bond = netdev_priv(bond_dev);
>  	struct sockaddr *sa = addr;
> @@ -1747,9 +1745,6 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
>  		}
>  	}
>  
> -	write_unlock_bh(&bond->curr_slave_lock);
> -	read_unlock(&bond->lock);
> -
>  	if (swap_slave) {
>  		alb_swap_mac_addr(bond, swap_slave, bond->curr_active_slave);
>  		alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave);
> @@ -1757,16 +1752,17 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
>  		alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr,
>  				       bond->alb_info.rlb_enabled);
>  
> -		alb_send_learning_packets(bond->curr_active_slave, bond_dev->dev_addr);
> +		read_lock(&bond->lock);
> +		alb_send_learning_packets(bond->curr_active_slave,
> +					  bond_dev->dev_addr);
>  		if (bond->alb_info.rlb_enabled) {
>  			/* inform clients mac address has changed */
> -			rlb_req_update_slave_clients(bond, bond->curr_active_slave);
> +			rlb_req_update_slave_clients(bond,
> +						     bond->curr_active_slave);
>  		}
> +		read_unlock(&bond->lock);
>  	}
>  
> -	read_lock(&bond->lock);
> -	write_lock_bh(&bond->curr_slave_lock);
> -
>  	return 0;
>  }
>
Paul Smith - April 15, 2009, 4:56 p.m.
On Tue, 2009-04-14 at 18:12 -0700, Jay Vosburgh wrote:
> 	I think I know what's going on.  I believe this patch will
> resolve things, but I won't be able to test it until tomorrow.  If you
> want to test this, great; if you want to wait, that's fine too.

Hi Jay; as I mentioned last night this patch is working fine for me so
far.

However, looking at the rest of this function it seems to me that there
are other locking issues, at least based on the documentation in the
header file:

 * Here are the locking policies for the two bonding locks:
 *
 * 1) Get bond->lock when reading/writing slave list.
 * 2) Get bond->curr_slave_lock when reading/writing bond->curr_active_slave.
 *    (It is unnecessary when the write-lock is put with bond->lock.)
 * 3) When we lock with bond->curr_slave_lock, we must lock with bond->lock
 *    beforehand.

For example, don't you need to hold bond->curr_slave_lock at least
around the "if (!bond->curr_active_slave)"?  What about around the
"bond_for_each_slave" loop?

Many of the other functions, later, also seem to work with
bond->curr_active_slave and they don't take this lock.

Unless I'm missing something, I think there are still more problems in
the locking in bond_alb_set_mac_address().

Thoughts?

> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 8dc6fbb..b22467a 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -1708,10 +1708,8 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
>   * Called with RTNL
>   */
>  int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
> -	__releases(&bond->curr_slave_lock)
> -	__releases(&bond->lock)
>  	__acquires(&bond->lock)
> -	__acquires(&bond->curr_slave_lock)
> +	__releases(&bond->lock)
>  {
>  	struct bonding *bond = netdev_priv(bond_dev);
>  	struct sockaddr *sa = addr;
> @@ -1747,9 +1745,6 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
>  		}
>  	}
>  
> -	write_unlock_bh(&bond->curr_slave_lock);
> -	read_unlock(&bond->lock);
> -
>  	if (swap_slave) {
>  		alb_swap_mac_addr(bond, swap_slave, bond->curr_active_slave);
>  		alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave);
> @@ -1757,16 +1752,17 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
>  		alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr,
>  				       bond->alb_info.rlb_enabled);
>  
> -		alb_send_learning_packets(bond->curr_active_slave, bond_dev->dev_addr);
> +		read_lock(&bond->lock);
> +		alb_send_learning_packets(bond->curr_active_slave,
> +					  bond_dev->dev_addr);
>  		if (bond->alb_info.rlb_enabled) {
>  			/* inform clients mac address has changed */
> -			rlb_req_update_slave_clients(bond, bond->curr_active_slave);
> +			rlb_req_update_slave_clients(bond,
> +						     bond->curr_active_slave);
>  		}
> +		read_unlock(&bond->lock);
>  	}
>  
> -	read_lock(&bond->lock);
> -	write_lock_bh(&bond->curr_slave_lock);
> -
>  	return 0;
>  }
>
Jay Vosburgh - April 15, 2009, 6:11 p.m.
Paul Smith <paul@mad-scientist.net> wrote:

>On Tue, 2009-04-14 at 18:12 -0700, Jay Vosburgh wrote:
>> 	I think I know what's going on.  I believe this patch will
>> resolve things, but I won't be able to test it until tomorrow.  If you
>> want to test this, great; if you want to wait, that's fine too.
>
>Hi Jay; as I mentioned last night this patch is working fine for me so
>far.

	Thanks for the test report.

>However, looking at the rest of this function it seems to me that there
>are other locking issues, at least based on the documentation in the
>header file:
>
> * Here are the locking policies for the two bonding locks:
> *
> * 1) Get bond->lock when reading/writing slave list.
> * 2) Get bond->curr_slave_lock when reading/writing bond->curr_active_slave.
> *    (It is unnecessary when the write-lock is put with bond->lock.)
> * 3) When we lock with bond->curr_slave_lock, we must lock with bond->lock
> *    beforehand.
>
>For example, don't you need to hold bond->curr_slave_lock at least
>around the "if (!bond->curr_active_slave)"?  What about around the
>"bond_for_each_slave" loop?
>
>Many of the other functions, later, also seem to work with
>bond->curr_active_slave and they don't take this lock.
>
>Unless I'm missing something, I think there are still more problems in
>the locking in bond_alb_set_mac_address().

	The various MAC manipulating functions are either called under
RTNL (as bond_alb_set_mac_address is) or take pains to acquire RTNL
before doing anything with the MAC.  Also, the slave list and
curr_active_slave are mutexed by RTNL, so those inspections should be
safe.

	I'm reasonably sure that the curr_slave_lock is superfluous
(which wasn't the case when it was originally introduced), but I haven't
had a chance to validate this.  The locking has changed from what's
documented in the header file; RTNL wasn't used for this when that was
written.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
--
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
Paul Smith - April 15, 2009, 6:39 p.m.
On Wed, 2009-04-15 at 11:11 -0700, Jay Vosburgh wrote:
>         The various MAC manipulating functions are either called under
> RTNL (as bond_alb_set_mac_address is) or take pains to acquire RTNL
> before doing anything with the MAC.  Also, the slave list and
> curr_active_slave are mutexed by RTNL, so those inspections should be
> safe.
> 
>         I'm reasonably sure that the curr_slave_lock is superfluous
> (which wasn't the case when it was originally introduced), but I
> haven't had a chance to validate this.  The locking has changed from
> what's documented in the header file; RTNL wasn't used for this when
> that was written.

OK, sounds good.  I'll let you know if I observe any other odd behavior
with the bonding driver.

Thanks for the great support!  Cheers!

Patch

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 8dc6fbb..b22467a 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1708,10 +1708,8 @@  void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
  * Called with RTNL
  */
 int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
-	__releases(&bond->curr_slave_lock)
-	__releases(&bond->lock)
 	__acquires(&bond->lock)
-	__acquires(&bond->curr_slave_lock)
+	__releases(&bond->lock)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct sockaddr *sa = addr;
@@ -1747,9 +1745,6 @@  int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 		}
 	}
 
-	write_unlock_bh(&bond->curr_slave_lock);
-	read_unlock(&bond->lock);
-
 	if (swap_slave) {
 		alb_swap_mac_addr(bond, swap_slave, bond->curr_active_slave);
 		alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave);
@@ -1757,16 +1752,17 @@  int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 		alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr,
 				       bond->alb_info.rlb_enabled);
 
-		alb_send_learning_packets(bond->curr_active_slave, bond_dev->dev_addr);
+		read_lock(&bond->lock);
+		alb_send_learning_packets(bond->curr_active_slave,
+					  bond_dev->dev_addr);
 		if (bond->alb_info.rlb_enabled) {
 			/* inform clients mac address has changed */
-			rlb_req_update_slave_clients(bond, bond->curr_active_slave);
+			rlb_req_update_slave_clients(bond,
+						     bond->curr_active_slave);
 		}
+		read_unlock(&bond->lock);
 	}
 
-	read_lock(&bond->lock);
-	write_lock_bh(&bond->curr_slave_lock);
-
 	return 0;
 }