diff mbox

bonding: clean muticast addresses when bevice changes type

Message ID 4A5DEE1F.4090707@Voltaire.COM
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Moni Shoua July 15, 2009, 2:56 p.m. UTC 
Bonding device  forbids slave device of different types under the same master.
However, it is possible for a bonding master to change  type during its lifetime. 
This can be either from ARPHRD_ETHER to ARPHRD_INFINIBAND or the other way arround.  
The change of type requires device level multicast address cleanup because device
level multicast addresses depend on the device type. 

The patch adds a call to dev_close() before the bonding master changes type and 
dev_open() just after that.

In the example below I enslaved an IPoIB device (ib0) under bond0. Since
each bonding master starts as device of type ARPHRD_ETHER by default, a change
of type occurs when ib0 is enslaved.

This is how /proc/net/dev_mcast looks like without the patch

5    bond0           1     0     00ffffffff12601bffff000000000001ff96ca05
5    bond0           1     0     01005e000116
5    bond0           1     0     01005e7ffffd
5    bond0           1     0     01005e000001
5    bond0           1     0     333300000001
6    ib0             1     0     00ffffffff12601bffff000000000001ff96ca05
6    ib0             1     0     333300000001
6    ib0             1     0     01005e000001
6    ib0             1     0     01005e7ffffd
6    ib0             1     0     01005e000116
6    ib0             1     0     00ffffffff12401bffff00000000000000000001
6    ib0             1     0     00ffffffff12601bffff00000000000000000001

and this is how it looks like after the patch.

5    bond0           1     0     00ffffffff12601bffff000000000001ff96ca05
5    bond0           1     0     00ffffffff12601bffff00000000000000000001
5    bond0           1     0     00ffffffff12401bffff0000000000000ffffffd
5    bond0           1     0     00ffffffff12401bffff00000000000000000116
5    bond0           1     0     00ffffffff12401bffff00000000000000000001
6    ib0             1     0     00ffffffff12601bffff000000000001ff96ca05
6    ib0             1     0     00ffffffff12401bffff00000000000000000116
6    ib0             1     0     00ffffffff12401bffff0000000000000ffffffd
6    ib0             2     0     00ffffffff12401bffff00000000000000000001
6    ib0             2     0     00ffffffff12601bffff00000000000000000001

Signed-off-by: Moni Shoua <monis@voltaire.com>
--

 drivers/net/bonding/bond_main.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

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

Comments

Moni Shoua July 15, 2009, 3:03 p.m. UTC | #1 
I forgot to mention that the need for this patch was raised here
http://lists.openfabrics.org/pipermail/general/2009-July/060496.html

--
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
Jay Vosburgh July 16, 2009, 8:57 p.m. UTC | #2 
Moni Shoua <monis@voltaire.com> wrote:

>Bonding device  forbids slave device of different types under the same master.
>However, it is possible for a bonding master to change  type during its lifetime. 
>This can be either from ARPHRD_ETHER to ARPHRD_INFINIBAND or the other way arround.  
>The change of type requires device level multicast address cleanup because device
>level multicast addresses depend on the device type. 
>
>The patch adds a call to dev_close() before the bonding master changes type and 
>dev_open() just after that.
>
>In the example below I enslaved an IPoIB device (ib0) under bond0. Since
>each bonding master starts as device of type ARPHRD_ETHER by default, a change
>of type occurs when ib0 is enslaved.
>
>This is how /proc/net/dev_mcast looks like without the patch
>
>5    bond0           1     0     00ffffffff12601bffff000000000001ff96ca05
>5    bond0           1     0     01005e000116
>5    bond0           1     0     01005e7ffffd
>5    bond0           1     0     01005e000001
>5    bond0           1     0     333300000001
>6    ib0             1     0     00ffffffff12601bffff000000000001ff96ca05
>6    ib0             1     0     333300000001
>6    ib0             1     0     01005e000001
>6    ib0             1     0     01005e7ffffd
>6    ib0             1     0     01005e000116
>6    ib0             1     0     00ffffffff12401bffff00000000000000000001
>6    ib0             1     0     00ffffffff12601bffff00000000000000000001
>
>and this is how it looks like after the patch.
>
>5    bond0           1     0     00ffffffff12601bffff000000000001ff96ca05
>5    bond0           1     0     00ffffffff12601bffff00000000000000000001
>5    bond0           1     0     00ffffffff12401bffff0000000000000ffffffd
>5    bond0           1     0     00ffffffff12401bffff00000000000000000116
>5    bond0           1     0     00ffffffff12401bffff00000000000000000001
>6    ib0             1     0     00ffffffff12601bffff000000000001ff96ca05
>6    ib0             1     0     00ffffffff12401bffff00000000000000000116
>6    ib0             1     0     00ffffffff12401bffff0000000000000ffffffd
>6    ib0             2     0     00ffffffff12401bffff00000000000000000001
>6    ib0             2     0     00ffffffff12601bffff00000000000000000001
>
>Signed-off-by: Moni Shoua <monis@voltaire.com>

	I checked the discussion noted in a separate email, and the
rationale seems reasonable.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>


>
> drivers/net/bonding/bond_main.c |   12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index d927f71..aa1be1f 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1459,8 +1459,16 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	 * ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same bond
> 	 */
> 	if (bond->slave_cnt == 0) {
>-		if (slave_dev->type != ARPHRD_ETHER)
>-			bond_setup_by_slave(bond_dev, slave_dev);
>+		if (bond_dev->type != slave_dev->type) {
>+			dev_close(bond_dev);
>+			pr_debug("%s: change device type from %d to %d\n",
>+				bond_dev->name, bond_dev->type, slave_dev->type);
>+			if (slave_dev->type != ARPHRD_ETHER)
>+				bond_setup_by_slave(bond_dev, slave_dev);
>+			else
>+				ether_setup(bond_dev);
>+			dev_open(bond_dev);
>+		}
> 	} else if (bond_dev->type != slave_dev->type) {
> 		pr_err(DRV_NAME ": %s ether type (%d) is different "
> 			"from other slaves (%d), can not enslave it.\n",
--
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 July 17, 2009, 1:09 a.m. UTC | #3 
From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 16 Jul 2009 13:57:59 -0700

>>Signed-off-by: Moni Shoua <monis@voltaire.com>
> 
> 	I checked the discussion noted in a separate email, and the
> rationale seems reasonable.
> 
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Applied.
--
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
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d927f71..aa1be1f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1459,8 +1459,16 @@  int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	 * ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same bond
 	 */
 	if (bond->slave_cnt == 0) {
-		if (slave_dev->type != ARPHRD_ETHER)
-			bond_setup_by_slave(bond_dev, slave_dev);
+		if (bond_dev->type != slave_dev->type) {
+			dev_close(bond_dev);
+			pr_debug("%s: change device type from %d to %d\n",
+				bond_dev->name, bond_dev->type, slave_dev->type);
+			if (slave_dev->type != ARPHRD_ETHER)
+				bond_setup_by_slave(bond_dev, slave_dev);
+			else
+				ether_setup(bond_dev);
+			dev_open(bond_dev);
+		}
 	} else if (bond_dev->type != slave_dev->type) {
 		pr_err(DRV_NAME ": %s ether type (%d) is different "
 			"from other slaves (%d), can not enslave it.\n",