[PATCH net] bonding: send igmp report for its master

[PATCH net] bonding: send igmp report for its master

[Weiping Pan]

Liang Zheng(lzheng@redhat.com) found that in the following topo,
bonding can not send igmp report.

eth0--
      |-- bond0 -- br0
eth1--

modprobe bonding mode=1 miimon=100 resend_igmp=10
ifconfig bond0 up
ifenslave bond0 eth0 eth1

brctl addbr br0
ifconfig br0 192.168.100.2/24 up
brctl addif br0 bond0

Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
then trigger a fali-over in bonding.
You can see that parameter "resend_igmp" does not work.

The reason is that when we add br0 into a multicast group,
it does not propagate multicast knowledge down to its ports.

If we choose to propagate multicast knowledge down to all ports for bridge,
then we have to track every change that is done to bridge, and keep a backup
for all ports. It is hard to track, I think.

Instead I choose to modify bonding to send igmp report for its master.

There is a problem in this patch, in the following topo,
bonding will send duplicate igmp reports.

bond0--
      |-- bond2
bond1--

The reason is that I do not check whether master_dev is a bridge or a bonding.
I prefer to skip such check, since this topo is rare.
And if we add such check, then for every kind of virtual device,
we should check whether bonding can be a slave of it,
and if we add a new virtual device, we have to add a check in bonding, too.

Signed-off-by: Weiping Pan <panweiping3@gmail.com>
---
 drivers/net/bonding/bond_main.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 435984a..300c490 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -794,6 +794,14 @@ static void bond_resend_igmp_join_requests_delayed(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
 					    mcast_work.work);
+	struct net_device *bond_dev, *master_dev = NULL;
+	bond_dev = bond->dev;
+	master_dev = bond_dev->master;
+
+	if (unlikely(master_dev)) {
+		__bond_resend_igmp_join_requests(master_dev);
+	}
+
 	bond_resend_igmp_join_requests(bond);
 }
 
-- 
1.7.4.4

Re: [PATCH net V2] bonding: send igmp report for its master

[Cong Wang]

On 03/14/2012 11:37 AM, Weiping Pan wrote:
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 435984a..300c490 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -794,6 +794,14 @@ static void bond_resend_igmp_join_requests_delayed(struct work_struct *work)
>   {
>   	struct bonding *bond = container_of(work, struct bonding,
>   					    mcast_work.work);
> +	struct net_device *bond_dev, *master_dev = NULL;
> +	bond_dev = bond->dev;

IIRC, you need to hold read_lock for this...

Re: [PATCH net V2] bonding: send igmp report for its master

[Andy Gospodarek]

On Wed, Mar 14, 2012 at 11:37:22AM +0800, Weiping Pan wrote:
> Liang Zheng(lzheng@redhat.com) found that in the following topo,
> bonding does not send igmp report when we trigger a fail-over of bonding.
> 
> eth0--
>       |-- bond0 -- br0
> eth1--
> 
> modprobe bonding mode=1 miimon=100 resend_igmp=10
> ifconfig bond0 up
> ifenslave bond0 eth0 eth1
> 
> brctl addbr br0
> ifconfig br0 192.168.100.2/24 up
> brctl addif br0 bond0
> 
> Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
> then trigger a fali-over in bonding.
> You can see that parameter "resend_igmp" does not work.
> 
> The reason is that when we add br0 into a multicast group,
> it does not propagate multicast knowledge down to its ports.
> 
> If we choose to propagate multicast knowledge down to all ports for bridge,
> then we have to track every change that is done to bridge, and keep a backup
> for all ports. It is hard to track, I think.
> 
> Instead I choose to modify bonding to send igmp report for its master.
> 
> Changelog:
> V2: correct comments
> 
> Signed-off-by: Weiping Pan <panweiping3@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 435984a..300c490 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -794,6 +794,14 @@ static void bond_resend_igmp_join_requests_delayed(struct work_struct *work)
>  {
>  	struct bonding *bond = container_of(work, struct bonding,
>  					    mcast_work.work);
> +	struct net_device *bond_dev, *master_dev = NULL;
> +	bond_dev = bond->dev;
> +	master_dev = bond_dev->master;
> +
> +	if (unlikely(master_dev)) {
> +		__bond_resend_igmp_join_requests(master_dev);
> +	}
> +
>  	bond_resend_igmp_join_requests(bond);
>  }
>  

This check and call to __bond_resend_igmp_join_requests  would be better
in bond_resend_igmp_join_requests for 2 reasons:

1.  bond_resend_igmp_join_requests already has a check for vlan devices.
It seems logical to check for the bridging case there also.

2.  Calling __bond_resend_igmp_join_requests will not work as expected
when igmp_retrans>1 since no decrementing or rescheduling is done in
__bond_resend_igmp_join_requests.

[PATCH net V3] bonding: send igmp report for its master

[Weiping Pan]

Liang Zheng(lzheng@redhat.com) found that in the following topo,
bonding does not send igmp report when we trigger a fail-over of bonding.

eth0--
      |-- bond0 -- br0
eth1--

modprobe bonding mode=1 miimon=100 resend_igmp=10
ifconfig bond0 up
ifenslave bond0 eth0 eth1

brctl addbr br0
ifconfig br0 192.168.100.2/24 up
brctl addif br0 bond0

Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
then trigger a fali-over in bonding.
You can see that parameter "resend_igmp" does not work.

The reason is that when we add br0 into a multicast group,
it does not propagate multicast knowledge down to its ports.

If we choose to propagate multicast knowledge down to all ports for bridge,
then we have to track every change that is done to bridge, and keep a backup
for all ports. It is hard to track, I think.

Instead I choose to modify bonding to send igmp report for its master.

Changelog:
V2: correct comments
V3: move this check into bond_resend_igmp_join_requests()

Signed-off-by: Weiping Pan <panweiping3@gmail.com>
---
 drivers/net/bonding/bond_main.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 435984a..037fdd3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -766,18 +766,26 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
  */
 static void bond_resend_igmp_join_requests(struct bonding *bond)
 {
-	struct net_device *vlan_dev;
+	struct net_device *bond_dev, *vlan_dev, *master_dev;
 	struct vlan_entry *vlan;
 
 	read_lock(&bond->lock);
 
+	bond_dev = bond->dev;
+
 	/* rejoin all groups on bond device */
-	__bond_resend_igmp_join_requests(bond->dev);
+	__bond_resend_igmp_join_requests(bond_dev);
+
+	/* rejoin all groups on its master */
+	master_dev = bond_dev->master;
+	if (unlikely(master_dev)) {
+		__bond_resend_igmp_join_requests(master_dev);
+	}
 
 	/* rejoin all groups on vlan devices */
 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		rcu_read_lock();
-		vlan_dev = __vlan_find_dev_deep(bond->dev,
+		vlan_dev = __vlan_find_dev_deep(bond_dev,
 						vlan->vlan_id);
 		rcu_read_unlock();
 		if (vlan_dev)
-- 
1.7.4.4

Re: [PATCH net V3] bonding: send igmp report for its master

[Jay Vosburgh]

Weiping Pan <panweiping3@gmail.com> wrote:

>Liang Zheng(lzheng@redhat.com) found that in the following topo,
>bonding does not send igmp report when we trigger a fail-over of bonding.
>
>eth0--
>      |-- bond0 -- br0
>eth1--
>
>modprobe bonding mode=1 miimon=100 resend_igmp=10
>ifconfig bond0 up
>ifenslave bond0 eth0 eth1
>
>brctl addbr br0
>ifconfig br0 192.168.100.2/24 up
>brctl addif br0 bond0
>
>Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
>then trigger a fali-over in bonding.
>You can see that parameter "resend_igmp" does not work.
>
>The reason is that when we add br0 into a multicast group,
>it does not propagate multicast knowledge down to its ports.
>
>If we choose to propagate multicast knowledge down to all ports for bridge,
>then we have to track every change that is done to bridge, and keep a backup
>for all ports. It is hard to track, I think.
>
>Instead I choose to modify bonding to send igmp report for its master.
>
>Changelog:
>V2: correct comments
>V3: move this check into bond_resend_igmp_join_requests()
>
>Signed-off-by: Weiping Pan <panweiping3@gmail.com>
>---
> drivers/net/bonding/bond_main.c |   14 +++++++++++---
> 1 files changed, 11 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 435984a..037fdd3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -766,18 +766,26 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
>  */
> static void bond_resend_igmp_join_requests(struct bonding *bond)
> {
>-	struct net_device *vlan_dev;
>+	struct net_device *bond_dev, *vlan_dev, *master_dev;
> 	struct vlan_entry *vlan;
>
> 	read_lock(&bond->lock);
>
>+	bond_dev = bond->dev;
>+
> 	/* rejoin all groups on bond device */
>-	__bond_resend_igmp_join_requests(bond->dev);
>+	__bond_resend_igmp_join_requests(bond_dev);
>+
>+	/* rejoin all groups on its master */
>+	master_dev = bond_dev->master;
>+	if (unlikely(master_dev)) {
>+		__bond_resend_igmp_join_requests(master_dev);
>+	}

	Will this do the right thing if the master is not a bridge?
Granted, right now the only other possible master is a team (since
bonding will not enslave itself), but is this generically safe and
desirable for any possible master_dev?

	-J

> 	/* rejoin all groups on vlan devices */
> 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
> 		rcu_read_lock();
>-		vlan_dev = __vlan_find_dev_deep(bond->dev,
>+		vlan_dev = __vlan_find_dev_deep(bond_dev,
> 						vlan->vlan_id);
> 		rcu_read_unlock();
> 		if (vlan_dev)
>-- 
>1.7.4.4

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net V3] bonding: send igmp report for its master

[Andy Gospodarek]

On Thu, Mar 15, 2012 at 08:43:19PM -0700, Jay Vosburgh wrote:
> Weiping Pan <panweiping3@gmail.com> wrote:
> 
> >Liang Zheng(lzheng@redhat.com) found that in the following topo,
> >bonding does not send igmp report when we trigger a fail-over of bonding.
> >
> >eth0--
> >      |-- bond0 -- br0
> >eth1--
> >
> >modprobe bonding mode=1 miimon=100 resend_igmp=10
> >ifconfig bond0 up
> >ifenslave bond0 eth0 eth1
> >
> >brctl addbr br0
> >ifconfig br0 192.168.100.2/24 up
> >brctl addif br0 bond0
> >
> >Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
> >then trigger a fali-over in bonding.
> >You can see that parameter "resend_igmp" does not work.
> >
> >The reason is that when we add br0 into a multicast group,
> >it does not propagate multicast knowledge down to its ports.
> >
> >If we choose to propagate multicast knowledge down to all ports for bridge,
> >then we have to track every change that is done to bridge, and keep a backup
> >for all ports. It is hard to track, I think.
> >
> >Instead I choose to modify bonding to send igmp report for its master.
> >
> >Changelog:
> >V2: correct comments
> >V3: move this check into bond_resend_igmp_join_requests()
> >
> >Signed-off-by: Weiping Pan <panweiping3@gmail.com>
> >---
> > drivers/net/bonding/bond_main.c |   14 +++++++++++---
> > 1 files changed, 11 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 435984a..037fdd3 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -766,18 +766,26 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
> >  */
> > static void bond_resend_igmp_join_requests(struct bonding *bond)
> > {
> >-	struct net_device *vlan_dev;
> >+	struct net_device *bond_dev, *vlan_dev, *master_dev;
> > 	struct vlan_entry *vlan;
> >
> > 	read_lock(&bond->lock);
> >
> >+	bond_dev = bond->dev;
> >+
> > 	/* rejoin all groups on bond device */
> >-	__bond_resend_igmp_join_requests(bond->dev);
> >+	__bond_resend_igmp_join_requests(bond_dev);
> >+
> >+	/* rejoin all groups on its master */
> >+	master_dev = bond_dev->master;
> >+	if (unlikely(master_dev)) {
> >+		__bond_resend_igmp_join_requests(master_dev);
> >+	}
> 
> 	Will this do the right thing if the master is not a bridge?
> Granted, right now the only other possible master is a team (since
> bonding will not enslave itself), but is this generically safe and
> desirable for any possible master_dev?
> 

I agree with Jay.  You should also check the private flags to see if
IFF_BRIDGE_PORT is set.

Re: [PATCH net V2] bonding: send igmp report for its master

[David Miller]

From: Weiping Pan <panweiping3@gmail.com>
Date: Wed, 14 Mar 2012 11:37:22 +0800

> +	if (unlikely(master_dev)) {
> +		__bond_resend_igmp_join_requests(master_dev);
> +	}

Please do not enclose a single-statement basic block inside of
braces.

[PATCH net V4] bonding: send igmp report for its master

[Weiping Pan]

Liang Zheng(lzheng@redhat.com) found that in the following topo,
bonding does not send igmp report when we trigger a fail-over of bonding.

eth0--
      |-- bond0 -- br0
eth1--

modprobe bonding mode=1 miimon=100 resend_igmp=10
ifconfig bond0 up
ifenslave bond0 eth0 eth1

brctl addbr br0
ifconfig br0 192.168.100.2/24 up
brctl addif br0 bond0

Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
then trigger a fali-over in bonding.
You can see that parameter "resend_igmp" does not work.

The reason is that when we add br0 into a multicast group,
it does not propagate multicast knowledge down to its ports.

If we choose to propagate multicast knowledge down to all ports for bridge,
then we have to track every change that is done to bridge, and keep a backup
for all ports. It is hard to track, I think.

Instead I choose to modify bonding to send igmp report for its master.

Changelog:
V2: correct comments
V3: move this check into bond_resend_igmp_join_requests()
V4: only send igmp reports if bond is enslaved to a bridge

Signed-off-by: Weiping Pan <panweiping3@gmail.com>
---
 drivers/net/bonding/bond_main.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 435984a..4a70e01 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -766,18 +766,30 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
  */
 static void bond_resend_igmp_join_requests(struct bonding *bond)
 {
-	struct net_device *vlan_dev;
+	struct net_device *bond_dev, *vlan_dev, *master_dev;
 	struct vlan_entry *vlan;
 
 	read_lock(&bond->lock);
 
+	bond_dev = bond->dev;
+
 	/* rejoin all groups on bond device */
-	__bond_resend_igmp_join_requests(bond->dev);
+	__bond_resend_igmp_join_requests(bond_dev);
+
+	/* 
+	 * if bond is enslaved to a bridge,
+	 * then rejoin all groups on its master
+	 */
+	master_dev = bond_dev->master;
+	if (master_dev)
+		if ((master_dev->priv_flags & IFF_EBRIDGE)
+			&& (bond_dev->priv_flags & IFF_BRIDGE_PORT))
+			__bond_resend_igmp_join_requests(master_dev);
 
 	/* rejoin all groups on vlan devices */
 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		rcu_read_lock();
-		vlan_dev = __vlan_find_dev_deep(bond->dev,
+		vlan_dev = __vlan_find_dev_deep(bond_dev,
 						vlan->vlan_id);
 		rcu_read_unlock();
 		if (vlan_dev)
-- 
1.7.4.4

Re: [PATCH net V4] bonding: send igmp report for its master

[David Miller]

From: Weiping Pan <panweiping3@gmail.com>
Date: Sun, 18 Mar 2012 11:23:27 +0800

> Liang Zheng(lzheng@redhat.com) found that in the following topo,
> bonding does not send igmp report when we trigger a fail-over of bonding.
> 
> eth0--
>       |-- bond0 -- br0
> eth1--
> 
> modprobe bonding mode=1 miimon=100 resend_igmp=10
> ifconfig bond0 up
> ifenslave bond0 eth0 eth1
> 
> brctl addbr br0
> ifconfig br0 192.168.100.2/24 up
> brctl addif br0 bond0
> 
> Add 192.168.100.2(br0) into a multicast group, like 224.10.10.10,
> then trigger a fali-over in bonding.
> You can see that parameter "resend_igmp" does not work.
> 
> The reason is that when we add br0 into a multicast group,
> it does not propagate multicast knowledge down to its ports.
> 
> If we choose to propagate multicast knowledge down to all ports for bridge,
> then we have to track every change that is done to bridge, and keep a backup
> for all ports. It is hard to track, I think.
> 
> Instead I choose to modify bonding to send igmp report for its master.
> 
> Changelog:
> V2: correct comments
> V3: move this check into bond_resend_igmp_join_requests()
> V4: only send igmp reports if bond is enslaved to a bridge
> 
> Signed-off-by: Weiping Pan <panweiping3@gmail.com>

Applied.