[PATCH net-next 3/8] bonding: add downdelay netlink support

[PATCH net-next 3/8] bonding: add downdelay netlink support

[Scott Feldman]

Add IFLA_BOND_DOWNDELAY to allow get/set of bonding parameter
downdelay via netlink.

Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
---
 drivers/net/bonding/bond_netlink.c |   12 +++++++++++
 drivers/net/bonding/bond_options.c |   29 +++++++++++++++++++++++++++
 drivers/net/bonding/bond_sysfs.c   |   38 +++++++++---------------------------
 drivers/net/bonding/bonding.h      |    1 +
 include/uapi/linux/if_link.h       |    1 +
 5 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 9ba5431..e684713 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -26,6 +26,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
 	[IFLA_BOND_ACTIVE_SLAVE]	= { .type = NLA_U32 },
 	[IFLA_BOND_MIIMON]		= { .type = NLA_U32 },
 	[IFLA_BOND_UPDELAY]		= { .type = NLA_U32 },
+	[IFLA_BOND_DOWNDELAY]		= { .type = NLA_U32 },
 };
 
 static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
@@ -85,6 +86,13 @@ static int bond_changelink(struct net_device *bond_dev,
 		if (err)
 			return err;
 	}
+	if (data[IFLA_BOND_DOWNDELAY]) {
+		int downdelay = nla_get_u32(data[IFLA_BOND_DOWNDELAY]);
+
+		err = bond_option_downdelay_set(bond, downdelay);
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
@@ -106,6 +114,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_ACTIVE_SLAVE */
 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MIIMON */
 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_UPDELAY */
+		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_DOWNDELAY */
 		0;
 }
 
@@ -128,6 +137,9 @@ static int bond_fill_info(struct sk_buff *skb,
 	if (nla_put_u32(skb, IFLA_BOND_UPDELAY, bond->params.updelay))
 		goto nla_put_failure;
 
+	if (nla_put_u32(skb, IFLA_BOND_DOWNDELAY, bond->params.downdelay))
+		goto nla_put_failure;
+
 	return 0;
 
 nla_put_failure:
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index c0fea66..738be5f 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -211,3 +211,32 @@ int bond_option_updelay_set(struct bonding *bond, int updelay)
 
 	return 0;
 }
+
+int bond_option_downdelay_set(struct bonding *bond, int downdelay)
+{
+	if (!(bond->params.miimon)) {
+		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);
+		return -EPERM;
+	}
+
+	if (downdelay < 0) {
+		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
+		       bond->dev->name, downdelay, 0, INT_MAX);
+		return -EINVAL;
+	} else {
+		if ((downdelay % bond->params.miimon) != 0) {
+			pr_warn("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
+				bond->dev->name, downdelay,
+				bond->params.miimon,
+				(downdelay / bond->params.miimon) *
+				bond->params.miimon);
+		}
+		bond->params.downdelay = downdelay / bond->params.miimon;
+		pr_info("%s: Setting down delay to %d.\n",
+			bond->dev->name,
+			bond->params.downdelay * bond->params.miimon);
+
+	}
+
+	return 0;
+}
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 1125bef..41a62ac 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -706,42 +706,22 @@ static ssize_t bonding_store_downdelay(struct device *d,
 				       struct device_attribute *attr,
 				       const char *buf, size_t count)
 {
-	int new_value, ret = count;
+	int new_value, ret;
 	struct bonding *bond = to_bond(d);
 
-	if (!(bond->params.miimon)) {
-		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
-		       bond->dev->name);
-		ret = -EPERM;
-		goto out;
-	}
-
 	if (sscanf(buf, "%d", &new_value) != 1) {
 		pr_err("%s: no down delay value specified.\n", bond->dev->name);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
-	if (new_value < 0) {
-		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
-		       bond->dev->name, new_value, 0, INT_MAX);
-		ret = -EINVAL;
-		goto out;
-	} else {
-		if ((new_value % bond->params.miimon) != 0) {
-			pr_warning("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
-				   bond->dev->name, new_value,
-				   bond->params.miimon,
-				   (new_value / bond->params.miimon) *
-				   bond->params.miimon);
-		}
-		bond->params.downdelay = new_value / bond->params.miimon;
-		pr_info("%s: Setting down delay to %d.\n",
-			bond->dev->name,
-			bond->params.downdelay * bond->params.miimon);
 
-	}
+	if (!rtnl_trylock())
+		return restart_syscall();
 
-out:
+	ret = bond_option_downdelay_set(bond, new_value);
+	if (!ret)
+		ret = count;
+
+	rtnl_unlock();
 	return ret;
 }
 static DEVICE_ATTR(downdelay, S_IRUGO | S_IWUSR,
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 408e6c2..40987f3 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -430,6 +430,7 @@ int bond_option_mode_set(struct bonding *bond, int mode);
 int bond_option_active_slave_set(struct bonding *bond, struct net_device *slave_dev);
 int bond_option_miimon_set(struct bonding *bond, int miimon);
 int bond_option_updelay_set(struct bonding *bond, int updelay);
+int bond_option_downdelay_set(struct bonding *bond, int downdelay);
 struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
 struct net_device *bond_option_active_slave_get(struct bonding *bond);
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 361414e..a372831 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -333,6 +333,7 @@ enum {
 	IFLA_BOND_ACTIVE_SLAVE,
 	IFLA_BOND_MIIMON,
 	IFLA_BOND_UPDELAY,
+	IFLA_BOND_DOWNDELAY,
 	__IFLA_BOND_MAX,
 };
 

Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Veaceslav Falico]

On Thu, Nov 07, 2013 at 01:43:08AM -0800, Scott Feldman wrote:
>Add IFLA_BOND_DOWNDELAY to allow get/set of bonding parameter
>downdelay via netlink.
>
>Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
>---
> drivers/net/bonding/bond_netlink.c |   12 +++++++++++
> drivers/net/bonding/bond_options.c |   29 +++++++++++++++++++++++++++
> drivers/net/bonding/bond_sysfs.c   |   38 +++++++++---------------------------
> drivers/net/bonding/bonding.h      |    1 +
> include/uapi/linux/if_link.h       |    1 +
> 5 files changed, 52 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 9ba5431..e684713 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -26,6 +26,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> 	[IFLA_BOND_ACTIVE_SLAVE]	= { .type = NLA_U32 },
> 	[IFLA_BOND_MIIMON]		= { .type = NLA_U32 },
> 	[IFLA_BOND_UPDELAY]		= { .type = NLA_U32 },
>+	[IFLA_BOND_DOWNDELAY]		= { .type = NLA_U32 },
> };
>
> static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
>@@ -85,6 +86,13 @@ static int bond_changelink(struct net_device *bond_dev,
> 		if (err)
> 			return err;
> 	}
>+	if (data[IFLA_BOND_DOWNDELAY]) {
>+		int downdelay = nla_get_u32(data[IFLA_BOND_DOWNDELAY]);
>+
>+		err = bond_option_downdelay_set(bond, downdelay);
>+		if (err)
>+			return err;
>+	}
> 	return 0;
> }
>
>@@ -106,6 +114,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_ACTIVE_SLAVE */
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MIIMON */
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_UPDELAY */
>+		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_DOWNDELAY */
> 		0;
> }
>
>@@ -128,6 +137,9 @@ static int bond_fill_info(struct sk_buff *skb,
> 	if (nla_put_u32(skb, IFLA_BOND_UPDELAY, bond->params.updelay))
> 		goto nla_put_failure;
>
>+	if (nla_put_u32(skb, IFLA_BOND_DOWNDELAY, bond->params.downdelay))
>+		goto nla_put_failure;
>+
> 	return 0;
>
> nla_put_failure:
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index c0fea66..738be5f 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -211,3 +211,32 @@ int bond_option_updelay_set(struct bonding *bond, int updelay)
>
> 	return 0;
> }
>+
>+int bond_option_downdelay_set(struct bonding *bond, int downdelay)
>+{
>+	if (!(bond->params.miimon)) {
>+		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);

Maybe get bond->dev->name on the next line?

>+		return -EPERM;
>+	}
>+
>+	if (downdelay < 0) {
>+		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
>+		       bond->dev->name, downdelay, 0, INT_MAX);
>+		return -EINVAL;
>+	} else {
>+		if ((downdelay % bond->params.miimon) != 0) {
>+			pr_warn("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
>+				bond->dev->name, downdelay,
>+				bond->params.miimon,
>+				(downdelay / bond->params.miimon) *
>+				bond->params.miimon);
>+		}
>+		bond->params.downdelay = downdelay / bond->params.miimon;
>+		pr_info("%s: Setting down delay to %d.\n",
>+			bond->dev->name,
>+			bond->params.downdelay * bond->params.miimon);
>+
>+	}
>+
>+	return 0;
>+}
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 1125bef..41a62ac 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -706,42 +706,22 @@ static ssize_t bonding_store_downdelay(struct device *d,
> 				       struct device_attribute *attr,
> 				       const char *buf, size_t count)
> {
>-	int new_value, ret = count;
>+	int new_value, ret;
> 	struct bonding *bond = to_bond(d);
>
>-	if (!(bond->params.miimon)) {
>-		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
>-		       bond->dev->name);
>-		ret = -EPERM;
>-		goto out;
>-	}
>-
> 	if (sscanf(buf, "%d", &new_value) != 1) {
> 		pr_err("%s: no down delay value specified.\n", bond->dev->name);
>-		ret = -EINVAL;
>-		goto out;
>+		return -EINVAL;
> 	}
>-	if (new_value < 0) {
>-		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
>-		       bond->dev->name, new_value, 0, INT_MAX);
>-		ret = -EINVAL;
>-		goto out;
>-	} else {
>-		if ((new_value % bond->params.miimon) != 0) {
>-			pr_warning("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
>-				   bond->dev->name, new_value,
>-				   bond->params.miimon,
>-				   (new_value / bond->params.miimon) *
>-				   bond->params.miimon);
>-		}
>-		bond->params.downdelay = new_value / bond->params.miimon;
>-		pr_info("%s: Setting down delay to %d.\n",
>-			bond->dev->name,
>-			bond->params.downdelay * bond->params.miimon);
>
>-	}
>+	if (!rtnl_trylock())
>+		return restart_syscall();
>
>-out:
>+	ret = bond_option_downdelay_set(bond, new_value);
>+	if (!ret)
>+		ret = count;
>+
>+	rtnl_unlock();
> 	return ret;
> }
> static DEVICE_ATTR(downdelay, S_IRUGO | S_IWUSR,
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 408e6c2..40987f3 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -430,6 +430,7 @@ int bond_option_mode_set(struct bonding *bond, int mode);
> int bond_option_active_slave_set(struct bonding *bond, struct net_device *slave_dev);
> int bond_option_miimon_set(struct bonding *bond, int miimon);
> int bond_option_updelay_set(struct bonding *bond, int updelay);
>+int bond_option_downdelay_set(struct bonding *bond, int downdelay);
> struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
> struct net_device *bond_option_active_slave_get(struct bonding *bond);
>
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index 361414e..a372831 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -333,6 +333,7 @@ enum {
> 	IFLA_BOND_ACTIVE_SLAVE,
> 	IFLA_BOND_MIIMON,
> 	IFLA_BOND_UPDELAY,
>+	IFLA_BOND_DOWNDELAY,
> 	__IFLA_BOND_MAX,
> };
>
>

Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Jay Vosburgh]

Scott Feldman <sfeldma@cumulusnetworks.com> wrote:

>Add IFLA_BOND_DOWNDELAY to allow get/set of bonding parameter
>downdelay via netlink.
>
>Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
>---
> drivers/net/bonding/bond_netlink.c |   12 +++++++++++
> drivers/net/bonding/bond_options.c |   29 +++++++++++++++++++++++++++
> drivers/net/bonding/bond_sysfs.c   |   38 +++++++++---------------------------
> drivers/net/bonding/bonding.h      |    1 +
> include/uapi/linux/if_link.h       |    1 +
> 5 files changed, 52 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 9ba5431..e684713 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -26,6 +26,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> 	[IFLA_BOND_ACTIVE_SLAVE]	= { .type = NLA_U32 },
> 	[IFLA_BOND_MIIMON]		= { .type = NLA_U32 },
> 	[IFLA_BOND_UPDELAY]		= { .type = NLA_U32 },
>+	[IFLA_BOND_DOWNDELAY]		= { .type = NLA_U32 },
> };
>
> static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
>@@ -85,6 +86,13 @@ static int bond_changelink(struct net_device *bond_dev,
> 		if (err)
> 			return err;
> 	}
>+	if (data[IFLA_BOND_DOWNDELAY]) {
>+		int downdelay = nla_get_u32(data[IFLA_BOND_DOWNDELAY]);
>+
>+		err = bond_option_downdelay_set(bond, downdelay);
>+		if (err)
>+			return err;
>+	}
> 	return 0;
> }
>
>@@ -106,6 +114,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_ACTIVE_SLAVE */
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_MIIMON */
> 		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_UPDELAY */
>+		nla_total_size(sizeof(u32)) +	/* IFLA_BOND_DOWNDELAY */
> 		0;
> }
>
>@@ -128,6 +137,9 @@ static int bond_fill_info(struct sk_buff *skb,
> 	if (nla_put_u32(skb, IFLA_BOND_UPDELAY, bond->params.updelay))
> 		goto nla_put_failure;
>
>+	if (nla_put_u32(skb, IFLA_BOND_DOWNDELAY, bond->params.downdelay))
>+		goto nla_put_failure;
>+
> 	return 0;
>
> nla_put_failure:
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index c0fea66..738be5f 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -211,3 +211,32 @@ int bond_option_updelay_set(struct bonding *bond, int updelay)
>
> 	return 0;
> }
>+
>+int bond_option_downdelay_set(struct bonding *bond, int downdelay)
>+{
>+	if (!(bond->params.miimon)) {
>+		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);
>+		return -EPERM;
>+	}

	I would argue that it is better to permit this option to be set
at any time, regardless of whether miimon is enabled or not.  This
removes an ordering constraint and makes things generally easier to deal
with.  Setting the option has no effect if miimon is not set, but that's
ok.

	My comment here would also apply to other options that are
"secondary," in the sense that they affect the behavior of other options
or modes, e.g., updelay, arp_validate (in another path of this series),
arp_ip_targets, etc.

	I'm aware that this is simply duplicating the existing behavior
of the sysfs API, but I'd be in favor of changing that, too.  These
limitations are a holdover from the module paramters days, and should
have been made more flexible a long time ago.

	-J

>+
>+	if (downdelay < 0) {
>+		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
>+		       bond->dev->name, downdelay, 0, INT_MAX);
>+		return -EINVAL;
>+	} else {
>+		if ((downdelay % bond->params.miimon) != 0) {
>+			pr_warn("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
>+				bond->dev->name, downdelay,
>+				bond->params.miimon,
>+				(downdelay / bond->params.miimon) *
>+				bond->params.miimon);
>+		}
>+		bond->params.downdelay = downdelay / bond->params.miimon;
>+		pr_info("%s: Setting down delay to %d.\n",
>+			bond->dev->name,
>+			bond->params.downdelay * bond->params.miimon);
>+
>+	}
>+
>+	return 0;
>+}
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 1125bef..41a62ac 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -706,42 +706,22 @@ static ssize_t bonding_store_downdelay(struct device *d,
> 				       struct device_attribute *attr,
> 				       const char *buf, size_t count)
> {
>-	int new_value, ret = count;
>+	int new_value, ret;
> 	struct bonding *bond = to_bond(d);
>
>-	if (!(bond->params.miimon)) {
>-		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
>-		       bond->dev->name);
>-		ret = -EPERM;
>-		goto out;
>-	}
>-
> 	if (sscanf(buf, "%d", &new_value) != 1) {
> 		pr_err("%s: no down delay value specified.\n", bond->dev->name);
>-		ret = -EINVAL;
>-		goto out;
>+		return -EINVAL;
> 	}
>-	if (new_value < 0) {
>-		pr_err("%s: Invalid down delay value %d not in range %d-%d; rejected.\n",
>-		       bond->dev->name, new_value, 0, INT_MAX);
>-		ret = -EINVAL;
>-		goto out;
>-	} else {
>-		if ((new_value % bond->params.miimon) != 0) {
>-			pr_warning("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
>-				   bond->dev->name, new_value,
>-				   bond->params.miimon,
>-				   (new_value / bond->params.miimon) *
>-				   bond->params.miimon);
>-		}
>-		bond->params.downdelay = new_value / bond->params.miimon;
>-		pr_info("%s: Setting down delay to %d.\n",
>-			bond->dev->name,
>-			bond->params.downdelay * bond->params.miimon);
>
>-	}
>+	if (!rtnl_trylock())
>+		return restart_syscall();
>
>-out:
>+	ret = bond_option_downdelay_set(bond, new_value);
>+	if (!ret)
>+		ret = count;
>+
>+	rtnl_unlock();
> 	return ret;
> }
> static DEVICE_ATTR(downdelay, S_IRUGO | S_IWUSR,
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 408e6c2..40987f3 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -430,6 +430,7 @@ int bond_option_mode_set(struct bonding *bond, int mode);
> int bond_option_active_slave_set(struct bonding *bond, struct net_device *slave_dev);
> int bond_option_miimon_set(struct bonding *bond, int miimon);
> int bond_option_updelay_set(struct bonding *bond, int updelay);
>+int bond_option_downdelay_set(struct bonding *bond, int downdelay);
> struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
> struct net_device *bond_option_active_slave_get(struct bonding *bond);
>
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index 361414e..a372831 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -333,6 +333,7 @@ enum {
> 	IFLA_BOND_ACTIVE_SLAVE,
> 	IFLA_BOND_MIIMON,
> 	IFLA_BOND_UPDELAY,
>+	IFLA_BOND_DOWNDELAY,
> 	__IFLA_BOND_MAX,
> };
>
>

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

Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Scott Feldman]

On Nov 7, 2013, at 7:05 AM, Jay Vosburgh <fubar@us.ibm.com> wrote:

> Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
> 
>> +int bond_option_downdelay_set(struct bonding *bond, int downdelay)
>> +{
>> +	if (!(bond->params.miimon)) {
>> +		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);
>> +		return -EPERM;
>> +	}
> 
> 	I would argue that it is better to permit this option to be set
> at any time, regardless of whether miimon is enabled or not.  This
> removes an ordering constraint and makes things generally easier to deal
> with.  Setting the option has no effect if miimon is not set, but that's
> ok.
> 
> 	My comment here would also apply to other options that are
> "secondary," in the sense that they affect the behavior of other options
> or modes, e.g., updelay, arp_validate (in another path of this series),
> arp_ip_targets, etc.
> 
> 	I'm aware that this is simply duplicating the existing behavior
> of the sysfs API, but I'd be in favor of changing that, too.  These
> limitations are a holdover from the module paramters days, and should
> have been made more flexible a long time ago.

What I’d like to propose, and I hope that you’ll agree, is we tackle this in two phases.  The first phase is to finish the current duplication effort to enable netlink equivalents of the attributes in sysfs.  This is a fairly mechanical process, preserving existing behavior as you point out, and keeps the patches single-minded and easy to review.

The second phase is to revisit ordering constraints and find places where we can remove constraints or streamline the dependency checks.

Sound like a plan?

-scott

Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Jay Vosburgh]

Scott Feldman <sfeldma@cumulusnetworks.com> wrote:

>
>On Nov 7, 2013, at 7:05 AM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>
>> Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
>> 
>>> +int bond_option_downdelay_set(struct bonding *bond, int downdelay)
>>> +{
>>> +	if (!(bond->params.miimon)) {
>>> +		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);
>>> +		return -EPERM;
>>> +	}
>> 
>> 	I would argue that it is better to permit this option to be set
>> at any time, regardless of whether miimon is enabled or not.  This
>> removes an ordering constraint and makes things generally easier to deal
>> with.  Setting the option has no effect if miimon is not set, but that's
>> ok.
>> 
>> 	My comment here would also apply to other options that are
>> "secondary," in the sense that they affect the behavior of other options
>> or modes, e.g., updelay, arp_validate (in another path of this series),
>> arp_ip_targets, etc.
>> 
>> 	I'm aware that this is simply duplicating the existing behavior
>> of the sysfs API, but I'd be in favor of changing that, too.  These
>> limitations are a holdover from the module paramters days, and should
>> have been made more flexible a long time ago.
>
>What I’d like to propose, and I hope that you’ll agree, is we tackle
>this in two phases.  The first phase is to finish the current
>duplication effort to enable netlink equivalents of the attributes in
>sysfs.  This is a fairly mechanical process, preserving existing
>behavior as you point out, and keeps the patches single-minded and easy
>to review.
>
>The second phase is to revisit ordering constraints and find places
>where we can remove constraints or streamline the dependency checks.
>
>Sound like a plan?

	Well, maybe.

	What I want to avoid for the iproute / netlink bonding support
is all of the hoops that the initscripts / sysconfig scripts had to jump
through to obey the current sysfs ordering limtations.

	The primary user of this is going to be iproute.  Will the above
quoted limitations (and their equivalents for various other options)
make any difference with regards to the following:

ip [...] bond arp_validate all arp_interval 1000
ip [...] bond arp_interval 1000 arp_validate all

	I.e., does the ordering matter at the iproute level when
multiple options are specified simultaneously?

	What happens if I do:

ip [...] bond arp_interval 1000 arp_validate all miimon 100

	This is separate from simply changing one thing at a time, e.g.,

ip [...] bond arp_validate all
ip [...] bond arp_interval 1000

	will presuambly hit the test above, and this is where the
ordering stuff comes into play for sure.

	Also, as I look at the iproute patch, it doesn't appear to
accept the text names for the options, only numeric values (e.g., "ip
[...]  bond arp_validate 3").  That appears to be a limitation of Jiri's
original iproute patch as well.  Am I the only person that perfers the
text labels (e.g., arp_validate as "all") to numbers (arp_validate as
"3")?

	That number-only limitation also may make life for initscripts /
sysconfig more difficult, or at least the upgrade path more of a hassle,
since currently the ifcfg-bond* files can have the bonding options
specified as text labels in addition to the numeric values.

	So, honestly, I think if the ordering constraints are going to
be relaxed, it should happen sooner rather than later.  Perhaps not in
the same patch as the netlink support, but ideally at least in the same
series, so there is no real release with the constraints.  Changing the
constraints after the script, etc, conversion is done doesn't really
help much.

	-J

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

Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Scott Feldman]

On Nov 8, 2013, at 11:15 AM, Jay Vosburgh <fubar@us.ibm.com> wrote:

> Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
> 
>> What I’d like to propose, and I hope that you’ll agree, is we tackle
>> this in two phases.  The first phase is to finish the current
>> duplication effort to enable netlink equivalents of the attributes in
>> sysfs.  This is a fairly mechanical process, preserving existing
>> behavior as you point out, and keeps the patches single-minded and easy
>> to review.
>> 
>> The second phase is to revisit ordering constraints and find places
>> where we can remove constraints or streamline the dependency checks.
>> 
>> Sound like a plan?
> 
> 	Well, maybe.
> 
> 	What I want to avoid for the iproute / netlink bonding support
> is all of the hoops that the initscripts / sysconfig scripts had to jump
> through to obey the current sysfs ordering limtations.
> 
> 	The primary user of this is going to be iproute.  Will the above
> quoted limitations (and their equivalents for various other options)
> make any difference with regards to the following:
> 
> ip [...] bond arp_validate all arp_interval 1000
> ip [...] bond arp_interval 1000 arp_validate all
> 
> 	I.e., does the ordering matter at the iproute level when
> multiple options are specified simultaneously?

Surprisingly, order doesn’t matter.  Regardless of the ordering of attrs within the netlink msg, the processing order of attrs is always the same.  In this case of bonding, it’s bond_netlink.c:bond_changelink() that dictates the order attrs are processed.  So in your examples above, both versions would yield the same results.

> 	What happens if I do:
> 
> ip [...] bond arp_interval 1000 arp_validate all miimon 100
> 
> 	This is separate from simply changing one thing at a time, e.g.,
> 
> ip [...] bond arp_validate all
> ip [...] bond arp_interval 1000

This is the problem to be solved.  Bonding has several mutually exclusive attrs.  When mutually exclusive options are presented at the same time (like in you first example above), we can pick a winner using the processing order in bond_changelink().  When single attrs are set (like in second example), it’s trickier to pick winner in bond_changelink(), but do-able, I think.

> 	will presuambly hit the test above, and this is where the
> ordering stuff comes into play for sure.
> 
> 	Also, as I look at the iproute patch, it doesn't appear to
> accept the text names for the options, only numeric values (e.g., "ip
> [...]  bond arp_validate 3").  That appears to be a limitation of Jiri's
> original iproute patch as well.  Am I the only person that perfers the
> text labels (e.g., arp_validate as "all") to numbers (arp_validate as
> "3")?

I can add text names to the iproute patch for v2.

> 	So, honestly, I think if the ordering constraints are going to
> be relaxed, it should happen sooner rather than later.  Perhaps not in
> the same patch as the netlink support, but ideally at least in the same
> series, so there is no real release with the constraints.  Changing the
> constraints after the script, etc, conversion is done doesn't really
> help much.

Ok, let me study this some more before sending v2.

-scott