bonding: lacp_port_id setting for 802.3ad

PATCH In review Krisztian Ivancso 3 participants 9 messages Aug 12, 2013 – Aug 16, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Krisztian Ivancso

author

2013-08-12 11:49 UTC #0 lore raw

PATCH 1/1 bonding: lacp_port_id setting for 802.3ad

The patch adds port id setting feature by lacp_port_id module parameter for 802.3ad ports.

Adding more linux boxes to a link aggregation makes possible to do load-balancing by using port channel load balancing of switch (eg. by source-dest-ip or src-ip).

It's essential to set the same MAC address to bonding interface and to set different port ids on different linux boxes for a working solution.

Possible use cases:
1. Redundant DNS servers with same IP address
2. Reverse proxies with same IP address
3. "multi-master" LVS

Beside redundancy it provides scalability, scalability depends on hardware capability.
E.g. a LAG port with 4 members - linux LVS servers - using a Cisco switch/router splits traffic to ports equally by src IP (2-2-2-2). If a link fails, switch forwards traffic to 3 switch ports (3-3-3). It means you can utilize overall bandwith as much as 66% with full redundancy. It's better with 16% than using 2 master-slave LVS and you use just 1 IP and can handle 100% more peak traffic.

Using this feature with vPC (virtual portchannel) or similar solution it provides a physically redundant service from network devices to servers using the same IP address.

Quoted 1 line

From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001

From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports

By setting this parameter to different values on different hosts it's possible to add more Linux boxes to the same Link Aggregation.

Signed-off-by Krisztian Ivancso <[email protected]>

---
 drivers/net/bonding/bond_main.c   | 18 ++++++++++++++++--
 drivers/net/bonding/bond_procfs.c |  1 +
 drivers/net/bonding/bond_sysfs.c  | 33 +++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h     |  1 +
 4 files changed, 51 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +16 -2

index 4264a76..452f4d4 100644
@@ -110,6 +110,7 @@ static char *fail_over_mac;
 static int all_slaves_active;
 static struct bond_params bonding_defaults;
 static int resend_igmp = BOND_DEFAULT_RESEND_IGMP;
+static int lacp_port_id = 1;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");

@@ -150,7 +151,7 @@ module_param(lacp_rate, charp, 0);
 MODULE_PARM_DESC(lacp_rate, "LACPDU tx rate to request from 802.3ad partner; "
 			    "0 for slow, 1 for fast");
 module_param(ad_select, charp, 0);
-MODULE_PARM_DESC(ad_select, "803.ad aggregation selection logic; "
+MODULE_PARM_DESC(ad_select, "802.3ad aggregation selection logic; "
 			    "0 for stable (default), 1 for bandwidth, "
 			    "2 for count");
 module_param(min_links, int, 0);

@@ -181,6 +182,8 @@ MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface"
 module_param(resend_igmp, int, 0);
 MODULE_PARM_DESC(resend_igmp, "Number of IGMP membership reports to send on "
 			      "link failure");
+module_param(lacp_port_id, int, 0);
+MODULE_PARM_DESC(lacp_port_id, "802.3ad port id to send to switch in LACPDU");
 
 /*----------------------------- Global variables ----------------------------*/

@@ -1699,7 +1702,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		bond_set_slave_inactive_flags(new_slave);
 		/* if this is the first slave */
 		if (bond_first_slave(bond) == new_slave) {
-			SLAVE_AD_INFO(new_slave).id = 1;
+			SLAVE_AD_INFO(new_slave).id = lacp_port_id;
 			/* Initialize AD with the number of times that the AD timer is called in 1 second
 			 * can be called only after the mac address of the bond is set
 			 */

@@ -4377,6 +4380,16 @@ static int bond_check_params(struct bond_params *params)
 		resend_igmp = BOND_DEFAULT_RESEND_IGMP;
 	}
 
+	if (bond_mode == BOND_MODE_8023AD) {
+		/* we set upper limit to 65000 because new slaves will increase port
+                   id so we don't want id to overflow */
+		if (lacp_port_id < 1 || lacp_port_id > 65000) {
+			pr_warning("Warning: lacp_port_id (%d) should be between "
+				   "1 and 65000, resetting to 1\n", lacp_port_id);
+			lacp_port_id = 1;
+		}
+	}
+
 	/* reset values for TLB/ALB */
 	if ((bond_mode == BOND_MODE_TLB) ||
 	    (bond_mode == BOND_MODE_ALB)) {

@@ -4565,6 +4578,7 @@ static int bond_check_params(struct bond_params *params)
 	params->all_slaves_active = all_slaves_active;
 	params->resend_igmp = resend_igmp;
 	params->min_links = min_links;
+	params->lacp_port_id = lacp_port_id;
 
 	if (primary) {
 		strncpy(params->primary, primary, IFNAMSIZ);

drivers/net/bonding/bond_procfs.c +1 -0

index 20a6ee2..6d52716 100644
@@ -129,6 +129,7 @@ static void bond_info_show_master(struct seq_file *seq)
 		seq_printf(seq, "Min links: %d\n", bond->params.min_links);
 		seq_printf(seq, "Aggregator selection policy (ad_select): %s\n",
 			   ad_select_tbl[bond->params.ad_select].modename);
+		seq_printf(seq, "802.3ad starting port id: %d\n", bond->params.lacp_port_id);
 
 		if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 			seq_printf(seq, "bond %s has no active aggregator\n",

drivers/net/bonding/bond_sysfs.c +33 -0

index 0f539de..a79b7c3 100644
@@ -920,6 +920,38 @@ static ssize_t bonding_store_min_links(struct device *d,
 static DEVICE_ATTR(min_links, S_IRUGO | S_IWUSR,
 		   bonding_show_min_links, bonding_store_min_links);
 
+static ssize_t bonding_show_lacp_port_id(struct device *d,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct bonding *bond = to_bond(d);
+
+	return sprintf(buf, "%d\n", bond->params.lacp_port_id);
+}
+
+static ssize_t bonding_store_lacp_port_id(struct device *d,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct bonding *bond = to_bond(d);
+	int ret;
+	unsigned int new_value;
+
+	ret = kstrtouint(buf, 0, &new_value);
+	if ((ret < 0) || (new_value < 1) || (new_value > 65000)) {
+		pr_err("%s: Ignoring invalid 802.3ad port id value %s.\n",
+		       bond->dev->name, buf);
+		return -EINVAL;
+	}
+
+	pr_info("%s: Setting 802.3ad port id value to %u\n",
+		bond->dev->name, new_value);
+	bond->params.lacp_port_id = new_value;
+	return count;
+}
+static DEVICE_ATTR(lacp_port_id, S_IRUGO | S_IWUSR,
+		   bonding_show_lacp_port_id, bonding_store_lacp_port_id);
+
 static ssize_t bonding_show_ad_select(struct device *d,
 				      struct device_attribute *attr,
 				      char *buf)

@@ -1705,6 +1737,7 @@ static struct attribute *per_bond_attrs[] = {
 	&dev_attr_all_slaves_active.attr,
 	&dev_attr_resend_igmp.attr,
 	&dev_attr_min_links.attr,
+	&dev_attr_lacp_port_id.attr,
 	NULL,
 };

drivers/net/bonding/bonding.h +1 -0

index 4bf52d5..8a078d6 100644
@@ -176,6 +176,7 @@ struct bond_params {
 	int tx_queues;
 	int all_slaves_active;
 	int resend_igmp;
+	int lacp_port_id;
 };
 
 struct bond_parm_tbl {

Signature · 1 line

1.8.3.1

Ding Tianhong

2013-08-13 01:08 UTC #1 lore raw

Quoted 30 lines · Krisztian Ivancso

The patch adds port id setting feature by lacp_port_id module parameter for 802.3ad ports.
Adding more linux boxes to a link aggregation makes possible to do load-balancing by using port channel load balancing of switch (eg. by source-dest-ip or src-ip).
It's essential to set the same MAC address to bonding interface and to set different port ids on different linux boxes for a working solution.
Possible use cases:
1. Redundant DNS servers with same IP address
2. Reverse proxies with same IP address
3. "multi-master" LVS
Beside redundancy it provides scalability, scalability depends on hardware capability.
E.g. a LAG port with 4 members - linux LVS servers - using a Cisco switch/router splits traffic to ports equally by src IP (2-2-2-2). If a link fails, switch forwards traffic to 3 switch ports (3-3-3). It means you can utilize overall bandwith as much as 66% with full redundancy. It's better with 16% than using 2 master-slave LVS and you use just 1 IP and can handle 100% more peak traffic.
Using this feature with vPC (virtual portchannel) or similar solution it provides a physically redundant service from network devices to servers using the same IP address.

Quoted 8 lines

Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
By setting this parameter to different values on different hosts it's possible to add more Linux boxes to the same Link Aggregation.

I think the port id was made by negotiation, if the negotiation failed, the bonding slave could not get the port id you gave, which lie on the hardware message.

Krisztian Ivancso

author

2013-08-13 09:20 UTC #2 lore raw

Quoted 6 lines · Ding Tianhong

Quoted 5 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports

Quoted 6 lines

Quoted 3 lines
By setting this parameter to different values on different hosts it's possible to add more Linux boxes to the same Link Aggregation.
I think the port id was made by negotiation, if the negotiation failed, the bonding slave could not get the port id you gave, which lie on the hardware message.

Port id is currently preset to 1 in bonding driver for first slave. (SLAVE_AD_INFO(new_slave).id = 1; this id is used as actor_port_number in slave initialization)
Port id parameter modifies just this starting value and new slaves get incremented port ids as in current implementation.

As I see in documentation port id (port number) set by each participating devices by it's own and given values are used by the other side.

In Cisco and Juniper documentations port id is mentioned, the value is derived from port number which is unique within a switch.

Cisco:

Quoted 2 lines

LACP uses the port priority with the port number to form the port identifier.

Juniper:

Quoted 3 lines

The LACP port ID consists of the port priority as the two most-significant octets and the port number as the two least-significant octets.

Cisco:

Quoted 2 lines

The LACP system ID is the combination of the LACP system priority value and the MAC address.

This is why we need to use the same MAC for all participating bonding device in different linux boxes.

The solution is tested with Cisco devices.

Ding Tianhong

2013-08-13 09:39 UTC #3 lore raw

Quoted 45 lines · Krisztian Ivancso

Quoted 6 lines · Ding Tianhong
Quoted 5 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
Quoted 6 lines
Quoted 3 lines
By setting this parameter to different values on different hosts it's possible to add more Linux boxes to the same Link Aggregation.
I think the port id was made by negotiation, if the negotiation failed, the bonding slave could not get the port id you gave, which lie on the hardware message.
Port id is currently preset to 1 in bonding driver for first slave. (SLAVE_AD_INFO(new_slave).id = 1; this id is used as actor_port_number in slave initialization)
Port id parameter modifies just this starting value and new slaves get incremented port ids as in current implementation.
As I see in documentation port id (port number) set by each participating devices by it's own and given values are used by the other side.
In Cisco and Juniper documentations port id is mentioned, the value is derived from port number which is unique within a switch.
Cisco:
Quoted 2 lines
LACP uses the port priority with the port number to form the port identifier.
Juniper:
Quoted 3 lines
The LACP port ID consists of the port priority as the two most-significant octets and the port number as the two least-significant octets.
Cisco:
Quoted 2 lines
The LACP system ID is the combination of the LACP system priority value and the MAC address.
This is why we need to use the same MAC for all participating bonding device in different linux boxes.

Quoted 8 lines

The solution is tested with Cisco devices.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.

--Ding Tianhong

Krisztian Ivancso

author

2013-08-13 11:00 UTC #4 lore raw

Quoted 10 lines · Ding Tianhong

Quoted 6 lines · Krisztian Ivancso
Quoted 5 lines · Ding Tianhong
Quoted 4 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.

this is not possible, because all slave have to be a member of the same aggregation group.

i think we misunderstood each other.

here is a new example:
- switch1 is a switch with a configured lag with two members ports (member1 and member2)
- two linux (linux1 and linux2) box with a configured bonding device (bond0) with the same MAC set in both box and one
slave on each
- lacp_port_id is set to 10 in linux1 and 20 in linux2

you can attach the slave from both linux boxes to the same lag on switch1. (slave from linux1 to port member1 and slave from linux2 to port member2 on switch1)

port id must be unique within a system.
bonding implementation set a unique system id for every bonding device which is derived from MAC of one of the slave interfaces.

if we use the current bonding implementation second linux box can't be a member on switch1 because port id is 1 in both linux bonding device.

if we can set different starting port id for bonding in different boxes the second box can be a member also.

Jay Vosburgh

2013-08-13 18:25 UTC #5 lore raw

Quoted 9 lines · Krisztian Ivancso

Quoted 8 lines · Ding Tianhong
Quoted 6 lines · Krisztian Ivancso
Quoted 5 lines · Ding Tianhong
Quoted 4 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports

Quoted 5 lines

Quoted 2 lines
ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.
this is not possible, because all slave have to be a member of the same aggregation group.

Just on the above point, bonding can group slaves into multiple
aggregators, but only one aggregator will be active at any given time.

To answer the question, the four slaves would each be given
unique port IDs that do not conflict.

Quoted 18 lines

i think we misunderstood each other.
here is a new example:
- switch1 is a switch with a configured lag with two members ports (member1 and member2)
- two linux (linux1 and linux2) box with a configured bonding device (bond0) with the same MAC set in both box and one
slave on each
- lacp_port_id is set to 10 in linux1 and 20 in linux2
you can attach the slave from both linux boxes to the same lag on switch1. (slave from linux1 to port member1 and slave from linux2 to port member2 on switch1)
port id must be unique within a system.
bonding implementation set a unique system id for every bonding device which is derived from MAC of one of the slave interfaces.

Quoted 5 lines

if we use the current bonding implementation second linux box can't be a member on switch1 because port id is 1 in both linux bonding device.
if we can set different starting port id for bonding in different boxes the second box can be a member also.

I understand what you're trying to do here (permit multiple
instances of bonding on different systems to connect to a single aggregator on a switch), and I don't really have a problem with it in general.

I do have some comments:

First, altering the lacp_port_id (via sysfs) should only be
permitted when there are no slaves in the bond, otherwise the /proc/net/bonding/bond0 output for the first port id will not match the actual first port id value assigned to the slaves. As a practical matter, altering lacp_port_id while slaves are present in the bond has no effect until all slaves are released and the first new slave is added, so this is not reducing functionality.

Second, the lacp_port_id is global across all bonds created
within the loaded module, and so multiple bonds will all use the same starting value. Setting the lacp_port_id via sysfs has no effect, as it alters a per-bond value, bond->params.lacp_port_id, that is never actually used to set the port ID of a first slave in bond_enslave.

The global default value should only be used to initialize the
per-bond value when a bond is created, and that per-bond value should be used when setting the port id in bond_enslave(). The per-bond value is already displayed in /proc/net/bonding/bond0, and is the value modified by the sysfs functions

Third, consider adding the port ID to the 803.2ad section in
bond_info_show_slave.

Lastly, I think this should be tested against systems other than
Cisco to insure that it really interoperates with, for example, Juniper's methodology for spanning an aggregator across physical chassis. I'm not sure why it wouldn't, but once new functionality becomes part of the kernel, changing it in non-backwards compatible ways is difficult.

-J

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

Krisztian Ivancso

author

2013-08-13 23:34 UTC #6 lore raw

Quoted 68 lines · Jay Vosburgh

Quoted 9 lines · Krisztian Ivancso
Quoted 8 lines · Ding Tianhong
Quoted 6 lines · Krisztian Ivancso
Quoted 5 lines · Ding Tianhong
Quoted 4 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
Quoted 5 lines
Quoted 2 lines
ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.
this is not possible, because all slave have to be a member of the same aggregation group.
Just on the above point, bonding can group slaves into multiple
aggregators, but only one aggregator will be active at any given time.
To answer the question, the four slaves would each be given
unique port IDs that do not conflict.
Quoted 18 lines
i think we misunderstood each other.
here is a new example:
- switch1 is a switch with a configured lag with two members ports (member1 and member2)
- two linux (linux1 and linux2) box with a configured bonding device (bond0) with the same MAC set in both box and one
slave on each
- lacp_port_id is set to 10 in linux1 and 20 in linux2
you can attach the slave from both linux boxes to the same lag on switch1. (slave from linux1 to port member1 and slave from linux2 to port member2 on switch1)
port id must be unique within a system.
bonding implementation set a unique system id for every bonding device which is derived from MAC of one of the slave interfaces.
Quoted 5 lines
if we use the current bonding implementation second linux box can't be a member on switch1 because port id is 1 in both linux bonding device.
if we can set different starting port id for bonding in different boxes the second box can be a member also.
I understand what you're trying to do here (permit multiple
instances of bonding on different systems to connect to a single aggregator on a switch), and I don't really have a problem with it in general.
I do have some comments:
First, altering the lacp_port_id (via sysfs) should only be
permitted when there are no slaves in the bond, otherwise the /proc/net/bonding/bond0 output for the first port id will not match the actual first port id value assigned to the slaves. As a practical matter, altering lacp_port_id while slaves are present in the bond has no effect until all slaves are released and the first new slave is added, so this is not reducing functionality.
Second, the lacp_port_id is global across all bonds created
within the loaded module, and so multiple bonds will all use the same starting value. Setting the lacp_port_id via sysfs has no effect, as it alters a per-bond value, bond->params.lacp_port_id, that is never actually used to set the port ID of a first slave in bond_enslave.

Quoted 8 lines

The global default value should only be used to initialize the
per-bond value when a bond is created, and that per-bond value should be used when setting the port id in bond_enslave(). The per-bond value is already displayed in /proc/net/bonding/bond0, and is the value modified by the sysfs functions
Third, consider adding the port ID to the 803.2ad section in
bond_info_show_slave.

Thanks for these great comments. I'll soon fix sysfs related bug and rework patch.

Quoted 7 lines

Lastly, I think this should be tested against systems other than
Cisco to insure that it really interoperates with, for example, Juniper's methodology for spanning an aggregator across physical chassis. I'm not sure why it wouldn't, but once new functionality becomes part of the kernel, changing it in non-backwards compatible ways is difficult.

I agree. I try to test it with devices from other manufacturers.

Ding Tianhong

2013-08-14 01:47 UTC #7 lore raw

Quoted 75 lines · Krisztian Ivancso

Quoted 74 lines · Jay Vosburgh
Quoted 9 lines · Krisztian Ivancso
Quoted 8 lines · Ding Tianhong
Quoted 6 lines · Krisztian Ivancso
Quoted 5 lines · Ding Tianhong
Quoted 4 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
Quoted 5 lines
Quoted 2 lines
ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.
this is not possible, because all slave have to be a member of the same aggregation group.
Just on the above point, bonding can group slaves into multiple
aggregators, but only one aggregator will be active at any given time.
To answer the question, the four slaves would each be given
unique port IDs that do not conflict.
Quoted 18 lines
i think we misunderstood each other.
here is a new example:
- switch1 is a switch with a configured lag with two members ports (member1 and member2)
- two linux (linux1 and linux2) box with a configured bonding device (bond0) with the same MAC set in both box and one
slave on each
- lacp_port_id is set to 10 in linux1 and 20 in linux2
you can attach the slave from both linux boxes to the same lag on switch1. (slave from linux1 to port member1 and slave from linux2 to port member2 on switch1)
port id must be unique within a system.
bonding implementation set a unique system id for every bonding device which is derived from MAC of one of the slave interfaces.
Quoted 5 lines
if we use the current bonding implementation second linux box can't be a member on switch1 because port id is 1 in both linux bonding device.
if we can set different starting port id for bonding in different boxes the second box can be a member also.
I understand what you're trying to do here (permit multiple
instances of bonding on different systems to connect to a single aggregator on a switch), and I don't really have a problem with it in general.
I do have some comments:
First, altering the lacp_port_id (via sysfs) should only be
permitted when there are no slaves in the bond, otherwise the /proc/net/bonding/bond0 output for the first port id will not match the actual first port id value assigned to the slaves. As a practical matter, altering lacp_port_id while slaves are present in the bond has no effect until all slaves are released and the first new slave is added, so this is not reducing functionality.
Second, the lacp_port_id is global across all bonds created
within the loaded module, and so multiple bonds will all use the same starting value. Setting the lacp_port_id via sysfs has no effect, as it alters a per-bond value, bond->params.lacp_port_id, that is never actually used to set the port ID of a first slave in bond_enslave.
The global default value should only be used to initialize the
per-bond value when a bond is created, and that per-bond value should be used when setting the port id in bond_enslave(). The per-bond value is already displayed in /proc/net/bonding/bond0, and is the value modified by the sysfs functions

Quoted 14 lines

Quoted 2 lines
Third, consider adding the port ID to the 803.2ad section in
bond_info_show_slave.
Thanks for these great comments. I'll soon fix sysfs related bug and rework patch.
Quoted 7 lines
Lastly, I think this should be tested against systems other than
Cisco to insure that it really interoperates with, for example, Juniper's methodology for spanning an aggregator across physical chassis. I'm not sure why it wouldn't, but once new functionality becomes part of the kernel, changing it in non-backwards compatible ways is difficult.

agreed

Ding Tianhong

Quoted 5 lines

I agree. I try to test it with devices from other manufacturers.
.

Krisztian Ivancso

author

2013-08-16 02:07 UTC #8 lore raw

Quoted 56 lines · Ding Tianhong

Quoted 55 lines · Krisztian Ivancso
Quoted 54 lines · Jay Vosburgh
Quoted 9 lines · Krisztian Ivancso
Quoted 8 lines · Ding Tianhong
Quoted 6 lines · Krisztian Ivancso
Quoted 5 lines · Ding Tianhong
Quoted 4 lines · Krisztian Ivancso
Quoted 1 line
From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <[email protected]>
Date: Sun, 11 Aug 2013 20:30:44 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad ports
Quoted 5 lines
Quoted 2 lines
ok, for example: the bonding has four slave, slave1 and slave2 aggregation to 1 group, and slave3 and slave4 aggregtion to 2 group, how you distinguish the 1 and 2 group by initialize id.
this is not possible, because all slave have to be a member of the same aggregation group.
Just on the above point, bonding can group slaves into multiple
aggregators, but only one aggregator will be active at any given time.
To answer the question, the four slaves would each be given
unique port IDs that do not conflict.
Quoted 18 lines
i think we misunderstood each other.
here is a new example:
- switch1 is a switch with a configured lag with two members ports (member1 and member2)
- two linux (linux1 and linux2) box with a configured bonding device (bond0) with the same MAC set in both box and one
slave on each
- lacp_port_id is set to 10 in linux1 and 20 in linux2
you can attach the slave from both linux boxes to the same lag on switch1. (slave from linux1 to port member1 and slave from linux2 to port member2 on switch1)
port id must be unique within a system.
bonding implementation set a unique system id for every bonding device which is derived from MAC of one of the slave interfaces.
Quoted 5 lines
if we use the current bonding implementation second linux box can't be a member on switch1 because port id is 1 in both linux bonding device.
if we can set different starting port id for bonding in different boxes the second box can be a member also.
I understand what you're trying to do here (permit multiple
instances of bonding on different systems to connect to a single aggregator on a switch), and I don't really have a problem with it in general.
I do have some comments:

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
First, altering the lacp_port_id (via sysfs) should only be
permitted when there are no slaves in the bond, otherwise the /proc/net/bonding/bond0 output for the first port id will not match the actual first port id value assigned to the slaves. As a practical matter, altering lacp_port_id while slaves are present in the bond has no effect until all slaves are released and the first new slave is added, so this is not reducing functionality.

fixed in bonding_store_lacp_port_id():

(        struct slave *first_slave = bond_first_slave(bond);
        if (first_slave) {
                pr_err("%s: Can't set port id because device have slave(s).\n",
                       bond->dev->name);
                return -EPERM;
        }

)

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Second, the lacp_port_id is global across all bonds created
within the loaded module, and so multiple bonds will all use the same starting value. Setting the lacp_port_id via sysfs has no effect, as it alters a per-bond value, bond->params.lacp_port_id, that is never actually used to set the port ID of a first slave in bond_enslave.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
The global default value should only be used to initialize the
per-bond value when a bond is created, and that per-bond value should be used when setting the port id in bond_enslave(). The per-bond value is already displayed in /proc/net/bonding/bond0, and is the value modified by the sysfs functions

global default value is used to initialize bonding_defaults.lacp_port_id now. (params->lacp_port_id = lacp_port_id;)

first slave is initialized by per-bond port id in bond_enslave(). (SLAVE_AD_INFO(new_slave).id = bond->params.lacp_port_id;)

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Third, consider adding the port ID to the 803.2ad section in
bond_info_show_slave.

slave port id is shown in bond_info_show_slave():
(seq_printf(seq, "Slave port id: %d\n", SLAVE_AD_INFO(slave).id);)

Quoted 13 lines

Quoted 12 lines
Thanks for these great comments. I'll soon fix sysfs related bug and rework patch.
Quoted 7 lines
Lastly, I think this should be tested against systems other than
Cisco to insure that it really interoperates with, for example, Juniper's methodology for spanning an aggregator across physical chassis. I'm not sure why it wouldn't, but once new functionality becomes part of the kernel, changing it in non-backwards compatible ways is difficult.

Quoted 3 lines

agreed
Ding Tianhong

port id overflow check in bond_enslave():

(                        prev_slave = bond_prev_slave(bond, new_slave);
                        if (SLAVE_AD_INFO(prev_slave).id == 65535) {
                                pr_err("%s: Error: Couldn't add new slave (%s) because maximum port id (65535) is reached.\n",
                                       bond_dev->name, slave_dev->name);
                                res = -EPERM;
                                goto err_detach;
                        }

)
i'm not sure "goto err_detach" is the proper jump but it seems to be good.

i hope next week i can test this solution with devices other than cisco. (older juniper and low-end hp 3com)

btw i welcome any test with any network devices related to this feature.

Quoted 1 line

From 974763f179cf741a8b9b0459be2e9c84d23e8989 Mon Sep 17 00:00:00 2001

From: Krisztian Ivancso <[email protected]>
Date: Fri, 16 Aug 2013 02:25:45 +0200
Subject: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

---
 drivers/net/bonding/bond_main.c   | 24 +++++++++++++++++++++--
 drivers/net/bonding/bond_procfs.c |  2 ++
 drivers/net/bonding/bond_sysfs.c  | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h     |  1 +
 4 files changed, 65 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +22 -2

index 4264a76..3580866 100644
@@ -110,6 +110,7 @@ static char *fail_over_mac;
 static int all_slaves_active;
 static struct bond_params bonding_defaults;
 static int resend_igmp = BOND_DEFAULT_RESEND_IGMP;
+static int lacp_port_id = 1;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");

@@ -150,7 +151,7 @@ module_param(lacp_rate, charp, 0);
 MODULE_PARM_DESC(lacp_rate, "LACPDU tx rate to request from 802.3ad partner; "
 			    "0 for slow, 1 for fast");
 module_param(ad_select, charp, 0);
-MODULE_PARM_DESC(ad_select, "803.ad aggregation selection logic; "
+MODULE_PARM_DESC(ad_select, "802.3ad aggregation selection logic; "
 			    "0 for stable (default), 1 for bandwidth, "
 			    "2 for count");
 module_param(min_links, int, 0);

@@ -181,6 +182,8 @@ MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface"
 module_param(resend_igmp, int, 0);
 MODULE_PARM_DESC(resend_igmp, "Number of IGMP membership reports to send on "
 			      "link failure");
+module_param(lacp_port_id, int, 0);
+MODULE_PARM_DESC(lacp_port_id, "802.3ad port id to send to switch in LACPDU");
 
 /*----------------------------- Global variables ----------------------------*/

@@ -1699,7 +1702,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		bond_set_slave_inactive_flags(new_slave);
 		/* if this is the first slave */
 		if (bond_first_slave(bond) == new_slave) {
-			SLAVE_AD_INFO(new_slave).id = 1;
+			SLAVE_AD_INFO(new_slave).id = bond->params.lacp_port_id;
 			/* Initialize AD with the number of times that the AD timer is called in 1 second
 			 * can be called only after the mac address of the bond is set
 			 */

@@ -1708,6 +1711,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			struct slave *prev_slave;
 
 			prev_slave = bond_prev_slave(bond, new_slave);
+			if (SLAVE_AD_INFO(prev_slave).id == 65535) {
+				pr_err("%s: Error: Couldn't add new slave (%s) because maximum port id (65535) is reached.\n",
+				       bond_dev->name, slave_dev->name);
+				res = -EPERM;
+				goto err_detach;
+			}
 			SLAVE_AD_INFO(new_slave).id =
 				SLAVE_AD_INFO(prev_slave).id + 1;
 		}

@@ -4377,6 +4386,16 @@ static int bond_check_params(struct bond_params *params)
 		resend_igmp = BOND_DEFAULT_RESEND_IGMP;
 	}
 
+	if (bond_mode == BOND_MODE_8023AD) {
+		/* we set upper limit to 65000 because new slaves will increase port
+                   id so we don't want id to overflow */
+		if (lacp_port_id < 1 || lacp_port_id > 65000) {
+			pr_warning("Warning: lacp_port_id (%d) should be between "
+				   "1 and 65000, resetting to 1\n", lacp_port_id);
+			lacp_port_id = 1;
+		}
+	}
+
 	/* reset values for TLB/ALB */
 	if ((bond_mode == BOND_MODE_TLB) ||
 	    (bond_mode == BOND_MODE_ALB)) {

@@ -4565,6 +4584,7 @@ static int bond_check_params(struct bond_params *params)
 	params->all_slaves_active = all_slaves_active;
 	params->resend_igmp = resend_igmp;
 	params->min_links = min_links;
+	params->lacp_port_id = lacp_port_id;
 
 	if (primary) {
 		strncpy(params->primary, primary, IFNAMSIZ);

drivers/net/bonding/bond_procfs.c +2 -0

index 20a6ee2..96977d5 100644
@@ -129,6 +129,7 @@ static void bond_info_show_master(struct seq_file *seq)
 		seq_printf(seq, "Min links: %d\n", bond->params.min_links);
 		seq_printf(seq, "Aggregator selection policy (ad_select): %s\n",
 			   ad_select_tbl[bond->params.ad_select].modename);
+		seq_printf(seq, "802.3ad starting port id: %d\n", bond->params.lacp_port_id);
 
 		if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 			seq_printf(seq, "bond %s has no active aggregator\n",

@@ -193,6 +194,7 @@ static void bond_info_show_slave(struct seq_file *seq,
 				   agg->aggregator_identifier);
 		else
 			seq_puts(seq, "Aggregator ID: N/A\n");
+		seq_printf(seq, "Slave port id: %d\n", SLAVE_AD_INFO(slave).id);
 	}
 	seq_printf(seq, "Slave queue ID: %d\n", slave->queue_id);
 }

drivers/net/bonding/bond_sysfs.c +40 -0

index 0f539de..35b4bdd 100644
@@ -920,6 +920,45 @@ static ssize_t bonding_store_min_links(struct device *d,
 static DEVICE_ATTR(min_links, S_IRUGO | S_IWUSR,
 		   bonding_show_min_links, bonding_store_min_links);
 
+static ssize_t bonding_show_lacp_port_id(struct device *d,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct bonding *bond = to_bond(d);
+
+	return sprintf(buf, "%d\n", bond->params.lacp_port_id);
+}
+
+static ssize_t bonding_store_lacp_port_id(struct device *d,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct bonding *bond = to_bond(d);
+	int ret;
+	unsigned int new_value;
+
+	struct slave *first_slave = bond_first_slave(bond);
+	if (first_slave) {
+		pr_err("%s: Can't set port id because device have slave(s).\n",
+		       bond->dev->name);
+		return -EPERM;
+	}
+
+	ret = kstrtouint(buf, 0, &new_value);
+	if ((ret < 0) || (new_value < 1) || (new_value > 65000)) {
+		pr_err("%s: Ignoring invalid 802.3ad port id value %s.\n",
+		       bond->dev->name, buf);
+		return -EINVAL;
+	}
+
+	pr_info("%s: Setting 802.3ad port id value to %u\n",
+		bond->dev->name, new_value);
+	bond->params.lacp_port_id = new_value;
+	return count;
+}
+static DEVICE_ATTR(lacp_port_id, S_IRUGO | S_IWUSR,
+		   bonding_show_lacp_port_id, bonding_store_lacp_port_id);
+
 static ssize_t bonding_show_ad_select(struct device *d,
 				      struct device_attribute *attr,
 				      char *buf)

@@ -1705,6 +1744,7 @@ static struct attribute *per_bond_attrs[] = {
 	&dev_attr_all_slaves_active.attr,
 	&dev_attr_resend_igmp.attr,
 	&dev_attr_min_links.attr,
+	&dev_attr_lacp_port_id.attr,
 	NULL,
 };

drivers/net/bonding/bonding.h +1 -0

index 4bf52d5..8a078d6 100644
@@ -176,6 +176,7 @@ struct bond_params {
 	int tx_queues;
 	int all_slaves_active;
 	int resend_igmp;
+	int lacp_port_id;
 };
 
 struct bond_parm_tbl {

Signature · 1 line

1.8.3.1