[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Andy Gospodarek]

Round-robin (mode 0) does nothing to ensure that any multicast traffic
originally destined for the host will continue to arrive at the host when
the link that sent the IGMP join or membership report goes down.  One of
the benefits of absolute round-robin transmit.

Keeping track of subscribed multicast groups for each slave did not seem
like a good use of resources, so I decided to simply send on the
curr_active slave of the bond (typically the first enslaved device that
is up).  This makes failover management simple as IGMP membership
reports only need to be sent when the curr_active_slave changes.  I
tested this patch and it appears to work as expected.

Originally reported by Lon Hohberger <lhh@redhat.com>.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
CC: Lon Hohberger <lhh@redhat.com>
CC: Jay Vosburgh <fubar@us.ibm.com>

---
 drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 430c022..0b38455 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 			write_lock_bh(&bond->curr_slave_lock);
 		}
 	}
+
+	/* resend IGMP joins since all were sent on curr_active_slave */
+	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
+		bond_resend_igmp_join_requests(bond);
+	}
 }
 
 /**
@@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave, *start_at;
 	int i, slave_no, res = 1;
+	struct iphdr *iph = ip_hdr(skb);
 
 	read_lock(&bond->lock);
 
 	if (!BOND_IS_OK(bond))
 		goto out;
-
 	/*
-	 * Concurrent TX may collide on rr_tx_counter; we accept that
-	 * as being rare enough not to justify using an atomic op here
+	 * Start with the curr_active_slave that joined the bond as the
+	 * default for sending IGMP traffic.  For failover purposes one
+	 * needs to maintain some consistency for the interface that will
+	 * send the join/membership reports.  The curr_active_slave found
+	 * will send all of this type of traffic.
 	 */
-	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
+	if ((skb->protocol == htons(ETH_P_IP)) &&
+	    (iph->protocol == htons(IPPROTO_IGMP))) {
+		slave = bond->curr_active_slave;
+	} else {
+		/*
+		 * Concurrent TX may collide on rr_tx_counter; we accept
+		 * that as being rare enough not to justify using an
+		 * atomic op here.
+		 */
+		slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
 
-	bond_for_each_slave(bond, slave, i) {
-		slave_no--;
-		if (slave_no < 0)
-			break;
+		bond_for_each_slave(bond, slave, i) {
+			slave_no--;
+			if (slave_no < 0)
+				break;
+		}
 	}
 
 	start_at = slave;

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Jay Vosburgh]

Andy Gospodarek <andy@greyhouse.net> wrote:

>Round-robin (mode 0) does nothing to ensure that any multicast traffic
>originally destined for the host will continue to arrive at the host when
>the link that sent the IGMP join or membership report goes down.  One of
>the benefits of absolute round-robin transmit.
>
>Keeping track of subscribed multicast groups for each slave did not seem
>like a good use of resources, so I decided to simply send on the
>curr_active slave of the bond (typically the first enslaved device that
>is up).  This makes failover management simple as IGMP membership
>reports only need to be sent when the curr_active_slave changes.  I
>tested this patch and it appears to work as expected.
>
>Originally reported by Lon Hohberger <lhh@redhat.com>.
>
>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

	Seems reasonable, modulo a couple of minor things (see below).

	I checked, and the link failover logic appears to maintain
curr_active_slave even for round robin mode, which, prior to this patch,
didn't use it.

>CC: Lon Hohberger <lhh@redhat.com>
>CC: Jay Vosburgh <fubar@us.ibm.com>
>
>---
> drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++--------
> 1 files changed, 26 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 430c022..0b38455 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> 			write_lock_bh(&bond->curr_slave_lock);
> 		}
> 	}
>+
>+	/* resend IGMP joins since all were sent on curr_active_slave */
>+	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
>+		bond_resend_igmp_join_requests(bond);
>+	}
> }
>
> /**
>@@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct slave *slave, *start_at;
> 	int i, slave_no, res = 1;
>+	struct iphdr *iph = ip_hdr(skb);
>
> 	read_lock(&bond->lock);
>
> 	if (!BOND_IS_OK(bond))
> 		goto out;
>-
> 	/*
>-	 * Concurrent TX may collide on rr_tx_counter; we accept that
>-	 * as being rare enough not to justify using an atomic op here
>+	 * Start with the curr_active_slave that joined the bond as the
>+	 * default for sending IGMP traffic.  For failover purposes one
>+	 * needs to maintain some consistency for the interface that will
>+	 * send the join/membership reports.  The curr_active_slave found
>+	 * will send all of this type of traffic.
> 	 */
>-	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+	if ((skb->protocol == htons(ETH_P_IP)) &&
>+	    (iph->protocol == htons(IPPROTO_IGMP))) {
>+		slave = bond->curr_active_slave;

	Technically, this should acquire bond->curr_slave_lock for read
around the inspection of curr_active_slave.

	I believe you'll also want a test for curr_active_slave == NULL,
and free the skb if so (or do something else).  There's a race window in
bond_release: when releasing the curr_active_slave, the field is left
momentarily NULL with the bond unlocked.  This occurs after the
bond_change_active_slave(bond, NULL) call, during the lock dance prior
to the call bond_select_active_slave:

bond_main.c:bond_release():
[...]
	if (oldcurrent == slave)
		bond_change_active_slave(bond, NULL);
[...]
	if (oldcurrent == slave) {
		/*
		 * Note that we hold RTNL over this sequence, so there
		 * is no concern that another slave add/remove event
		 * will interfere.
		 */
		write_unlock_bh(&bond->lock);

		[ race window is here ]

		read_lock(&bond->lock);
		write_lock_bh(&bond->curr_slave_lock);

		bond_select_active_slave(bond);

		write_unlock_bh(&bond->curr_slave_lock);
		read_unlock(&bond->lock);
		write_lock_bh(&bond->lock);
	}

	I'm reasonably sure the other TX functions (that need to) will
handle the case that curr_active_slave is NULL.

>+	} else {
>+		/*
>+		 * Concurrent TX may collide on rr_tx_counter; we accept
>+		 * that as being rare enough not to justify using an
>+		 * atomic op here.
>+		 */
>+		slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>
>-	bond_for_each_slave(bond, slave, i) {
>-		slave_no--;
>-		if (slave_no < 0)
>-			break;
>+		bond_for_each_slave(bond, slave, i) {
>+			slave_no--;
>+			if (slave_no < 0)
>+				break;
>+		}
> 	}
>
> 	start_at = slave;

	-J

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

Re: [net-2.6 PATCH v2] bonding: fix broken multicast with round-robin mode

[Andy Gospodarek]

On Thu, Mar 25, 2010 at 03:31:11PM -0700, Jay Vosburgh wrote:
> Andy Gospodarek <andy@greyhouse.net> wrote:
> 
> >Round-robin (mode 0) does nothing to ensure that any multicast traffic
> >originally destined for the host will continue to arrive at the host when
> >the link that sent the IGMP join or membership report goes down.  One of
> >the benefits of absolute round-robin transmit.
> >
> >Keeping track of subscribed multicast groups for each slave did not seem
> >like a good use of resources, so I decided to simply send on the
> >curr_active slave of the bond (typically the first enslaved device that
> >is up).  This makes failover management simple as IGMP membership
> >reports only need to be sent when the curr_active_slave changes.  I
> >tested this patch and it appears to work as expected.
> >
> >Originally reported by Lon Hohberger <lhh@redhat.com>.
> >
> >Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> 
> 	Seems reasonable, modulo a couple of minor things (see below).
> 
> 	I checked, and the link failover logic appears to maintain
> curr_active_slave even for round robin mode, which, prior to this patch,
> didn't use it.

Correct.  I initially didn't plan to use that, but when I saw that it
was maintained for round-robin mode as well I decided it would be good
to use it rather than wasting space with another pointer.

> 
> >CC: Lon Hohberger <lhh@redhat.com>
> >CC: Jay Vosburgh <fubar@us.ibm.com>
> >
> >---
> > drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++--------
> > 1 files changed, 26 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 430c022..0b38455 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> > 			write_lock_bh(&bond->curr_slave_lock);
> > 		}
> > 	}
> >+
> >+	/* resend IGMP joins since all were sent on curr_active_slave */
> >+	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
> >+		bond_resend_igmp_join_requests(bond);
> >+	}
> > }
> >
> > /**
> >@@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> > 	struct bonding *bond = netdev_priv(bond_dev);
> > 	struct slave *slave, *start_at;
> > 	int i, slave_no, res = 1;
> >+	struct iphdr *iph = ip_hdr(skb);
> >
> > 	read_lock(&bond->lock);
> >
> > 	if (!BOND_IS_OK(bond))
> > 		goto out;
> >-
> > 	/*
> >-	 * Concurrent TX may collide on rr_tx_counter; we accept that
> >-	 * as being rare enough not to justify using an atomic op here
> >+	 * Start with the curr_active_slave that joined the bond as the
> >+	 * default for sending IGMP traffic.  For failover purposes one
> >+	 * needs to maintain some consistency for the interface that will
> >+	 * send the join/membership reports.  The curr_active_slave found
> >+	 * will send all of this type of traffic.
> > 	 */
> >-	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
> >+	if ((skb->protocol == htons(ETH_P_IP)) &&
> >+	    (iph->protocol == htons(IPPROTO_IGMP))) {
> >+		slave = bond->curr_active_slave;
> 
> 	Technically, this should acquire bond->curr_slave_lock for read
> around the inspection of curr_active_slave.
> 
> 	I believe you'll also want a test for curr_active_slave == NULL,
> and free the skb if so (or do something else).  There's a race window in
> bond_release: when releasing the curr_active_slave, the field is left
> momentarily NULL with the bond unlocked.  This occurs after the
> bond_change_active_slave(bond, NULL) call, during the lock dance prior
> to the call bond_select_active_slave:
> 
> bond_main.c:bond_release():
> [...]
> 	if (oldcurrent == slave)
> 		bond_change_active_slave(bond, NULL);
> [...]
> 	if (oldcurrent == slave) {
> 		/*
> 		 * Note that we hold RTNL over this sequence, so there
> 		 * is no concern that another slave add/remove event
> 		 * will interfere.
> 		 */
> 		write_unlock_bh(&bond->lock);
> 
> 		[ race window is here ]
> 
> 		read_lock(&bond->lock);
> 		write_lock_bh(&bond->curr_slave_lock);
> 
> 		bond_select_active_slave(bond);
> 
> 		write_unlock_bh(&bond->curr_slave_lock);
> 		read_unlock(&bond->lock);
> 		write_lock_bh(&bond->lock);
> 	}
> 
> 	I'm reasonably sure the other TX functions (that need to) will
> handle the case that curr_active_slave is NULL.
> 

Good catch, Jay.  Thanks for looking at this.

Here is an updated and tested patch:

[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

Round-robin (mode 0) does nothing to ensure that any multicast traffic
originally destined for the host will continue to arrive at the host when
the link that sent the IGMP join or membership report goes down.  One of
the benefits of absolute round-robin transmit.

Keeping track of subscribed multicast groups for each slave did not seem
like a good use of resources, so I decided to simply send on the
curr_active slave of the bond (typically the first enslaved device that
is up).  This makes failover management simple as IGMP membership
reports only need to be sent when the curr_active_slave changes.  I
tested this patch and it appears to work as expected.

Originally reported by Lon Hohberger <lhh@redhat.com>.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
CC: Lon Hohberger <lhh@redhat.com>
CC: Jay Vosburgh <fubar@us.ibm.com>

---
 drivers/net/bonding/bond_main.c |   40 +++++++++++++++++++++++++++++++-------
 1 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 430c022..5b92fbf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 			write_lock_bh(&bond->curr_slave_lock);
 		}
 	}
+
+	/* resend IGMP joins since all were sent on curr_active_slave */
+	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
+		bond_resend_igmp_join_requests(bond);
+	}
 }
 
 /**
@@ -4138,22 +4143,41 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave, *start_at;
 	int i, slave_no, res = 1;
+	struct iphdr *iph = ip_hdr(skb);
 
 	read_lock(&bond->lock);
 
 	if (!BOND_IS_OK(bond))
 		goto out;
-
 	/*
-	 * Concurrent TX may collide on rr_tx_counter; we accept that
-	 * as being rare enough not to justify using an atomic op here
+	 * Start with the curr_active_slave that joined the bond as the
+	 * default for sending IGMP traffic.  For failover purposes one
+	 * needs to maintain some consistency for the interface that will
+	 * send the join/membership reports.  The curr_active_slave found
+	 * will send all of this type of traffic.
 	 */
-	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
+	if ((iph->protocol == htons(IPPROTO_IGMP)) &&
+	    (skb->protocol == htons(ETH_P_IP))) {
 
-	bond_for_each_slave(bond, slave, i) {
-		slave_no--;
-		if (slave_no < 0)
-			break;
+		read_lock(&bond->curr_slave_lock);
+		slave = bond->curr_active_slave;
+		read_unlock(&bond->curr_slave_lock);
+
+		if (!slave)
+			goto out;
+	} else {
+		/*
+		 * Concurrent TX may collide on rr_tx_counter; we accept
+		 * that as being rare enough not to justify using an
+		 * atomic op here.
+		 */
+		slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
+
+		bond_for_each_slave(bond, slave, i) {
+			slave_no--;
+			if (slave_no < 0)
+				break;
+		}
 	}
 
 	start_at = slave;

Re: [net-2.6 PATCH v2] bonding: fix broken multicast with round-robin mode

[Jay Vosburgh]

Andy Gospodarek <andy@greyhouse.net> wrote:
[...]
>Here is an updated and tested patch:
>
>[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode
>
>Round-robin (mode 0) does nothing to ensure that any multicast traffic
>originally destined for the host will continue to arrive at the host when
>the link that sent the IGMP join or membership report goes down.  One of
>the benefits of absolute round-robin transmit.
>
>Keeping track of subscribed multicast groups for each slave did not seem
>like a good use of resources, so I decided to simply send on the
>curr_active slave of the bond (typically the first enslaved device that
>is up).  This makes failover management simple as IGMP membership
>reports only need to be sent when the curr_active_slave changes.  I
>tested this patch and it appears to work as expected.
>
>Originally reported by Lon Hohberger <lhh@redhat.com>.
>
>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>CC: Lon Hohberger <lhh@redhat.com>
>CC: Jay Vosburgh <fubar@us.ibm.com>

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


>---
> drivers/net/bonding/bond_main.c |   40 +++++++++++++++++++++++++++++++-------
> 1 files changed, 32 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 430c022..5b92fbf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> 			write_lock_bh(&bond->curr_slave_lock);
> 		}
> 	}
>+
>+	/* resend IGMP joins since all were sent on curr_active_slave */
>+	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
>+		bond_resend_igmp_join_requests(bond);
>+	}
> }
>
> /**
>@@ -4138,22 +4143,41 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct slave *slave, *start_at;
> 	int i, slave_no, res = 1;
>+	struct iphdr *iph = ip_hdr(skb);
>
> 	read_lock(&bond->lock);
>
> 	if (!BOND_IS_OK(bond))
> 		goto out;
>-
> 	/*
>-	 * Concurrent TX may collide on rr_tx_counter; we accept that
>-	 * as being rare enough not to justify using an atomic op here
>+	 * Start with the curr_active_slave that joined the bond as the
>+	 * default for sending IGMP traffic.  For failover purposes one
>+	 * needs to maintain some consistency for the interface that will
>+	 * send the join/membership reports.  The curr_active_slave found
>+	 * will send all of this type of traffic.
> 	 */
>-	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+	if ((iph->protocol == htons(IPPROTO_IGMP)) &&
>+	    (skb->protocol == htons(ETH_P_IP))) {
>
>-	bond_for_each_slave(bond, slave, i) {
>-		slave_no--;
>-		if (slave_no < 0)
>-			break;
>+		read_lock(&bond->curr_slave_lock);
>+		slave = bond->curr_active_slave;
>+		read_unlock(&bond->curr_slave_lock);
>+
>+		if (!slave)
>+			goto out;
>+	} else {
>+		/*
>+		 * Concurrent TX may collide on rr_tx_counter; we accept
>+		 * that as being rare enough not to justify using an
>+		 * atomic op here.
>+		 */
>+		slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+
>+		bond_for_each_slave(bond, slave, i) {
>+			slave_no--;
>+			if (slave_no < 0)
>+				break;
>+		}
> 	}
>
> 	start_at = slave;

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Eric Dumazet]

Le jeudi 25 mars 2010 à 17:40 -0400, Andy Gospodarek a écrit :
> Round-robin (mode 0) does nothing to ensure that any multicast traffic
> originally destined for the host will continue to arrive at the host when
> the link that sent the IGMP join or membership report goes down.  One of
> the benefits of absolute round-robin transmit.
> 
> Keeping track of subscribed multicast groups for each slave did not seem
> like a good use of resources, so I decided to simply send on the
> curr_active slave of the bond (typically the first enslaved device that
> is up).  This makes failover management simple as IGMP membership
> reports only need to be sent when the curr_active_slave changes.  I
> tested this patch and it appears to work as expected.
> 
> Originally reported by Lon Hohberger <lhh@redhat.com>.
> 
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> CC: Lon Hohberger <lhh@redhat.com>
> CC: Jay Vosburgh <fubar@us.ibm.com>
> 
> ---
>  drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++--------
>  1 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 430c022..0b38455 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>  			write_lock_bh(&bond->curr_slave_lock);
>  		}
>  	}
> +
> +	/* resend IGMP joins since all were sent on curr_active_slave */
> +	if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
> +		bond_resend_igmp_join_requests(bond);
> +	}
>  }
>  
>  /**
> @@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
>  	struct bonding *bond = netdev_priv(bond_dev);
>  	struct slave *slave, *start_at;
>  	int i, slave_no, res = 1;
> +	struct iphdr *iph = ip_hdr(skb);
>  
>  	read_lock(&bond->lock);
>  
>  	if (!BOND_IS_OK(bond))
>  		goto out;
> -
>  	/*
> -	 * Concurrent TX may collide on rr_tx_counter; we accept that
> -	 * as being rare enough not to justify using an atomic op here
> +	 * Start with the curr_active_slave that joined the bond as the
> +	 * default for sending IGMP traffic.  For failover purposes one
> +	 * needs to maintain some consistency for the interface that will
> +	 * send the join/membership reports.  The curr_active_slave found
> +	 * will send all of this type of traffic.
>  	 */
> -	slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
> +	if ((skb->protocol == htons(ETH_P_IP)) &&
> +	    (iph->protocol == htons(IPPROTO_IGMP))) {

Hmm...

iph->protocol is a u8, how can htons(IPPROTO_IGMP) be equal to
iph->protocol ?

[PATCH] bonding: bond_xmit_roundrobin() fix

Commit a2fd940f (bonding: fix broken multicast with round-robin mode)
added a problem on litle endian machines.

drivers/net/bonding/bond_main.c:4159: warning: comparison is always
false due to limited range of data type

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5b92fbf..5972a52 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4156,7 +4156,7 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 	 * send the join/membership reports.  The curr_active_slave found
 	 * will send all of this type of traffic.
 	 */
-	if ((iph->protocol == htons(IPPROTO_IGMP)) &&
+	if ((iph->protocol == IPPROTO_IGMP) &&
 	    (skb->protocol == htons(ETH_P_IP))) {
 
 		read_lock(&bond->curr_slave_lock);

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Andy Gospodarek]

On Wed, Mar 31, 2010 at 5:08 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 25 mars 2010 à 17:40 -0400, Andy Gospodarek a écrit :
>> Round-robin (mode 0) does nothing to ensure that any multicast traffic
>> originally destined for the host will continue to arrive at the host when
>> the link that sent the IGMP join or membership report goes down.  One of
>> the benefits of absolute round-robin transmit.
>>
>> Keeping track of subscribed multicast groups for each slave did not seem
>> like a good use of resources, so I decided to simply send on the
>> curr_active slave of the bond (typically the first enslaved device that
>> is up).  This makes failover management simple as IGMP membership
>> reports only need to be sent when the curr_active_slave changes.  I
>> tested this patch and it appears to work as expected.
>>
>> Originally reported by Lon Hohberger <lhh@redhat.com>.
>>
>> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>> CC: Lon Hohberger <lhh@redhat.com>
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>>
>> ---
>>  drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++--------
>>  1 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 430c022..0b38455 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>>                       write_lock_bh(&bond->curr_slave_lock);
>>               }
>>       }
>> +
>> +     /* resend IGMP joins since all were sent on curr_active_slave */
>> +     if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
>> +             bond_resend_igmp_join_requests(bond);
>> +     }
>>  }
>>
>>  /**
>> @@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
>>       struct bonding *bond = netdev_priv(bond_dev);
>>       struct slave *slave, *start_at;
>>       int i, slave_no, res = 1;
>> +     struct iphdr *iph = ip_hdr(skb);
>>
>>       read_lock(&bond->lock);
>>
>>       if (!BOND_IS_OK(bond))
>>               goto out;
>> -
>>       /*
>> -      * Concurrent TX may collide on rr_tx_counter; we accept that
>> -      * as being rare enough not to justify using an atomic op here
>> +      * Start with the curr_active_slave that joined the bond as the
>> +      * default for sending IGMP traffic.  For failover purposes one
>> +      * needs to maintain some consistency for the interface that will
>> +      * send the join/membership reports.  The curr_active_slave found
>> +      * will send all of this type of traffic.
>>        */
>> -     slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>> +     if ((skb->protocol == htons(ETH_P_IP)) &&
>> +         (iph->protocol == htons(IPPROTO_IGMP))) {
>
> Hmm...
>
> iph->protocol is a u8, how can htons(IPPROTO_IGMP) be equal to
> iph->protocol ?

Heh, this isn't needed for a single-byte check.  Thanks for catching that.

> [PATCH] bonding: bond_xmit_roundrobin() fix
>
> Commit a2fd940f (bonding: fix broken multicast with round-robin mode)
> added a problem on litle endian machines.
>
> drivers/net/bonding/bond_main.c:4159: warning: comparison is always
> false due to limited range of data type

Curious what version of GCC are you using?  Before applying your patch
it compiles without warning on my x86_64 F11-ish system with:

gcc version 4.4.1 20090725 (Red Hat 4.4.1-2) (GCC)

> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 5b92fbf..5972a52 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4156,7 +4156,7 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
>         * send the join/membership reports.  The curr_active_slave found
>         * will send all of this type of traffic.
>         */
> -       if ((iph->protocol == htons(IPPROTO_IGMP)) &&
> +       if ((iph->protocol == IPPROTO_IGMP) &&
>            (skb->protocol == htons(ETH_P_IP))) {
>
>                read_lock(&bond->curr_slave_lock);
>
>
>
>

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Eric Dumazet]

Le mercredi 31 mars 2010 à 10:49 -0400, Andy Gospodarek a écrit :

> Curious what version of GCC are you using?  Before applying your patch
> it compiles without warning on my x86_64 F11-ish system with:
> 
> gcc version 4.4.1 20090725 (Red Hat 4.4.1-2) (GCC)
> 

ARCH=x86_64 CROSS_COMPILE=x86_64-unknown-linux- make vmlinux

so, it was a cross compiler :

/data/x86-64/bin/x86_64-unknown-linux-gcc-4.1.2 -v
Using built-in specs.
Target: x86_64-unknown-linux
Configured with: ../gcc-4.1.2/configure --prefix=/data/x86-64
--target=x86_64-unknown-linux --enable-languages=c --disable-shared
--disable-multilib --disable-threads --disable-libssp --without-headers
--disable-libmudflap
Thread model: single
gcc version 4.1.2

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 31 Mar 2010 17:14:08 +0200

> ARCH=x86_64 CROSS_COMPILE=x86_64-unknown-linux- make vmlinux
> 
> so, it was a cross compiler :
> 
> /data/x86-64/bin/x86_64-unknown-linux-gcc-4.1.2 -v
> Using built-in specs.
> Target: x86_64-unknown-linux
> Configured with: ../gcc-4.1.2/configure --prefix=/data/x86-64
> --target=x86_64-unknown-linux --enable-languages=c --disable-shared
> --disable-multilib --disable-threads --disable-libssp --without-headers
> --disable-libmudflap
> Thread model: single
> gcc version 4.1.2

Funny how going back in time gives us better diagnostic messages from
the compiler :-)

FWIW I also didn't get the warning, and that was with gcc-4.5 built
from the gcc trunk just the other day.

I suspect this is to do with a change to what warnings get enabled by
default with the -W options we put in the cflags rather than gcc
losing the ability to detect this case.

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[Eric Dumazet]

Le mercredi 31 mars 2010 à 14:00 -0700, David Miller a écrit :

> Funny how going back in time gives us better diagnostic messages from
> the compiler :-)
> 
> FWIW I also didn't get the warning, and that was with gcc-4.5 built
> from the gcc trunk just the other day.
> 
> I suspect this is to do with a change to what warnings get enabled by
> default with the -W options we put in the cflags rather than gcc
> losing the ability to detect this case.

  x86_64-unknown-linux-gcc -Wp,-MD,drivers/net/bonding/.bond_main.o.d
-nostdinc
-isystem /data/x86-64/lib/gcc/x86_64-unknown-linux/4.1.2/include
-I/data/src/linux-2.6/arch/x86/include -Iinclude  -include
include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef
-Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common
-Werror-implicit-function-declaration -Wno-format-security
-fno-delete-null-pointer-checks -O2 -m64 -mno-red-zone -mcmodel=kernel
-funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1
-DCONFIG_AS_CFI_SIGNAL_FRAME=1 -pipe -Wno-sign-compare
-fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow
-fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls
-g -pg -Wdeclaration-after-statement -Wno-pointer-sign
-D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(bond_main)"
-D"KBUILD_MODNAME=KBUILD_STR(bonding)"  -c -o
drivers/net/bonding/.tmp_bond_main.o drivers/net/bonding/bond_main.c
drivers/net/bonding/bond_main.c: In function ‘bond_xmit_roundrobin’:
drivers/net/bonding/bond_main.c:4159: warning: comparison is always
false due to limited range of data type


while with gcc-4.4.2 (native compiler, no warning displayed)

 gcc -Wp,-MD,drivers/net/bonding/.bond_main.o.d  -nostdinc
-isystem /usr/lib/gcc/x86_64-linux-gnu/4.4.3/include
-I/usr/src/git/linux-2.6/arch/x86/include -Iinclude  -include
include/generated/autoconf.h -D__KERNEL__ -Wall -Wundef
-Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common
-Werror-implicit-function-declaration -Wno-format-security
-fno-delete-null-pointer-checks -O2 -m64 -mtune=generic -mno-red-zone
-mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args
-DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -pipe -Wno-sign-compare
-fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow
-Wframe-larger-than=1024 -fno-stack-protector -fno-omit-frame-pointer
-fno-optimize-sibling-calls -g -Wdeclaration-after-statement
-Wno-pointer-sign -fno-strict-overflow -fno-dwarf2-cfi-asm
-fconserve-stack   -D"KBUILD_STR(s)=#s"
-D"KBUILD_BASENAME=KBUILD_STR(bond_main)"
-D"KBUILD_MODNAME=KBUILD_STR(bond_main)"  -c -o
drivers/net/bonding/.tmp_bond_main.o drivers/net/bonding/bond_main.c


New compiler got these new options :  -Wframe-larger-than=1024
-fno-strict-overflow -fno-dwarf2-cfi-asm -fconserve-stack

Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 31 Mar 2010 23:23:06 +0200

> New compiler got these new options :  -Wframe-larger-than=1024
> -fno-strict-overflow -fno-dwarf2-cfi-asm -fconserve-stack

Ok, so much for my warning option theory :-)