Re: [PATCH net v3 1/5] bonding: 3ad: add lacp_fallback configuration knob

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>When an 802.3ad (LACP) bonding interface has no slaves in the
>collecting/distributing state, the bonding master still reports
>carrier as up as long as at least 'min_links' slaves have carrier.
>
>In this situation, only one slave is effectively used for TX/RX,
>while traffic received on other slaves is dropped. Upper-layer
>daemons therefore consider the interface operational, even though
>traffic may be blackholed if the lack of LACP negotiation means
>the partner is not ready to deal with traffic.
>
>Introduce a configuration knob to control this behavior. It allows
>the bonding master to assert carrier only when at least 'min_links'
>slaves are in collecting/distributing state (or collecting only
>when coupled_control is disabled).
>
>The default mode preserves the current (legacy) behavior. This
>patch only introduces the knob; its behavior is implemented in
>the subsequent commit.
>
>Fixes: 655f8919d549 ("bonding: add min links parameter to 802.3ad")
>Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
>---
> Documentation/networking/bonding.rst | 33 ++++++++++++++++++++++++++++
> drivers/net/bonding/bond_main.c      |  1 +
> drivers/net/bonding/bond_netlink.c   | 16 ++++++++++++++
> drivers/net/bonding/bond_options.c   | 26 ++++++++++++++++++++++
> include/net/bond_options.h           |  1 +
> include/net/bonding.h                |  1 +
> include/uapi/linux/if_link.h         |  1 +
> 7 files changed, 79 insertions(+)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index e700bf1d095c..465d06aead27 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -619,6 +619,39 @@ min_links
> 	aggregator cannot be active without at least one available link,
> 	setting this option to 0 or to 1 has the exact same effect.
> 
>+lacp_fallback
>+
>+	Specifies the fallback behavior of a bonding when LACP negotiation fails on
>+	all slave links, i.e. when no slave is in the Collecting/Distributing state
>+	(or only in Collecting state when coupled_control is disabled), while at
>+	least `min_links` link still reports carrier up.
>+
>+	This option is only applicable to 802.3ad mode (mode 4).
>+
>+	Valid values are:
>+
>+	legacy or 0
>+		In this situation, the bonding master remains carrier up and
>+		randomly selects a single slave to transmit and receive traffic.
>+		Traffic received on other slaves is dropped.
>+
>+		This mode is deprecated, as it may lead to traffic blackholing
>+		when the absence of LACP negotiation means the partner is not
>+		ready to collect and distribute traffic.
>+
>+		This is the legacy default behavior.
>+
>+	strict or 1
>+		In this situation, the bonding master reports carrier down, allowing
>+		upper-layer processes to detect that the interface is not usable for
>+		collecting and distributing traffic.
>+
>+		The master transitions to carrier up only when at least
>+		`min_links` slaves reach the Collecting(/Distributing) state,
>+		allowing traffic to flow.
>+
>+	The default value is 0 (legacy).
>+

	1- Please wrap text at approximately 75 columns.

	2- I don't agree with the nomenclature or language of the above.
The existing behavior is not going to be deprecated or considered to be
legacy, and the option nomenclature should reflect that.  I would
suggest naming the option "lacp_strict" and having it's possible
settings be on or off, with the default setting as off.

	I think the behavior can be described along the lines of

	off or 0
		One interface of the bond is selected to be active, in
		order to facilitate communication with peer devices that
		do not implement LACP.

	on or 1
		Interfaces are only permitted to be made active if they
		have an active LACP partner and have successfully reached
		Collecting or Collecting_Distributing state.

	The default is value is 0 (off).

	-J


> mode
> 
> 	Specifies one of the bonding policies. The default is
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index a5484d11553d..02cba0560a39 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -6440,6 +6440,7 @@ static int __init bond_check_params(struct bond_params *params)
> 	params->ad_user_port_key = ad_user_port_key;
> 	params->coupled_control = 1;
> 	params->broadcast_neighbor = 0;
>+	params->lacp_fallback = 0;
> 	if (packets_per_slave > 0) {
> 		params->reciprocal_packets_per_slave =
> 			reciprocal_value(packets_per_slave);
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 286f11c517f7..1f92ad786b51 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -130,6 +130,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> 	[IFLA_BOND_NS_IP6_TARGET]	= { .type = NLA_NESTED },
> 	[IFLA_BOND_COUPLED_CONTROL]	= { .type = NLA_U8 },
> 	[IFLA_BOND_BROADCAST_NEIGH]	= { .type = NLA_U8 },
>+	[IFLA_BOND_LACP_FALLBACK]	= { .type = NLA_U8 },
> };
> 
> static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
>@@ -586,6 +587,16 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
> 			return err;
> 	}
> 
>+	if (data[IFLA_BOND_LACP_FALLBACK]) {
>+		int fallback_mode = nla_get_u8(data[IFLA_BOND_LACP_FALLBACK]);
>+
>+		bond_opt_initval(&newval, fallback_mode);
>+		err = __bond_opt_set(bond, BOND_OPT_LACP_FALLBACK, &newval,
>+				     data[IFLA_BOND_LACP_FALLBACK], extack);
>+		if (err)
>+			return err;
>+	}
>+
> 	return 0;
> }
> 
>@@ -658,6 +669,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> 		nla_total_size(sizeof(struct in6_addr)) * BOND_MAX_NS_TARGETS +
> 		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_COUPLED_CONTROL */
> 		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_BROADCAST_NEIGH */
>+		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_LACP_FALLBACK */
> 		0;
> }
> 
>@@ -825,6 +837,10 @@ static int bond_fill_info(struct sk_buff *skb,
> 		       bond->params.broadcast_neighbor))
> 		goto nla_put_failure;
> 
>+	if (nla_put_u8(skb, IFLA_BOND_LACP_FALLBACK,
>+		       bond->params.lacp_fallback))
>+		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 7380cc4ee75a..b672b8a881bb 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -68,6 +68,8 @@ static int bond_option_lacp_active_set(struct bonding *bond,
> 				       const struct bond_opt_value *newval);
> static int bond_option_lacp_rate_set(struct bonding *bond,
> 				     const struct bond_opt_value *newval);
>+static int bond_option_lacp_fallback_set(struct bonding *bond,
>+					 const struct bond_opt_value *newval);
> static int bond_option_ad_select_set(struct bonding *bond,
> 				     const struct bond_opt_value *newval);
> static int bond_option_queue_id_set(struct bonding *bond,
>@@ -162,6 +164,12 @@ static const struct bond_opt_value bond_lacp_rate_tbl[] = {
> 	{ NULL,   -1,           0},
> };
> 
>+static const struct bond_opt_value bond_lacp_fallback_tbl[] = {
>+	{ "legacy", 0, BOND_VALFLAG_DEFAULT},
>+	{ "strict",  1, 0},
>+	{ NULL, -1, 0 }
>+};
>+
> static const struct bond_opt_value bond_ad_select_tbl[] = {
> 	{ "stable",          BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
> 	{ "bandwidth",       BOND_AD_BANDWIDTH, 0},
>@@ -363,6 +371,14 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
> 		.values = bond_lacp_rate_tbl,
> 		.set = bond_option_lacp_rate_set
> 	},
>+	[BOND_OPT_LACP_FALLBACK] = {
>+		.id = BOND_OPT_LACP_FALLBACK,
>+		.name = "lacp_fallback",
>+		.desc = "Define the LACP fallback mode when no slaves have negotiated",
>+		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_8023AD)),
>+		.values = bond_lacp_fallback_tbl,
>+		.set = bond_option_lacp_fallback_set
>+	},
> 	[BOND_OPT_MINLINKS] = {
> 		.id = BOND_OPT_MINLINKS,
> 		.name = "min_links",
>@@ -1684,6 +1700,16 @@ static int bond_option_lacp_rate_set(struct bonding *bond,
> 	return 0;
> }
> 
>+static int bond_option_lacp_fallback_set(struct bonding *bond,
>+					 const struct bond_opt_value *newval)
>+{
>+	netdev_dbg(bond->dev, "Setting LACP fallback to %s (%llu)\n",
>+		   newval->string, newval->value);
>+	bond->params.lacp_fallback = newval->value;
>+
>+	return 0;
>+}
>+
> static int bond_option_ad_select_set(struct bonding *bond,
> 				     const struct bond_opt_value *newval)
> {
>diff --git a/include/net/bond_options.h b/include/net/bond_options.h
>index e6eedf23aea1..5eb64c831f54 100644
>--- a/include/net/bond_options.h
>+++ b/include/net/bond_options.h
>@@ -79,6 +79,7 @@ enum {
> 	BOND_OPT_COUPLED_CONTROL,
> 	BOND_OPT_BROADCAST_NEIGH,
> 	BOND_OPT_ACTOR_PORT_PRIO,
>+	BOND_OPT_LACP_FALLBACK,
> 	BOND_OPT_LAST
> };
> 
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 395c6e281c5f..d8cb02643f8b 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -132,6 +132,7 @@ struct bond_params {
> 	int peer_notif_delay;
> 	int lacp_active;
> 	int lacp_fast;
>+	int lacp_fallback;
> 	unsigned int min_links;
> 	int ad_select;
> 	char primary[IFNAMSIZ];
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index e9b5f79e1ee1..7ad3fc600c71 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -1539,6 +1539,7 @@ enum {
> 	IFLA_BOND_NS_IP6_TARGET,
> 	IFLA_BOND_COUPLED_CONTROL,
> 	IFLA_BOND_BROADCAST_NEIGH,
>+	IFLA_BOND_LACP_FALLBACK,
> 	__IFLA_BOND_MAX,
> };
> 
>-- 
>2.39.2
>

---
	-Jay Vosburgh, jv@jvosburgh.net