Skip to contentSkip to conversation

net/bonding: Send more than one gratuitous ARP when slave takes over

PATCH In review Moni Shoua 4 participants 7 messages Apr 10, 2008 – Apr 14, 2008

Subsystems (3)
Networking Network drivers Bonding driver
lore
Share & tools
lore

Moni Shoua

author
#0 lore raw

PATCH 1/1 net/bonding: Send more than one gratuitous ARP when slave takes over

With IPoIB, reception of gratuitous ARP by neighboring hosts
is essential for a successful change of slaves in case of failure. Otherwise, they won't learn about the HW address change and need
to wait a long time until the neighboring system gives up and sends an ARP request to learn the new HW address. This patch decreases the chance for a lost of a gratuitous ARP packet by sending it more than once. The number retries is configurable and can be set with a module param.

Signed-off-by Moni Shoua <[email protected]>
---
 drivers/net/bonding/bond_main.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
drivers/net/bonding/bond_main.c +9 -4
index 0942d82..c066dc0 100644
@@ -88,6 +88,7 @@ #define BOND_LINK_MON_INTERV	0
 #define BOND_LINK_ARP_INTERV	0
 
 static int max_bonds	= BOND_DEFAULT_MAX_BONDS;
+static int num_grat_arp = 1;
 static int miimon	= BOND_LINK_MON_INTERV;
 static int updelay	= 0;
 static int downdelay	= 0;

@@ -104,6 +105,8 @@ struct bond_params bonding_defaults;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
+module_param(num_grat_arp, int, 0644);
+MODULE_PARM_DESC(num_grat_arp, "Number of gratuitous ARP packet to sned on failover");
 module_param(miimon, int, 0);
 MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");
 module_param(updelay, int, 0);

@@ -1109,14 +1112,16 @@ void bond_change_active_slave(struct bon
 		if (new_active && bond->params.fail_over_mac)
 			memcpy(bond->dev->dev_addr,  new_active->dev->dev_addr,
 				new_active->dev->addr_len);
+		bond->send_grat_arp = num_grat_arp;
 		if (bond->curr_active_slave &&
 			test_bit(__LINK_STATE_LINKWATCH_PENDING,
-					&bond->curr_active_slave->dev->state)) {
+					&bond->curr_active_slave->dev->state))
 			dprintk("delaying gratuitous arp on %s\n",
 				bond->curr_active_slave->dev->name);
-			bond->send_grat_arp = 1;
-		} else
+		else {
 			bond_send_gratuitous_arp(bond);
+			bond->send_grat_arp--;
+		}
 	}
 }
 

@@ -2144,7 +2149,7 @@ static int __bond_mii_monitor(struct bon
 			dprintk("sending delayed gratuitous arp on on %s\n",
 				bond->curr_active_slave->dev->name);
 			bond_send_gratuitous_arp(bond);
-			bond->send_grat_arp = 0;
+			bond->send_grat_arp--;
 		}
 	}
 	read_lock(&bond->curr_slave_lock);
#1 lore raw

drivers/net/bonding/bond_main.c struct bond_params bonding_defaults; · line 109

Quoted 6 lines
drivers/net/bonding/bond_main.c +2 -0
@@ -104,6 +105,8 @@ struct bond_params bonding_defaults;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
+module_param(num_grat_arp, int, 0644);
+MODULE_PARM_DESC(num_grat_arp, "Number of gratuitous ARP packet to

sned on failover");

Check your spelling here. s/sned/send/

Also, num_grat_arp is declared int, where send_grat_arp is s8. What happens if you overflow send_grat_arp? Either your module parameter needs to change to match what's in the bonding struct, or you needs some bounds checking to validate your inputs.

Review void bond_change_active_slave(struct bon · line 1113

Quoted 3 lines
(quoted diff) +0 -0
@@ -1109,14 +1112,16 @@ void bond_change_active_slave(struct bon
 		if (new_active && bond->params.fail_over_mac)
 			memcpy(bond->dev->dev_addr,

new_active->dev->dev_addr,

drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon

Quoted 5 lines
drivers/net/bonding/bond_main.c +1 -1
 				new_active->dev->addr_len);
+		bond->send_grat_arp = num_grat_arp;
 		if (bond->curr_active_slave &&
 			test_bit(__LINK_STATE_LINKWATCH_PENDING,
-

&bond->curr_active_slave->dev->state)) {

Review void bond_change_active_slave(struct bon · line 1118

Quoted 1 line
(quoted diff) +1 -0
+

&bond->curr_active_slave->dev->state))

drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon · line 1124

Quoted 8 lines
drivers/net/bonding/bond_main.c +3 -2
 			dprintk("delaying gratuitous arp on %s\n",
 				bond->curr_active_slave->dev->name);
-			bond->send_grat_arp = 1;
-		} else
+		else {
 			bond_send_gratuitous_arp(bond);
+			bond->send_grat_arp--;
+		}

Don't modify the if () else () formatting here. Kernel coding guidelines read if any part of your if/else statement requires braces, all sections need braces. So it still should be, with your changes:

		if (bond->curr_active_slave &&
			test_bit(__LINK_STATE_LINKWATCH_PENDING,
&bond->curr_active_slave->dev->state)) {
			dprintk("delaying gratuitous arp on %s\n",
				bond->curr_active_slave->dev->name);
		} else {
			bond_send_gratuitious_arp(bond);
			bond->send_grat_arp--;
		}

Review static int __bond_mii_monitor(struct bon · line 2149

Quoted 2 lines
(quoted diff) +0 -0
@@ -2144,7 +2149,7 @@ static int __bond_mii_monitor(struct bon
 			dprintk("sending delayed gratuitous arp on on

%s\n",

drivers/net/bonding/bond_main.c static int __bond_mii_monitor(struct bon · line 2152

Quoted 4 lines
drivers/net/bonding/bond_main.c +1 -1
 				bond->curr_active_slave->dev->name);
 			bond_send_gratuitous_arp(bond);
-			bond->send_grat_arp = 0;
+			bond->send_grat_arp--;

Can this ever cause send_grat_arp to become less than zero? What will happen if it does drop below zero? I think some bounds checking might be needed here.

Cheers,
-PJ Waskiewicz

#2 lore raw
Quoted 8 lines · Peter P

drivers/net/bonding/bond_main.c struct bond_params bonding_defaults; · line 109

Quoted 6 lines
drivers/net/bonding/bond_main.c +2 -0
@@ -104,6 +105,8 @@ struct bond_params bonding_defaults;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
+module_param(num_grat_arp, int, 0644);
+MODULE_PARM_DESC(num_grat_arp, "Number of gratuitous ARP packet to

sned on failover");

Quoted 6 lines

Check your spelling here. s/sned/send/

Also, num_grat_arp is declared int, where send_grat_arp is s8. What happens if you overflow send_grat_arp? Either your module parameter needs to change to match what's in the bonding struct, or you needs some bounds checking to validate your inputs.

I'd vote for bounds checking, myself. I think a legal range of
0 - 255 is appropriate.

Quoted 12 lines

Review void bond_change_active_slave(struct bon · line 1113

Quoted 3 lines
(quoted diff) +0 -0
@@ -1109,14 +1112,16 @@ void bond_change_active_slave(struct bon
 		if (new_active && bond->params.fail_over_mac)
 			memcpy(bond->dev->dev_addr,

new_active->dev->dev_addr,

drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon

Quoted 5 lines
drivers/net/bonding/bond_main.c +1 -1
 				new_active->dev->addr_len);
+		bond->send_grat_arp = num_grat_arp;
 		if (bond->curr_active_slave &&
 			test_bit(__LINK_STATE_LINKWATCH_PENDING,
-

&bond->curr_active_slave->dev->state)) {

Review void bond_change_active_slave(struct bon · line 1118

Quoted 1 line
(quoted diff) +1 -0
+

&bond->curr_active_slave->dev->state))

Quoted 12 lines

drivers/net/bonding/bond_main.c void bond_change_active_slave(struct bon · line 1124

Quoted 8 lines
drivers/net/bonding/bond_main.c +3 -2
 			dprintk("delaying gratuitous arp on %s\n",
 				bond->curr_active_slave->dev->name);
-			bond->send_grat_arp = 1;
-		} else
+		else {
 			bond_send_gratuitous_arp(bond);
+			bond->send_grat_arp--;
+		}

Don't modify the if () else () formatting here. Kernel coding guidelines read if any part of your if/else statement requires braces, all sections need braces. So it still should be, with your changes:

Agreed.

[...]

Quoted 11 lines

Review static int __bond_mii_monitor(struct bon · line 2149

Quoted 2 lines
(quoted diff) +0 -0
@@ -2144,7 +2149,7 @@ static int __bond_mii_monitor(struct bon
 			dprintk("sending delayed gratuitous arp on on

%s\n",

drivers/net/bonding/bond_main.c static int __bond_mii_monitor(struct bon · line 2152

Quoted 4 lines
drivers/net/bonding/bond_main.c +1 -1
 				bond->curr_active_slave->dev->name);
 			bond_send_gratuitous_arp(bond);
-			bond->send_grat_arp = 0;
+			bond->send_grat_arp--;

Can this ever cause send_grat_arp to become less than zero? What will happen if it does drop below zero? I think some bounds checking might be needed here.

I don't believe that send_grat_arp can go negative, because this
block is entered on an "if (bond->send_grat_arp)" that's just above the context in the patch.

The patch as a whole also needs a sysfs entry to permit
modification and inspection of the num_grat_arp parameter, as is done for other parameters.

	-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

author
#3 lore raw

Jay, Peter,

Thanks for the comments. I'll resubmit tomorrow.

MoniS

author
#4 lore raw
In reply to Moni Shoua

Jay,
This is a version of the patch with respect to the comments. Please note that besides enabling set/get of the value through sysfs, I also made num_grat_arp a per-bond parameter. thanks
MoniS

--------------------------------------------------------------

With IPoIB, reception of gratuitous ARP by neighboring hosts
is essential for a successful change of slaves in case of failure. Otherwise, they won't learn about the HW address change and need
to wait a long time until the neighboring system gives up and sends an ARP request to learn the new HW address. This patch decreases the chance for a lost of a gratuitous ARP packet by sending it more than once. The number retries is configurable and can be set with a module param.

Signed-off-by Moni Shoua <[email protected]>
---
 drivers/net/bonding/bond_main.c  |   23 ++++++++++++++++++----
 drivers/net/bonding/bond_sysfs.c |   40 +++++++++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h    |    1 
 3 files changed, 60 insertions(+), 4 deletions(-)
drivers/net/bonding/bond_main.c +19 -4
index 0f06753..d48edd4 100644
@@ -88,6 +88,7 @@ #define BOND_LINK_MON_INTERV	0
 #define BOND_LINK_ARP_INTERV	0
 
 static int max_bonds	= BOND_DEFAULT_MAX_BONDS;
+static int num_grat_arp = 1;
 static int miimon	= BOND_LINK_MON_INTERV;
 static int updelay	= 0;
 static int downdelay	= 0;

@@ -104,6 +105,8 @@ struct bond_params bonding_defaults;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
+module_param(num_grat_arp, int, 0644);
+MODULE_PARM_DESC(num_grat_arp, "Number of gratuitous ARP packets to send on failover event");
 module_param(miimon, int, 0);
 MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");
 module_param(updelay, int, 0);

@@ -1109,14 +1112,18 @@ void bond_change_active_slave(struct bon
 		if (new_active && bond->params.fail_over_mac)
 			memcpy(bond->dev->dev_addr,  new_active->dev->dev_addr,
 				new_active->dev->addr_len);
+		bond->send_grat_arp = bond->params.num_grat_arp;
 		if (bond->curr_active_slave &&
 			test_bit(__LINK_STATE_LINKWATCH_PENDING,
 					&bond->curr_active_slave->dev->state)) {
 			dprintk("delaying gratuitous arp on %s\n",
 				bond->curr_active_slave->dev->name);
-			bond->send_grat_arp = 1;
-		} else
-			bond_send_gratuitous_arp(bond);
+		} else {
+			if (bond->send_grat_arp > 0) {
+				bond_send_gratuitous_arp(bond);
+				bond->send_grat_arp--;
+			}
+		}
 	}
 }
 

@@ -2144,7 +2151,7 @@ static int __bond_mii_monitor(struct bon
 			dprintk("sending delayed gratuitous arp on on %s\n",
 				bond->curr_active_slave->dev->name);
 			bond_send_gratuitous_arp(bond);
-			bond->send_grat_arp = 0;
+			bond->send_grat_arp--;
 		}
 	}
 	read_lock(&bond->curr_slave_lock);

@@ -4663,6 +4670,13 @@ static int bond_check_params(struct bond
 		use_carrier = 1;
 	}
 
+	if (num_grat_arp < 0 || num_grat_arp > 255) {
+		printk(KERN_WARNING DRV_NAME
+		       ": Warning: num_grat_arp (%d) not in range 0-255 so it "
+		       "was reset to 1 \n", num_grat_arp);
+		num_grat_arp = 1;
+	}
+
 	/* reset values for 802.3ad */
 	if (bond_mode == BOND_MODE_8023AD) {
 		if (!miimon) {

@@ -4850,6 +4864,7 @@ static int bond_check_params(struct bond
 	params->mode = bond_mode;
 	params->xmit_policy = xmit_hashtype;
 	params->miimon = miimon;
+	params->num_grat_arp = num_grat_arp;
 	params->arp_interval = arp_interval;
 	params->arp_validate = arp_validate_value;
 	params->updelay = updelay;
drivers/net/bonding/bond_sysfs.c +40 -0
index 979c2d0..474ef02 100644
@@ -952,6 +952,45 @@ out:
 static DEVICE_ATTR(lacp_rate, S_IRUGO | S_IWUSR, bonding_show_lacp, bonding_store_lacp);
 
 /*
+ * Show and set the number of grat ARP to send after a failover event.
+ */
+static ssize_t bonding_show_n_grat_arp(struct device *d,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct bonding *bond = to_bond(d);
+
+	return sprintf(buf, "%d\n", bond->params.num_grat_arp);
+}
+
+static ssize_t bonding_store_n_grat_arp(struct device *d,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int new_value, ret = count;
+	struct bonding *bond = to_bond(d);
+
+	if (sscanf(buf, "%d", &new_value) != 1) {
+		printk(KERN_ERR DRV_NAME
+		       ": %s: no num_grat_arp value specified.\n",
+		       bond->dev->name);
+		ret = -EINVAL;
+		goto out;
+	}
+	if (new_value < 0 || new_value > 255) {
+		printk(KERN_ERR DRV_NAME
+		       ": %s: Invalid num_grat_arp value %d not in range 0-255; rejected.\n",
+		       bond->dev->name, new_value);
+		ret = -EINVAL;
+		goto out;
+	} else {
+		bond->params.num_grat_arp = new_value;
+	}
+out:
+	return ret;
+}
+static DEVICE_ATTR(num_grat_arp, S_IRUGO | S_IWUSR, bonding_show_n_grat_arp, bonding_store_n_grat_arp);
+/*
  * Show and set the MII monitor interval.  There are two tricky bits
  * here.  First, if MII monitoring is activated, then we must disable
  * ARP monitoring.  Second, if the timer isn't running, we must

@@ -1388,6 +1427,7 @@ static struct attribute *per_bond_attrs[
 	&dev_attr_updelay.attr,
 	&dev_attr_lacp_rate.attr,
 	&dev_attr_xmit_hash_policy.attr,
+	&dev_attr_num_grat_arp.attr,
 	&dev_attr_miimon.attr,
 	&dev_attr_primary.attr,
 	&dev_attr_use_carrier.attr,
drivers/net/bonding/bonding.h +1 -0
index a3c74e2..ffa4813 100644
@@ -125,6 +125,7 @@ struct bond_params {
 	int mode;
 	int xmit_policy;
 	int miimon;
+	int num_grat_arp;
 	int arp_interval;
 	int arp_validate;
 	int use_carrier;
#5 lore raw

Patch looks good to me, however I have a small nitpick below:

drivers/net/bonding/bond_sysfs.c out: · line 970

Quoted 5 lines
drivers/net/bonding/bond_sysfs.c +5 -0
+static ssize_t bonding_store_n_grat_arp(struct device *d,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int new_value, ret = count;

Avoid declaring multiple variables on the same line when possible. Write it like this:

	int new_value;
	int ret = count;

Here's a blurb from the Documentation/CodingStyle document (not gospel, but pretty darn close):

It's also important to comment data, whether they are basic types or derived
types. To this end, use just one data declaration per line (no commas for
multiple data declarations). This leaves you room for a small comment on each
item, explaining its use.

Cheers,
-PJ Waskiewicz

maintainer
#6 lore raw

From: "Waskiewicz Jr, Peter P" <[email protected]>
Date: Mon, 14 Apr 2008 10:50:17 -0700

Quoted 17 lines

Patch looks good to me, however I have a small nitpick below:

drivers/net/bonding/bond_sysfs.c out: · line 970

Quoted 5 lines
drivers/net/bonding/bond_sysfs.c +5 -0
+static ssize_t bonding_store_n_grat_arp(struct device *d,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int new_value, ret = count;

Avoid declaring multiple variables on the same line when possible. Write it like this:

	int new_value;
	int ret = count;

Here's a blurb from the Documentation/CodingStyle document (not gospel, but pretty darn close):

Quoted 7 lines

It's also important to comment data, whether they are basic types or derived
types. To this end, use just one data declaration per line (no commas for
multiple data declarations). This leaves you room for a small comment on each
item, explaining its use.

You're telling this person to use more lines, but not telling them to add comments, yet that is the reasoning behind the very CodingStyle recommendation you are quoting.

I think you're recommendation is gratuitous and in my opinion wrong. Splitting up the line adds no value, and makes the function take up more lines on the terminal screen.