[PATCH] bonding: only send arp monitor packets if no other traffic

[PATCH] bonding: only send arp monitor packets if no other traffic

[Chris Friesen]

In order to minimize network traffic, when using load balancing modes
only send out arp monitor packets if it's been more than delta_in_ticks
jiffies since we either received or transmitted packets.  The rationale
behind this is that if there is a lot of other traffic going on we don't
need the arp monitor packets to determine whether or not the link is
working.

This makes the most difference if you have a lot of hosts all arping
the same target at high frequency.

Signed-off-by: Chris Friesen <chris.friesen@genand.com>
---
 drivers/net/bonding/bond_main.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bc13b3d..4c8459a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2885,8 +2885,12 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
 		 * do - all replies will be rx'ed on same link causing slaves
 		 * to be unstable during low/no traffic periods
 		 */
-		if (IS_UP(slave->dev))
-			bond_arp_send_all(bond, slave);
+		if (IS_UP(slave->dev)) {
+			if (time_after_eq(jiffies, dev_trans_start(slave->dev) + delta_in_ticks) ||
+			    time_after_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) {
+				bond_arp_send_all(bond, slave);
+			}
+		}
 	}
 
 	if (do_failover) {

-- 

Chris Friesen
Software Designer
3500 Carling Avenue
Ottawa, Ontario K2H 8E9
www.genband.com

Re: [PATCH] bonding: only send arp monitor packets if no other traffic

[Jay Vosburgh]

Chris Friesen <chris.friesen@genband.com> wrote:

>In order to minimize network traffic, when using load balancing modes
>only send out arp monitor packets if it's been more than delta_in_ticks
>jiffies since we either received or transmitted packets.  The rationale
>behind this is that if there is a lot of other traffic going on we don't
>need the arp monitor packets to determine whether or not the link is
>working.
>
>This makes the most difference if you have a lot of hosts all arping
>the same target at high frequency.

	This logic would not work for the active-backup case (it would
break arp_validate, for one thing), but might be ok for the loadbalance
(balance-xor, balance-rr) case.

	This might adversely affect cases where the switch ports are not
configured into a port channel; in that case, the ARP broadcasts would
be sent to all slaves, but with this patch, will no longer be.  That's
technically not a correct configuration, but seems to be in use
nevertheless.

	I didn't think that the ARP monitor was particularly popular for
the loadbalance case, since it is not as reliable.  It depends upon the
switch to insure that some traffic comes in to each slave, and low
traffic periods can result in false detection of link failures.  Even
with the ARPs being sent out, if those are not evenly balanced by the
switch, false failure detections can occur.

	-J


>Signed-off-by: Chris Friesen <chris.friesen@genand.com>
>---
> drivers/net/bonding/bond_main.c |    8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index bc13b3d..4c8459a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2885,8 +2885,12 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
> 		 * do - all replies will be rx'ed on same link causing slaves
> 		 * to be unstable during low/no traffic periods
> 		 */
>-		if (IS_UP(slave->dev))
>-			bond_arp_send_all(bond, slave);
>+		if (IS_UP(slave->dev)) {
>+			if (time_after_eq(jiffies, dev_trans_start(slave->dev) + delta_in_ticks) ||
>+			    time_after_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) {
>+				bond_arp_send_all(bond, slave);
>+			}
>+		}
> 	}
>
> 	if (do_failover) {
>
>-- 
>
>Chris Friesen
>Software Designer
>3500 Carling Avenue
>Ottawa, Ontario K2H 8E9
>www.genband.com

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

Re: [PATCH] bonding: only send arp monitor packets if no other traffic

[David Miller]

From: Chris Friesen <chris.friesen@genband.com>
Date: Wed, 16 May 2012 12:48:28 -0600

> +		if (IS_UP(slave->dev)) {
> +			if (time_after_eq(jiffies, dev_trans_start(slave->dev) + delta_in_ticks) ||
> +			    time_after_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) {
> +				bond_arp_send_all(bond, slave);
> +			}

These lines are very long, and instead of just reformatting them I'd suggest
that you simply make the test a static helper function.

Also, you should not use braces to enclose a single line of code.

Thanks.