Possible bug in bonding driver (with VLAN in ALB mode)

Possible bug in bonding driver (with VLAN in ALB mode)

[Sven Anders]

[-- Attachment #1.1: Type: text/plain, Size: 3631 bytes --]

Hello!

We possible found an error in the bonding driver, but we are
not sure. Maybe some can help us here and possibly find time
to test it themself.

We have the following problem using Bonding and VLAN with ALB-Mode:

Shortly after initiating "iperf" traffic from each of the client
to each other client, all traffic collapses to one line. This is
expected behaviour of the ALB algorithmus, but we did not expect
this traffic to stay there!

We would expect the traffic to be rebalanced between the bonding
slaves, but it never happens.

We analysed it down to the ARP replies the bonding interface should
receive.
After an ARP request was sent (by the router) in the "rlb_arp_xmit()",
the "rlb_choose_channel()" function is called and reserves a
(placeholder) entry in the hash table. According to the comment this
entry will be updated later by the corresponding ARP reply.
Shortly afterwards we see an ARP reply package on the VLAN interface.
But it is never processed by "rlb_arp_recv()", because this function
only receives arp packages from bond0 and so the entry is never
updated.

Now the question:
 We are not sure, if this is an error in the bonding driver.

 Maybe we are doing something wrong?
 Do we need to set some special /proc/sys/... values or
 have to configure some driver special?

 (And yes we want ALB, not LACP or anything other...!)

Here are some details about our network:
                                            +-----------+
                                       +----+  Client 1 |
 +----------+        +----------+------+    +-----------+     +-----------+
 |          +--------+          +-----------------------------+  Client 2 |
 |  Router  +--------+  Switch  |                             +-----------+
 |          +--------+          |                             +-----------+
 |          +--------+          +-----------------------------+  Client 3 |
 +----------+        +----------+------+    +-----------+     +-----------+
                                       +----+  Client 4 |
                                            +-----------+

Router:

  eth0
  eth1
  eth2
  eth3
  bond0    (Slaves: eth0, eth1, eth2, eth3)
  bond0.11  (VLAN 11) IP: 10.0.1.250/24
  bond0.12  (VLAN 12) IP: 10.0.2.250/24
  bond0.13  (VLAN 13) IP: 10.0.3.250/24
  bond0.14  (VLAN 14) IP: 10.0.4.250/24

  Routing for each subnet is enabled.

Switch (HP ProCurve):
  4 wires to router (one for each physical interface, eth0-3)
        each VLAN tagged (with 11,12,13,14)
  1 wire to each client, untagged VLANs

Client1:
  Network 10.0.1.1/24 GW: 10.0.1.250/24

Client2:
  Network 10.0.2.1/24 GW: 10.0.2.250/24

Client 3:
  ...

Software/Hardware:
 Linux: 2.6.26.2
  Network driver: e1000e (0.4.1.7-NAPI)
 Switch: HP ProCurve 5406zl

Bonding configuration:

 Ethernet Channel Bonding Driver: v3.2.5 (March 21, 2008)
 Bonding Mode: adaptive load balancing
 Primary Slave: None
 Currently Active Slave: eth11
 MII Status: up
 MII Polling Interval (ms): 100
 Up Delay (ms): 100
 Down Delay (ms): 100

Cheers
 Sven Anders

-- 
 Sven Anders <anders@anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker
Vorsitzender des Aufsichtsrats: Mark Peters

[-- Attachment #1.2: anders.vcf --]
[-- Type: text/x-vcard, Size: 352 bytes --]

begin:vcard
fn:Sven Anders
n:Anders;Sven
org:ANDURAS AG;Research and Development
adr;quoted-printable:;;Innstra=C3=9Fe 71;Passau;Bavaria;94036;Germany
email;internet:anders@anduras.de
title:Dipl. Inf.
tel;work:++49 (0)851 / 490 50 -0
tel;fax:++49 (0)851 / 590 50 - 55
x-mozilla-html:FALSE
url:http://www.anduras.de
version:2.1
end:vcard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Possible bug in bonding driver (with VLAN in ALB mode)

[Jay Vosburgh]

Sven Anders <anders@anduras.de> wrote:
[...]
>After an ARP request was sent (by the router) in the "rlb_arp_xmit()",
>the "rlb_choose_channel()" function is called and reserves a
>(placeholder) entry in the hash table. According to the comment this
>entry will be updated later by the corresponding ARP reply.
>Shortly afterwards we see an ARP reply package on the VLAN interface.
>But it is never processed by "rlb_arp_recv()", because this function
>only receives arp packages from bond0 and so the entry is never
>updated.

	Can you try this patch and tell me if it fixes the problem?

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index ade5f3f..500a207 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -815,7 +815,7 @@ static int rlb_initialize(struct bonding *bond)
 
 	/*initialize packet type*/
 	pk_type->type = __constant_htons(ETH_P_ARP);
-	pk_type->dev = bond->dev;
+	pk_type->dev = NULL;
 	pk_type->func = rlb_arp_recv;
 
 	/* register to receive ARPs */


	-J

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

Re: Possible bug in bonding driver (with VLAN in ALB mode)

[Sven Anders]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

Jay Vosburgh schrieb:

> Can you try this patch and tell me if it fixes the problem?
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index ade5f3f..500a207 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -815,7 +815,7 @@ static int rlb_initialize(struct bonding *bond)
>  
>  	/*initialize packet type*/
>  	pk_type->type = __constant_htons(ETH_P_ARP);
> -	pk_type->dev = bond->dev;
> +	pk_type->dev = NULL;
>  	pk_type->func = rlb_arp_recv;
>  
>  	/* register to receive ARPs */

Hmm... I think the idea was good, but the whole system
crashes. To be correct, it simply reboots.

Any other suggestion for a quick test?!

Regards
 Sven

-- 
 Sven Anders <anders@anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker
Vorsitzender des Aufsichtsrats: Mark Peters

[-- Attachment #1.2: anders.vcf --]
[-- Type: text/x-vcard, Size: 352 bytes --]

begin:vcard
fn:Sven Anders
n:Anders;Sven
org:ANDURAS AG;Research and Development
adr;quoted-printable:;;Innstra=C3=9Fe 71;Passau;Bavaria;94036;Germany
email;internet:anders@anduras.de
title:Dipl. Inf.
tel;work:++49 (0)851 / 490 50 -0
tel;fax:++49 (0)851 / 590 50 - 55
x-mozilla-html:FALSE
url:http://www.anduras.de
version:2.1
end:vcard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Possible bug in bonding driver (with VLAN in ALB mode)

[Sven Anders]

[-- Attachment #1.1: Type: text/plain, Size: 3252 bytes --]

Jay Vosburgh schrieb:
> Sven Anders <anders@anduras.de> wrote:
> 
>> Any other ideas?
> 
> 	Try this; I actually tested this one and it seems to not crash
> my system.  This is against the current mainline, so you might have to
> apply it by hand on your older kernel.

Ok, it did not crash, but... (see comments below)

> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 3d39278..4fb78b0 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -341,14 +341,15 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>  
>  static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
>  {
> -	struct bonding *bond = bond_dev->priv;
> +	struct bonding *bond;
>  	struct arp_pkt *arp = (struct arp_pkt *)skb->data;
>  	int res = NET_RX_DROP;
>  
>  	if (dev_net(bond_dev) != &init_net)
>  		goto out;
>  
> -	if (!(bond_dev->flags & IFF_MASTER))
> +	if (!(bond_dev->priv_flags & IFF_BONDING) ||
> +	    !(bond_dev->flags & IFF_MASTER))
>  		goto out;
>  	if (!arp) {

This did not work, because the VLAN interface did not have the
BONDING flag set.

> @@ -363,6 +364,7 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct
>  
>  	if (arp->op_code == htons(ARPOP_REPLY)) {
>  		/* update rx hash table for this ARP */
> +		bond = bond_dev->priv;
>  		rlb_update_entry_from_arp(bond, arp);
>  		dprintk("Server received an ARP Reply from client\n");
>  	}

To make this work, even for the VLAN interfaces, I had to get the
stored "bond" from the "packet_type"'s private data.
Instead I used here:

     bond = ptype->af_packet_priv;
     if (!bond) goto out;

> @@ -813,7 +815,7 @@ static int rlb_initialize(struct bonding *bond)
>  
>  	/*initialize packet type*/
>  	pk_type->type = __constant_htons(ETH_P_ARP);
> -	pk_type->dev = bond->dev;
> +	pk_type->dev = NULL;
>  	pk_type->func = rlb_arp_recv;
>  
>  	/* register to receive ARPs */

Additionally, here I store the "bond" to the private data:

    pk_type->dev = NULL;
    pk_type->func = rlb_arp_recv;
    pk_type->af_packet_priv = bond;

With these fixes, it worked!
That means: Receive load balancing with VLANs works.

Certainly, this is only a test bugfix only, because it does not
work with more than one active bond. Any ideas how to implement
this correctly? Do we have to use an "dev_add_pack()" for each
VLAN we are adding?

What troubles me is, that nobody since the addition of VLAN support
to bonding in 2004 had this problem before. Did this (ever) work in
one older version of the bonding driver?

Regards
 Sven

-- 
 Sven Anders <anders@anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker
Vorsitzender des Aufsichtsrats: Mark Peters

[-- Attachment #1.2: anders.vcf --]
[-- Type: text/x-vcard, Size: 352 bytes --]

begin:vcard
fn:Sven Anders
n:Anders;Sven
org:ANDURAS AG;Research and Development
adr;quoted-printable:;;Innstra=C3=9Fe 71;Passau;Bavaria;94036;Germany
email;internet:anders@anduras.de
title:Dipl. Inf.
tel;work:++49 (0)851 / 490 50 -0
tel;fax:++49 (0)851 / 590 50 - 55
x-mozilla-html:FALSE
url:http://www.anduras.de
version:2.1
end:vcard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Possible bug in bonding driver (with VLAN in ALB mode)

[Jay Vosburgh]

Sven Anders <anders@anduras.de> wrote:
[...]
>Certainly, this is only a test bugfix only, because it does not
>work with more than one active bond. Any ideas how to implement
>this correctly? Do we have to use an "dev_add_pack()" for each
>VLAN we are adding?

	Can you give this a try?  This patch just checks for VLAN-ness
and extracts the real device if so, then does the usual "is it bonding"
stuff to the real device.

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3d39278..ec1c115 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -341,14 +341,18 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
 
 static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
 {
-	struct bonding *bond = bond_dev->priv;
+	struct bonding *bond;
 	struct arp_pkt *arp = (struct arp_pkt *)skb->data;
 	int res = NET_RX_DROP;
 
 	if (dev_net(bond_dev) != &init_net)
 		goto out;
 
-	if (!(bond_dev->flags & IFF_MASTER))
+	if (bond_dev->priv_flags & IFF_802_1Q_VLAN)
+		bond_dev = vlan_dev_real_dev(bond_dev);
+
+	if (!(bond_dev->priv_flags & IFF_BONDING) ||
+	    !(bond_dev->flags & IFF_MASTER))
 		goto out;
 
 	if (!arp) {
@@ -363,6 +367,7 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct
 
 	if (arp->op_code == htons(ARPOP_REPLY)) {
 		/* update rx hash table for this ARP */
+		bond = bond_dev->priv;
 		rlb_update_entry_from_arp(bond, arp);
 		dprintk("Server received an ARP Reply from client\n");
 	}
@@ -813,7 +818,7 @@ static int rlb_initialize(struct bonding *bond)
 
 	/*initialize packet type*/
 	pk_type->type = __constant_htons(ETH_P_ARP);
-	pk_type->dev = bond->dev;
+	pk_type->dev = NULL;
 	pk_type->func = rlb_arp_recv;
 
 	/* register to receive ARPs */


>What troubles me is, that nobody since the addition of VLAN support
>to bonding in 2004 had this problem before. Did this (ever) work in
>one older version of the bonding driver?

	I suspect not; I don't recall checking the load balancing
performance of this kind of configuration (all VLANs over balance-alb).

	-J

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

Re: Possible bug in bonding driver (with VLAN in ALB mode)

[Sven Anders]

[-- Attachment #1.1: Type: text/plain, Size: 2151 bytes --]

Jay Vosburgh schrieb:

> 	Can you give this a try?  This patch just checks for VLAN-ness
> and extracts the real device if so, then does the usual "is it bonding"
> stuff to the real device.
> 
> [...]

Ok, this worked as expected. I think you can submit the patch.

So:  Signed-off-by: Sven Anders <anders@anduras.de>

>> What troubles me is, that nobody since the addition of VLAN support
>> to bonding in 2004 had this problem before. Did this (ever) work in
>> one older version of the bonding driver?
> 
> 	I suspect not; I don't recall checking the load balancing
> performance of this kind of configuration (all VLANs over balance-alb).

Ok.
I wrote a patch to show the current status of the load-balancing in ALB mode,
which helped me a lot identifing the problem.
I attached a version of it to this mail.

Moreover I see two additional problem:

1. If I want to use bonding on 10GBit ethernet cards, the counters
   will most likely overflow, because 10 seconds rebalancing interval
   is too long. But reducing it to 3 or 4 seconds may not be a real
   solution. Maybe we should use 64 Bit counters here?!

2. Is this right, that there is no IPv6 support für RLB?
   I'm not really sure, but we only have the following entries
   in the RLB structure:

	struct rlb_client_info {
	        __be32 ip_src;          /* the server IP address */
        	__be32 ip_dst;          /* the client IP address */
		[...]
	}

   And the function "rlb_update_entry_from_arp()" for updating the info
   by the received arp packages, only supports IPv4 (see struct arp_pkt
   too!)

Any comments welcome...

Cheers
 Sven
-- 
 Sven Anders <anders@anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker
Vorsitzender des Aufsichtsrats: Mark Peters

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: bonding-status-output.patch --]
[-- Type: text/x-diff; name="bonding-status-output.patch", Size: 4389 bytes --]

Signed-off-by: Sven Anders <anders@anduras.de>

diff -ru linux-2.6.28rc2/drivers/net/bonding/bond_alb.c linux-2.6.28rc2-new/drivers/net/bonding/bond_alb.c
--- linux-2.6.28rc2/drivers/net/bonding/bond_alb.c	2008-10-16 13:05:38.000000000 +0200
+++ linux-2.6.28rc2-new/drivers/net/bonding/bond_alb.c	2008-10-27 14:07:38.000000000 +0100
@@ -26,6 +26,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/pkt_sched.h>
+#include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
@@ -1772,3 +1773,88 @@
 	}
 }
 
+void bond_alb_info_show(struct seq_file *seq)
+{
+	struct bonding *bond = seq->private;
+	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
+	struct rlb_client_info *rclient_info;
+	struct tlb_client_info *tclient_info;
+	struct slave *slave;
+	u32 index;
+	int i;
+	DECLARE_MAC_BUF(mac);
+
+	seq_puts(seq, "\nALB info\n");
+	seq_puts(seq, "\n Receive Load Balancing table:\n");
+	seq_puts(seq, "  Index Slave    Assigned Client-MAC"
+		      "         Server -> Client\n");
+
+	_lock_rx_hashtbl(bond);
+
+	index = bond_info->rx_hashtbl_head;
+	for (; index != RLB_NULL_INDEX; index = rclient_info->next) {
+		rclient_info = &(bond_info->rx_hashtbl[index]);
+		if (rclient_info) {
+			seq_printf(seq,	"%6u: %-8s %6s   %-17s  ",
+				   index,
+				   (rclient_info->slave &&
+				   rclient_info->slave->dev &&
+				   rclient_info->slave->dev->name ?
+				   rclient_info->slave->dev->name : "(none)"),
+				   (rclient_info->assigned ? "yes" : "no"),
+				   print_mac(mac, rclient_info->mac_dst));
+
+			/* Implemented as separate outputs to
+			   support IPv6 in the future (if it's supported) */
+			seq_printf(seq,	NIPQUAD_FMT " -> ",
+				   NIPQUAD(rclient_info->ip_src));
+			seq_printf(seq,	NIPQUAD_FMT "\n",
+				   NIPQUAD(rclient_info->ip_dst));
+		}
+	}
+
+	_unlock_rx_hashtbl(bond);
+
+	seq_puts(seq, "\n Transmit Load Balancing table:\n");
+	seq_printf(seq,	"  Unbalanced load: %u\n"
+			"  Rebalance interval: %u seconds\n\n",
+			bond_info->unbalanced_load,
+			BOND_TLB_REBALANCE_INTERVAL);
+
+	_lock_tx_hashtbl(bond);
+
+	/* Process each slave */
+	bond_for_each_slave(bond, slave, i) {
+
+		if (slave) {
+			seq_puts(seq, "  Slave    Used  Speed    Duplex"
+				      "  Current load\n");
+			seq_printf(seq, "  %-8s %3s   %-8u %4s      %10u\n",
+				   (slave->dev->name ?
+				    slave->dev->name : "none"),
+				   (SLAVE_IS_OK(slave) ? "yes" : "no"),
+				   slave->speed,
+				   (slave->duplex ? "full" : "half"),
+				   SLAVE_TLB_INFO(slave).load);
+
+			seq_puts(seq, "           Index    TX Bytes      "
+				      "Load history\n");
+
+			index = SLAVE_TLB_INFO(slave).head;
+			for (; index != TLB_NULL_INDEX;
+			     index = tclient_info->next) {
+				tclient_info = &(bond_info->tx_hashtbl[index]);
+				if (tclient_info)
+					seq_printf(seq,	"            "
+						   "%3u:  %10u"
+						   "        %10u\n",
+						   index,
+						   tclient_info->tx_bytes,
+						   tclient_info->load_history);
+			}
+			seq_puts(seq, "\n");
+		}
+	}
+
+	_unlock_tx_hashtbl(bond);
+}
diff -ru linux-2.6.28rc2/drivers/net/bonding/bond_alb.h linux-2.6.28rc2-new/drivers/net/bonding/bond_alb.h
--- linux-2.6.28rc2/drivers/net/bonding/bond_alb.h	2008-06-09 11:37:33.000000000 +0200
+++ linux-2.6.28rc2-new/drivers/net/bonding/bond_alb.h	2008-10-27 13:56:00.000000000 +0100
@@ -128,5 +128,6 @@
 void bond_alb_monitor(struct work_struct *);
 int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
+void bond_alb_info_show(struct seq_file *seq);
 #endif /* __BOND_ALB_H__ */
 
diff -ru linux-2.6.28rc2/drivers/net/bonding/bond_main.c linux-2.6.28rc2-new/drivers/net/bonding/bond_main.c
--- linux-2.6.28rc2/drivers/net/bonding/bond_main.c	2008-10-25 19:13:44.000000000 +0200
+++ linux-2.6.28rc2-new/drivers/net/bonding/bond_main.c	2008-10-27 14:08:25.000000000 +0100
@@ -3308,7 +3308,8 @@
 			seq_printf(seq, "\tPartner Mac Address: %s\n",
 				   print_mac(mac, ad_info.partner_system));
 		}
-	}
+	} else if (bond->params.mode == BOND_MODE_ALB)
+		bond_alb_info_show(seq);
 }
 
 static void bond_info_show_slave(struct seq_file *seq, const struct slave *slave)

[-- Attachment #1.3: anders.vcf --]
[-- Type: text/x-vcard, Size: 352 bytes --]

begin:vcard
fn:Sven Anders
n:Anders;Sven
org:ANDURAS AG;Research and Development
adr;quoted-printable:;;Innstra=C3=9Fe 71;Passau;Bavaria;94036;Germany
email;internet:anders@anduras.de
title:Dipl. Inf.
tel;work:++49 (0)851 / 490 50 -0
tel;fax:++49 (0)851 / 590 50 - 55
x-mozilla-html:FALSE
url:http://www.anduras.de
version:2.1
end:vcard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]