[Draft PATCH net-next] Bonding: add missed_max option

[Draft PATCH net-next] Bonding: add missed_max option

[Hangbin Liu]

Hi,

Currently, we use hard code number to verify if we are in the
arp_interval timeslice. But some user may want to narrow/extend
the verify timeslice. With the similar team option 'missed_max'
the uers could change that number based on their own environment.

Would you like to help review and see if this is a proper place
for `missed_max` and if I missed anything?

Thanks

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 Documentation/networking/bonding.rst | 10 ++++++++++
 drivers/net/bonding/bond_main.c      | 17 +++++++++--------
 drivers/net/bonding/bond_netlink.c   | 15 +++++++++++++++
 drivers/net/bonding/bond_options.c   | 21 +++++++++++++++++++++
 drivers/net/bonding/bond_procfs.c    |  2 ++
 drivers/net/bonding/bond_sysfs.c     | 13 +++++++++++++
 include/net/bond_options.h           |  1 +
 include/net/bonding.h                |  1 +
 include/uapi/linux/if_link.h         |  1 +
 tools/include/uapi/linux/if_link.h   |  1 +
 10 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index 31cfd7d674a6..41bb5869ff5f 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -421,6 +421,16 @@ arp_all_targets
 		consider the slave up only when all of the arp_ip_targets
 		are reachable
 
+missed_max
+
+        Maximum number of arp_interval for missed ARP replies.
+        If this number is exceeded, link is reported as down.
+
+        Note a small value means a strict time. e.g. missed_max is 1 means
+        the correct arp reply must be recived during the interval.
+
+        default 3
+
 downdelay
 
 	Specifies the time, in milliseconds, to wait before disabling
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index ff8da720a33a..3baae78a7736 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3129,8 +3129,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			 * when the source ip is 0, so don't take the link down
 			 * if we don't know our ip yet
 			 */
-			if (!bond_time_in_interval(bond, trans_start, 2) ||
-			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
+			if (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
+			    !bond_time_in_interval(bond, slave->last_rx, bond->params.missed_max)) {
 
 				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				slave_state_changed = 1;
@@ -3224,7 +3224,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 
 		/* Backup slave is down if:
 		 * - No current_arp_slave AND
-		 * - more than 3*delta since last receive AND
+		 * - more than missed_max*delta since last receive AND
 		 * - the bond has an IP address
 		 *
 		 * Note: a non-null current_arp_slave indicates
@@ -3236,20 +3236,20 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		 */
 		if (!bond_is_active_slave(slave) &&
 		    !rcu_access_pointer(bond->current_arp_slave) &&
-		    !bond_time_in_interval(bond, last_rx, 3)) {
+		    !bond_time_in_interval(bond, last_rx, bond->params.missed_max)) {
 			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 
 		/* Active slave is down if:
-		 * - more than 2*delta since transmitting OR
-		 * - (more than 2*delta since receive AND
+		 * - more than missed_max*delta since transmitting OR
+		 * - (more than missed_max*delta since receive AND
 		 *    the bond has an IP address)
 		 */
 		trans_start = dev_trans_start(slave->dev);
 		if (bond_is_active_slave(slave) &&
-		    (!bond_time_in_interval(bond, trans_start, 2) ||
-		     !bond_time_in_interval(bond, last_rx, 2))) {
+		    (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
+		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {
 			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
@@ -5822,6 +5822,7 @@ static int bond_check_params(struct bond_params *params)
 	params->arp_interval = arp_interval;
 	params->arp_validate = arp_validate_value;
 	params->arp_all_targets = arp_all_targets_value;
+	params->missed_max = 3;
 	params->updelay = updelay;
 	params->downdelay = downdelay;
 	params->peer_notif_delay = 0;
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 5d54e11d18fa..30ccea63228e 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -110,6 +110,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
 					    .len  = ETH_ALEN },
 	[IFLA_BOND_TLB_DYNAMIC_LB]	= { .type = NLA_U8 },
 	[IFLA_BOND_PEER_NOTIF_DELAY]    = { .type = NLA_U32 },
+	[IFLA_BOND_MISSED_MAX]		= { .type = NLA_U32 },
 };
 
 static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
@@ -453,6 +454,15 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
 			return err;
 	}
 
+	if (data[IFLA_BOND_MISSED_MAX]) {
+		int missed_max = nla_get_u8(data[IFLA_BOND_MISSED_MAX]);
+
+		bond_opt_initval(&newval, missed_max);
+		err = __bond_opt_set(bond, BOND_OPT_MISSED_MAX, &newval);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -515,6 +525,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
 		nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
 		nla_total_size(sizeof(u8)) + /* IFLA_BOND_TLB_DYNAMIC_LB */
 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_PEER_NOTIF_DELAY */
+		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MISSED_MAX */
 		0;
 }
 
@@ -650,6 +661,10 @@ static int bond_fill_info(struct sk_buff *skb,
 		       bond->params.tlb_dynamic_lb))
 		goto nla_put_failure;
 
+	if (nla_put_u8(skb, IFLA_BOND_MISSED_MAX,
+		       bond->params.missed_max))
+		goto nla_put_failure;
+
 	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
 		struct ad_info info;
 
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index a8fde3bc458f..57772a9da543 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -78,6 +78,8 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
 					   const struct bond_opt_value *newval);
 static int bond_option_ad_user_port_key_set(struct bonding *bond,
 					    const struct bond_opt_value *newval);
+static int bond_option_missed_max_set(struct bonding *bond,
+				      const struct bond_opt_value *newval);
 
 
 static const struct bond_opt_value bond_mode_tbl[] = {
@@ -270,6 +272,15 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
 		.values = bond_intmax_tbl,
 		.set = bond_option_arp_interval_set
 	},
+	[BOND_OPT_MISSED_MAX] = {
+		.id = BOND_OPT_MISSED_MAX,
+		.name = "missed_max",
+		.desc = "Maximum number of missed ARP interval",
+		.unsuppmodes = BIT(BOND_MODE_8023AD) | BIT(BOND_MODE_TLB) |
+			       BIT(BOND_MODE_ALB),
+		.values = bond_intmax_tbl,
+		.set = bond_option_missed_max_set
+	},
 	[BOND_OPT_ARP_TARGETS] = {
 		.id = BOND_OPT_ARP_TARGETS,
 		.name = "arp_ip_target",
@@ -1186,6 +1197,16 @@ static int bond_option_arp_all_targets_set(struct bonding *bond,
 	return 0;
 }
 
+static int bond_option_missed_max_set(struct bonding *bond,
+				      const struct bond_opt_value *newval)
+{
+	netdev_dbg(bond->dev, "Setting missed max to %s (%llu)\n",
+		   newval->string, newval->value);
+	bond->params.missed_max = newval->value;
+
+	return 0;
+}
+
 static int bond_option_primary_set(struct bonding *bond,
 				   const struct bond_opt_value *newval)
 {
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index f3e3bfd72556..2ec11af5f0cc 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -115,6 +115,8 @@ static void bond_info_show_master(struct seq_file *seq)
 
 		seq_printf(seq, "ARP Polling Interval (ms): %d\n",
 				bond->params.arp_interval);
+		seq_printf(seq, "ARP Missed Max: %u\n",
+				bond->params.missed_max);
 
 		seq_printf(seq, "ARP IP target/s (n.n.n.n form):");
 
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index c48b77167fab..04da21f17503 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -303,6 +303,18 @@ static ssize_t bonding_show_arp_targets(struct device *d,
 static DEVICE_ATTR(arp_ip_target, 0644,
 		   bonding_show_arp_targets, bonding_sysfs_store_option);
 
+/* Show the arp missed max. */
+static ssize_t bonding_show_missed_max(struct device *d,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct bonding *bond = to_bond(d);
+
+	return sprintf(buf, "%u\n", bond->params.missed_max);
+}
+static DEVICE_ATTR(missed_max, 0644,
+		   bonding_show_missed_max, bonding_sysfs_store_option);
+
 /* Show the up and down delays. */
 static ssize_t bonding_show_downdelay(struct device *d,
 				      struct device_attribute *attr,
@@ -779,6 +791,7 @@ static struct attribute *per_bond_attrs[] = {
 	&dev_attr_ad_actor_sys_prio.attr,
 	&dev_attr_ad_actor_system.attr,
 	&dev_attr_ad_user_port_key.attr,
+	&dev_attr_missed_max.attr,
 	NULL,
 };
 
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index e64833a674eb..dd75c071f67e 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -65,6 +65,7 @@ enum {
 	BOND_OPT_NUM_PEER_NOTIF_ALIAS,
 	BOND_OPT_PEER_NOTIF_DELAY,
 	BOND_OPT_LACP_ACTIVE,
+	BOND_OPT_MISSED_MAX,
 	BOND_OPT_LAST
 };
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 15e083e18f75..7b0bcddf9f26 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -124,6 +124,7 @@ struct bond_params {
 	int arp_interval;
 	int arp_validate;
 	int arp_all_targets;
+	unsigned int missed_max;
 	int use_carrier;
 	int fail_over_mac;
 	int updelay;
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index eebd3894fe89..4ac53b30b6dc 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -858,6 +858,7 @@ enum {
 	IFLA_BOND_TLB_DYNAMIC_LB,
 	IFLA_BOND_PEER_NOTIF_DELAY,
 	IFLA_BOND_AD_LACP_ACTIVE,
+	IFLA_BOND_MISSED_MAX,
 	__IFLA_BOND_MAX,
 };
 
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index b3610fdd1fee..4772a115231a 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -655,6 +655,7 @@ enum {
 	IFLA_BOND_TLB_DYNAMIC_LB,
 	IFLA_BOND_PEER_NOTIF_DELAY,
 	IFLA_BOND_AD_LACP_ACTIVE,
+	IFLA_BOND_MISSED_MAX,
 	__IFLA_BOND_MAX,
 };
 
-- 
2.31.1

Re: [Draft PATCH net-next] Bonding: add missed_max option

[Denis Kirjanov]

10/29/21 9:55 AM, Hangbin Liu пишет:
> Hi,
> 
> Currently, we use hard code number to verify if we are in the
> arp_interval timeslice. But some user may want to narrow/extend
> the verify timeslice. With the similar team option 'missed_max'
> the uers could change that number based on their own environment.
> 
> Would you like to help review and see if this is a proper place
> for `missed_max` and if I missed anything?
> 
> Thanks
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   Documentation/networking/bonding.rst | 10 ++++++++++
>   drivers/net/bonding/bond_main.c      | 17 +++++++++--------
>   drivers/net/bonding/bond_netlink.c   | 15 +++++++++++++++
>   drivers/net/bonding/bond_options.c   | 21 +++++++++++++++++++++
>   drivers/net/bonding/bond_procfs.c    |  2 ++
>   drivers/net/bonding/bond_sysfs.c     | 13 +++++++++++++
>   include/net/bond_options.h           |  1 +
>   include/net/bonding.h                |  1 +
>   include/uapi/linux/if_link.h         |  1 +
>   tools/include/uapi/linux/if_link.h   |  1 +
>   10 files changed, 74 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index 31cfd7d674a6..41bb5869ff5f 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -421,6 +421,16 @@ arp_all_targets
>   		consider the slave up only when all of the arp_ip_targets
>   		are reachable
>   
> +missed_max
> +
> +        Maximum number of arp_interval for missed ARP replies.
> +        If this number is exceeded, link is reported as down.
> +
> +        Note a small value means a strict time. e.g. missed_max is 1 means
> +        the correct arp reply must be recived during the interval.
> +
> +        default 3
> +
>   downdelay
>   
>   	Specifies the time, in milliseconds, to wait before disabling
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index ff8da720a33a..3baae78a7736 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3129,8 +3129,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>   			 * when the source ip is 0, so don't take the link down
>   			 * if we don't know our ip yet
>   			 */
> -			if (!bond_time_in_interval(bond, trans_start, 2) ||
> -			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
> +			if (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
> +			    !bond_time_in_interval(bond, slave->last_rx, bond->params.missed_max)) {
>   
>   				bond_propose_link_state(slave, BOND_LINK_DOWN);
>   				slave_state_changed = 1;
> @@ -3224,7 +3224,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>   
>   		/* Backup slave is down if:
>   		 * - No current_arp_slave AND
> -		 * - more than 3*delta since last receive AND
> +		 * - more than missed_max*delta since last receive AND
>   		 * - the bond has an IP address
>   		 *
>   		 * Note: a non-null current_arp_slave indicates
> @@ -3236,20 +3236,20 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>   		 */
>   		if (!bond_is_active_slave(slave) &&
>   		    !rcu_access_pointer(bond->current_arp_slave) &&
> -		    !bond_time_in_interval(bond, last_rx, 3)) {
> +		    !bond_time_in_interval(bond, last_rx, bond->params.missed_max)) {
>   			bond_propose_link_state(slave, BOND_LINK_DOWN);
>   			commit++;
>   		}
>   
>   		/* Active slave is down if:
> -		 * - more than 2*delta since transmitting OR
> -		 * - (more than 2*delta since receive AND
> +		 * - more than missed_max*delta since transmitting OR
> +		 * - (more than missed_max*delta since receive AND
>   		 *    the bond has an IP address)
>   		 */
>   		trans_start = dev_trans_start(slave->dev);
>   		if (bond_is_active_slave(slave) &&
> -		    (!bond_time_in_interval(bond, trans_start, 2) ||
> -		     !bond_time_in_interval(bond, last_rx, 2))) {
> +		    (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
> +		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {
>   			bond_propose_link_state(slave, BOND_LINK_DOWN);
>   			commit++;
>   		}
> @@ -5822,6 +5822,7 @@ static int bond_check_params(struct bond_params *params)
>   	params->arp_interval = arp_interval;
>   	params->arp_validate = arp_validate_value;
>   	params->arp_all_targets = arp_all_targets_value;
> +	params->missed_max = 3;
>   	params->updelay = updelay;
>   	params->downdelay = downdelay;
>   	params->peer_notif_delay = 0;
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 5d54e11d18fa..30ccea63228e 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -110,6 +110,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
>   					    .len  = ETH_ALEN },
>   	[IFLA_BOND_TLB_DYNAMIC_LB]	= { .type = NLA_U8 },
>   	[IFLA_BOND_PEER_NOTIF_DELAY]    = { .type = NLA_U32 },
> +	[IFLA_BOND_MISSED_MAX]		= { .type = NLA_U32 },
>   };
>   
>   static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
> @@ -453,6 +454,15 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
>   			return err;
>   	}
>   
> +	if (data[IFLA_BOND_MISSED_MAX]) {
> +		int missed_max = nla_get_u8(data[IFLA_BOND_MISSED_MAX]);
> +
> +		bond_opt_initval(&newval, missed_max);
> +		err = __bond_opt_set(bond, BOND_OPT_MISSED_MAX, &newval);
> +		if (err)
> +			return err;

	Not sure if 0 makes sense.

> +	}
> +
>   	return 0;
>   }
>   
> @@ -515,6 +525,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
>   		nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
>   		nla_total_size(sizeof(u8)) + /* IFLA_BOND_TLB_DYNAMIC_LB */
>   		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_PEER_NOTIF_DELAY */
> +		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MISSED_MAX */
>   		0;
>   }
>   
> @@ -650,6 +661,10 @@ static int bond_fill_info(struct sk_buff *skb,
>   		       bond->params.tlb_dynamic_lb))
>   		goto nla_put_failure;
>   
> +	if (nla_put_u8(skb, IFLA_BOND_MISSED_MAX,
> +		       bond->params.missed_max))
> +		goto nla_put_failure;
> +
>   	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>   		struct ad_info info;
>   
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index a8fde3bc458f..57772a9da543 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -78,6 +78,8 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
>   					   const struct bond_opt_value *newval);
>   static int bond_option_ad_user_port_key_set(struct bonding *bond,
>   					    const struct bond_opt_value *newval);
> +static int bond_option_missed_max_set(struct bonding *bond,
> +				      const struct bond_opt_value *newval);
>   
>   
>   static const struct bond_opt_value bond_mode_tbl[] = {
> @@ -270,6 +272,15 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
>   		.values = bond_intmax_tbl,
>   		.set = bond_option_arp_interval_set
>   	},
> +	[BOND_OPT_MISSED_MAX] = {
> +		.id = BOND_OPT_MISSED_MAX,
> +		.name = "missed_max",
> +		.desc = "Maximum number of missed ARP interval",
> +		.unsuppmodes = BIT(BOND_MODE_8023AD) | BIT(BOND_MODE_TLB) |
> +			       BIT(BOND_MODE_ALB),
> +		.values = bond_intmax_tbl,
> +		.set = bond_option_missed_max_set
> +	},
>   	[BOND_OPT_ARP_TARGETS] = {
>   		.id = BOND_OPT_ARP_TARGETS,
>   		.name = "arp_ip_target",
> @@ -1186,6 +1197,16 @@ static int bond_option_arp_all_targets_set(struct bonding *bond,
>   	return 0;
>   }
>   
> +static int bond_option_missed_max_set(struct bonding *bond,
> +				      const struct bond_opt_value *newval)
> +{
> +	netdev_dbg(bond->dev, "Setting missed max to %s (%llu)\n",
> +		   newval->string, newval->value);
> +	bond->params.missed_max = newval->value;
> +
> +	return 0;
> +}
> +
>   static int bond_option_primary_set(struct bonding *bond,
>   				   const struct bond_opt_value *newval)
>   {
> diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
> index f3e3bfd72556..2ec11af5f0cc 100644
> --- a/drivers/net/bonding/bond_procfs.c
> +++ b/drivers/net/bonding/bond_procfs.c
> @@ -115,6 +115,8 @@ static void bond_info_show_master(struct seq_file *seq)
>   
>   		seq_printf(seq, "ARP Polling Interval (ms): %d\n",
>   				bond->params.arp_interval);
> +		seq_printf(seq, "ARP Missed Max: %u\n",
> +				bond->params.missed_max);

You should specify it in units

>   
>   		seq_printf(seq, "ARP IP target/s (n.n.n.n form):");
>   
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index c48b77167fab..04da21f17503 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -303,6 +303,18 @@ static ssize_t bonding_show_arp_targets(struct device *d,
>   static DEVICE_ATTR(arp_ip_target, 0644,
>   		   bonding_show_arp_targets, bonding_sysfs_store_option);
>   
> +/* Show the arp missed max. */
> +static ssize_t bonding_show_missed_max(struct device *d,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct bonding *bond = to_bond(d);
> +
> +	return sprintf(buf, "%u\n", bond->params.missed_max);
> +}
> +static DEVICE_ATTR(missed_max, 0644,
> +		   bonding_show_missed_max, bonding_sysfs_store_option);
> +
>   /* Show the up and down delays. */
>   static ssize_t bonding_show_downdelay(struct device *d,
>   				      struct device_attribute *attr,
> @@ -779,6 +791,7 @@ static struct attribute *per_bond_attrs[] = {
>   	&dev_attr_ad_actor_sys_prio.attr,
>   	&dev_attr_ad_actor_system.attr,
>   	&dev_attr_ad_user_port_key.attr,
> +	&dev_attr_missed_max.attr,
>   	NULL,
>   };
>   
> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> index e64833a674eb..dd75c071f67e 100644
> --- a/include/net/bond_options.h
> +++ b/include/net/bond_options.h
> @@ -65,6 +65,7 @@ enum {
>   	BOND_OPT_NUM_PEER_NOTIF_ALIAS,
>   	BOND_OPT_PEER_NOTIF_DELAY,
>   	BOND_OPT_LACP_ACTIVE,
> +	BOND_OPT_MISSED_MAX,
>   	BOND_OPT_LAST
>   };
>   
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 15e083e18f75..7b0bcddf9f26 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -124,6 +124,7 @@ struct bond_params {
>   	int arp_interval;
>   	int arp_validate;
>   	int arp_all_targets;
> +	unsigned int missed_max;
>   	int use_carrier;
>   	int fail_over_mac;
>   	int updelay;
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index eebd3894fe89..4ac53b30b6dc 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -858,6 +858,7 @@ enum {
>   	IFLA_BOND_TLB_DYNAMIC_LB,
>   	IFLA_BOND_PEER_NOTIF_DELAY,
>   	IFLA_BOND_AD_LACP_ACTIVE,
> +	IFLA_BOND_MISSED_MAX,
>   	__IFLA_BOND_MAX,
>   };
>   
> diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
> index b3610fdd1fee..4772a115231a 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -655,6 +655,7 @@ enum {
>   	IFLA_BOND_TLB_DYNAMIC_LB,
>   	IFLA_BOND_PEER_NOTIF_DELAY,
>   	IFLA_BOND_AD_LACP_ACTIVE,
> +	IFLA_BOND_MISSED_MAX,
>   	__IFLA_BOND_MAX,
>   };
>   
> 

Re: [Draft PATCH net-next] Bonding: add missed_max option

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Hi,
>
>Currently, we use hard code number to verify if we are in the
>arp_interval timeslice. But some user may want to narrow/extend
>the verify timeslice. With the similar team option 'missed_max'
>the uers could change that number based on their own environment.
>
>Would you like to help review and see if this is a proper place
>for `missed_max` and if I missed anything?
>
>Thanks
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> Documentation/networking/bonding.rst | 10 ++++++++++
> drivers/net/bonding/bond_main.c      | 17 +++++++++--------
> drivers/net/bonding/bond_netlink.c   | 15 +++++++++++++++
> drivers/net/bonding/bond_options.c   | 21 +++++++++++++++++++++
> drivers/net/bonding/bond_procfs.c    |  2 ++
> drivers/net/bonding/bond_sysfs.c     | 13 +++++++++++++
> include/net/bond_options.h           |  1 +
> include/net/bonding.h                |  1 +
> include/uapi/linux/if_link.h         |  1 +
> tools/include/uapi/linux/if_link.h   |  1 +
> 10 files changed, 74 insertions(+), 8 deletions(-)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index 31cfd7d674a6..41bb5869ff5f 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -421,6 +421,16 @@ arp_all_targets
> 		consider the slave up only when all of the arp_ip_targets
> 		are reachable
> 
>+missed_max
>+
>+        Maximum number of arp_interval for missed ARP replies.
>+        If this number is exceeded, link is reported as down.
>+
>+        Note a small value means a strict time. e.g. missed_max is 1 means
>+        the correct arp reply must be recived during the interval.
>+
>+        default 3

	I'd suggest "arp" in the option name to make the scope more
obvious.

>+
> downdelay
> 
> 	Specifies the time, in milliseconds, to wait before disabling
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index ff8da720a33a..3baae78a7736 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3129,8 +3129,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
> 			 * when the source ip is 0, so don't take the link down
> 			 * if we don't know our ip yet
> 			 */
>-			if (!bond_time_in_interval(bond, trans_start, 2) ||
>-			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
>+			if (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
>+			    !bond_time_in_interval(bond, slave->last_rx, bond->params.missed_max)) {
> 
> 				bond_propose_link_state(slave, BOND_LINK_DOWN);
> 				slave_state_changed = 1;
>@@ -3224,7 +3224,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> 
> 		/* Backup slave is down if:
> 		 * - No current_arp_slave AND
>-		 * - more than 3*delta since last receive AND
>+		 * - more than missed_max*delta since last receive AND
> 		 * - the bond has an IP address
> 		 *
> 		 * Note: a non-null current_arp_slave indicates
>@@ -3236,20 +3236,20 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> 		 */
> 		if (!bond_is_active_slave(slave) &&
> 		    !rcu_access_pointer(bond->current_arp_slave) &&
>-		    !bond_time_in_interval(bond, last_rx, 3)) {
>+		    !bond_time_in_interval(bond, last_rx, bond->params.missed_max)) {
> 			bond_propose_link_state(slave, BOND_LINK_DOWN);
> 			commit++;
> 		}
> 
> 		/* Active slave is down if:
>-		 * - more than 2*delta since transmitting OR
>-		 * - (more than 2*delta since receive AND
>+		 * - more than missed_max*delta since transmitting OR
>+		 * - (more than missed_max*delta since receive AND
> 		 *    the bond has an IP address)
> 		 */
> 		trans_start = dev_trans_start(slave->dev);
> 		if (bond_is_active_slave(slave) &&
>-		    (!bond_time_in_interval(bond, trans_start, 2) ||
>-		     !bond_time_in_interval(bond, last_rx, 2))) {
>+		    (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
>+		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {
> 			bond_propose_link_state(slave, BOND_LINK_DOWN);
> 			commit++;
> 		}

	The above two changes make the backup and active logic both
switch to using the missed_max value (i.e., both set to the same value),
when previously these two cases used differing values (2 for active, 3
for backup).

	Historically, these intervals were staggered deliberately; an
old comment removed by b2220cad583c9b states:

			if ((slave != bond->curr_active_slave) &&
			    (!bond->current_arp_slave) &&
			    (time_after_eq(jiffies, slave_last_rx(bond, slave) + 3*delta_in_ticks))) {
				/* a backup slave has gone down; three times
				 * the delta allows the current slave to be
				 * taken out before the backup slave.

	I think it would be prudent to insure that having the active and
backup timeouts set in lockstep does not result in an undesirable change
of behavior.

>@@ -5822,6 +5822,7 @@ static int bond_check_params(struct bond_params *params)
> 	params->arp_interval = arp_interval;
> 	params->arp_validate = arp_validate_value;
> 	params->arp_all_targets = arp_all_targets_value;
>+	params->missed_max = 3;
> 	params->updelay = updelay;
> 	params->downdelay = downdelay;
> 	params->peer_notif_delay = 0;
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 5d54e11d18fa..30ccea63228e 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -110,6 +110,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> 					    .len  = ETH_ALEN },
> 	[IFLA_BOND_TLB_DYNAMIC_LB]	= { .type = NLA_U8 },
> 	[IFLA_BOND_PEER_NOTIF_DELAY]    = { .type = NLA_U32 },
>+	[IFLA_BOND_MISSED_MAX]		= { .type = NLA_U32 },
> };
> 
> static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
>@@ -453,6 +454,15 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
> 			return err;
> 	}
> 
>+	if (data[IFLA_BOND_MISSED_MAX]) {
>+		int missed_max = nla_get_u8(data[IFLA_BOND_MISSED_MAX]);
>+
>+		bond_opt_initval(&newval, missed_max);
>+		err = __bond_opt_set(bond, BOND_OPT_MISSED_MAX, &newval);
>+		if (err)
>+			return err;
>+	}
>+
> 	return 0;
> }
> 
>@@ -515,6 +525,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> 		nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
> 		nla_total_size(sizeof(u8)) + /* IFLA_BOND_TLB_DYNAMIC_LB */
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_PEER_NOTIF_DELAY */
>+		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MISSED_MAX */
> 		0;
> }
> 
>@@ -650,6 +661,10 @@ static int bond_fill_info(struct sk_buff *skb,
> 		       bond->params.tlb_dynamic_lb))
> 		goto nla_put_failure;
> 
>+	if (nla_put_u8(skb, IFLA_BOND_MISSED_MAX,
>+		       bond->params.missed_max))
>+		goto nla_put_failure;
>+
> 	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> 		struct ad_info info;
> 
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index a8fde3bc458f..57772a9da543 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -78,6 +78,8 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
> 					   const struct bond_opt_value *newval);
> static int bond_option_ad_user_port_key_set(struct bonding *bond,
> 					    const struct bond_opt_value *newval);
>+static int bond_option_missed_max_set(struct bonding *bond,
>+				      const struct bond_opt_value *newval);
> 
> 
> static const struct bond_opt_value bond_mode_tbl[] = {
>@@ -270,6 +272,15 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
> 		.values = bond_intmax_tbl,
> 		.set = bond_option_arp_interval_set
> 	},
>+	[BOND_OPT_MISSED_MAX] = {
>+		.id = BOND_OPT_MISSED_MAX,
>+		.name = "missed_max",
>+		.desc = "Maximum number of missed ARP interval",
>+		.unsuppmodes = BIT(BOND_MODE_8023AD) | BIT(BOND_MODE_TLB) |
>+			       BIT(BOND_MODE_ALB),
>+		.values = bond_intmax_tbl,

	This allows missed_max to be set to 0; is that intended to be a
valid setting?

	-J

>+		.set = bond_option_missed_max_set
>+	},
> 	[BOND_OPT_ARP_TARGETS] = {
> 		.id = BOND_OPT_ARP_TARGETS,
> 		.name = "arp_ip_target",
>@@ -1186,6 +1197,16 @@ static int bond_option_arp_all_targets_set(struct bonding *bond,
> 	return 0;
> }
> 
>+static int bond_option_missed_max_set(struct bonding *bond,
>+				      const struct bond_opt_value *newval)
>+{
>+	netdev_dbg(bond->dev, "Setting missed max to %s (%llu)\n",
>+		   newval->string, newval->value);
>+	bond->params.missed_max = newval->value;
>+
>+	return 0;
>+}
>+
> static int bond_option_primary_set(struct bonding *bond,
> 				   const struct bond_opt_value *newval)
> {
>diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
>index f3e3bfd72556..2ec11af5f0cc 100644
>--- a/drivers/net/bonding/bond_procfs.c
>+++ b/drivers/net/bonding/bond_procfs.c
>@@ -115,6 +115,8 @@ static void bond_info_show_master(struct seq_file *seq)
> 
> 		seq_printf(seq, "ARP Polling Interval (ms): %d\n",
> 				bond->params.arp_interval);
>+		seq_printf(seq, "ARP Missed Max: %u\n",
>+				bond->params.missed_max);
> 
> 		seq_printf(seq, "ARP IP target/s (n.n.n.n form):");
> 
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index c48b77167fab..04da21f17503 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -303,6 +303,18 @@ static ssize_t bonding_show_arp_targets(struct device *d,
> static DEVICE_ATTR(arp_ip_target, 0644,
> 		   bonding_show_arp_targets, bonding_sysfs_store_option);
> 
>+/* Show the arp missed max. */
>+static ssize_t bonding_show_missed_max(struct device *d,
>+				       struct device_attribute *attr,
>+				       char *buf)
>+{
>+	struct bonding *bond = to_bond(d);
>+
>+	return sprintf(buf, "%u\n", bond->params.missed_max);
>+}
>+static DEVICE_ATTR(missed_max, 0644,
>+		   bonding_show_missed_max, bonding_sysfs_store_option);
>+
> /* Show the up and down delays. */
> static ssize_t bonding_show_downdelay(struct device *d,
> 				      struct device_attribute *attr,
>@@ -779,6 +791,7 @@ static struct attribute *per_bond_attrs[] = {
> 	&dev_attr_ad_actor_sys_prio.attr,
> 	&dev_attr_ad_actor_system.attr,
> 	&dev_attr_ad_user_port_key.attr,
>+	&dev_attr_missed_max.attr,
> 	NULL,
> };
> 
>diff --git a/include/net/bond_options.h b/include/net/bond_options.h
>index e64833a674eb..dd75c071f67e 100644
>--- a/include/net/bond_options.h
>+++ b/include/net/bond_options.h
>@@ -65,6 +65,7 @@ enum {
> 	BOND_OPT_NUM_PEER_NOTIF_ALIAS,
> 	BOND_OPT_PEER_NOTIF_DELAY,
> 	BOND_OPT_LACP_ACTIVE,
>+	BOND_OPT_MISSED_MAX,
> 	BOND_OPT_LAST
> };
> 
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 15e083e18f75..7b0bcddf9f26 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -124,6 +124,7 @@ struct bond_params {
> 	int arp_interval;
> 	int arp_validate;
> 	int arp_all_targets;
>+	unsigned int missed_max;
> 	int use_carrier;
> 	int fail_over_mac;
> 	int updelay;
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index eebd3894fe89..4ac53b30b6dc 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -858,6 +858,7 @@ enum {
> 	IFLA_BOND_TLB_DYNAMIC_LB,
> 	IFLA_BOND_PEER_NOTIF_DELAY,
> 	IFLA_BOND_AD_LACP_ACTIVE,
>+	IFLA_BOND_MISSED_MAX,
> 	__IFLA_BOND_MAX,
> };
> 
>diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
>index b3610fdd1fee..4772a115231a 100644
>--- a/tools/include/uapi/linux/if_link.h
>+++ b/tools/include/uapi/linux/if_link.h
>@@ -655,6 +655,7 @@ enum {
> 	IFLA_BOND_TLB_DYNAMIC_LB,
> 	IFLA_BOND_PEER_NOTIF_DELAY,
> 	IFLA_BOND_AD_LACP_ACTIVE,
>+	IFLA_BOND_MISSED_MAX,
> 	__IFLA_BOND_MAX,
> };
> 
>-- 
>2.31.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [Draft PATCH net-next] Bonding: add missed_max option

[Hangbin Liu]

Hi Jay, Denis,

Thanks for the comments. Please see my replies below.

On Fri, Oct 29, 2021 at 03:11:18PM -0700, Jay Vosburgh wrote:
> >+missed_max
> >+
> >+        Maximum number of arp_interval for missed ARP replies.
> >+        If this number is exceeded, link is reported as down.
> >+
> >+        Note a small value means a strict time. e.g. missed_max is 1 means
> >+        the correct arp reply must be recived during the interval.
> >+
> >+        default 3
> 
> 	I'd suggest "arp" in the option name to make the scope more
> obvious.

I didn't add arp prefix in purpose because I'd like to re-use this parameter
after adding bonding IPv6 NS/NA support. I will add this reason in the commit
description.

Or if you like to make a difference for ARP and IPv6 NS, I can add the arp_
prefix.

> >@@ -3224,7 +3224,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> > 
> > 		/* Backup slave is down if:
> > 		 * - No current_arp_slave AND
> >-		 * - more than 3*delta since last receive AND
> >+		 * - more than missed_max*delta since last receive AND
> > 		 * - the bond has an IP address
> > 		 *
> > 		 * Note: a non-null current_arp_slave indicates
> >@@ -3236,20 +3236,20 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> > 		 */
> > 		if (!bond_is_active_slave(slave) &&
> > 		    !rcu_access_pointer(bond->current_arp_slave) &&
> >-		    !bond_time_in_interval(bond, last_rx, 3)) {
> >+		    !bond_time_in_interval(bond, last_rx, bond->params.missed_max)) {
> > 			bond_propose_link_state(slave, BOND_LINK_DOWN);
> > 			commit++;
> > 		}
> > 
> > 		/* Active slave is down if:
> >-		 * - more than 2*delta since transmitting OR
> >-		 * - (more than 2*delta since receive AND
> >+		 * - more than missed_max*delta since transmitting OR
> >+		 * - (more than missed_max*delta since receive AND
> > 		 *    the bond has an IP address)
> > 		 */
> > 		trans_start = dev_trans_start(slave->dev);
> > 		if (bond_is_active_slave(slave) &&
> >-		    (!bond_time_in_interval(bond, trans_start, 2) ||
> >-		     !bond_time_in_interval(bond, last_rx, 2))) {
> >+		    (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
> >+		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {
> > 			bond_propose_link_state(slave, BOND_LINK_DOWN);
> > 			commit++;
> > 		}
> 
> 	The above two changes make the backup and active logic both
> switch to using the missed_max value (i.e., both set to the same value),
> when previously these two cases used differing values (2 for active, 3
> for backup).
> 
> 	Historically, these intervals were staggered deliberately; an
> old comment removed by b2220cad583c9b states:
> 
> 			if ((slave != bond->curr_active_slave) &&
> 			    (!bond->current_arp_slave) &&
> 			    (time_after_eq(jiffies, slave_last_rx(bond, slave) + 3*delta_in_ticks))) {
> 				/* a backup slave has gone down; three times
> 				 * the delta allows the current slave to be
> 				 * taken out before the backup slave.
> 
> 	I think it would be prudent to insure that having the active and
> backup timeouts set in lockstep does not result in an undesirable change
> of behavior.

Yes, I'm also a little concern about this. What about make the backup slave
timeout 1 plus delta then active slave. i.e. for backup slave

bond_time_in_interval(bond, last_rx, bond->params.missed_max + 1)

> >@@ -270,6 +272,15 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
> > 		.values = bond_intmax_tbl,
> > 		.set = bond_option_arp_interval_set
> > 	},
> >+	[BOND_OPT_MISSED_MAX] = {
> >+		.id = BOND_OPT_MISSED_MAX,
> >+		.name = "missed_max",
> >+		.desc = "Maximum number of missed ARP interval",
> >+		.unsuppmodes = BIT(BOND_MODE_8023AD) | BIT(BOND_MODE_TLB) |
> >+			       BIT(BOND_MODE_ALB),
> >+		.values = bond_intmax_tbl,
> 
> 	This allows missed_max to be set to 0; is that intended to be a
> valid setting?

In bond_time_in_interval() there is an arp_interval/2 extra time. Do you think
if this is enough for fast network when we set missed_max to 0?

Of course in the very fast network the value should be at lease 1 in case
the ARP send/recv time frame is same.

Thanks
Hangbin