[PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[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.


>From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <github-ivan@ivancso.net>
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 <github-ivan@ivancso.net>
---
 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(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4264a76..452f4d4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 20a6ee2..6d52716 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -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",
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 0f539de..a79b7c3 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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,
 };

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..8a078d6 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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 {
-- 
1.8.3.1

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Ding Tianhong]

On 2013/8/12 19:19, Krisztian Ivancso wrote:
> 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.
> 
> 
>>From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
> From: Krisztian Ivancso <github-ivan@ivancso.net>
> 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.

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Krisztian Ivancso]

On 08/13/2013 03:07 AM, Ding Tianhong wrote:
> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>> 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.

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:
> LACP uses the port priority with the port number to form the
> port identifier.

Juniper:
> 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:
> 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.

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Ding Tianhong]

On 2013/8/13 17:20, Krisztian Ivancso wrote:
> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>> 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.
> 
> 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:
>> LACP uses the port priority with the port number to form the
>> port identifier.
> 
> Juniper:
>> 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:
>> 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.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> 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

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Krisztian Ivancso]

On 08/13/2013 11:39 AM, Ding Tianhong wrote:
> On 2013/8/13 17:20, Krisztian Ivancso wrote:
>> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>>> 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.

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Jay Vosburgh]

Krisztian Ivancso <github-ivan@ivancso.net> wrote:

>On 08/13/2013 11:39 AM, Ding Tianhong wrote:
>> On 2013/8/13 17:20, Krisztian Ivancso wrote:
>>> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>>>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>>>> 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.

	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.

>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.

	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, fubar@us.ibm.com

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Krisztian Ivancso]

On 08/13/2013 08:25 PM, Jay Vosburgh wrote:
> Krisztian Ivancso <github-ivan@ivancso.net> wrote:
> 
>> On 08/13/2013 11:39 AM, Ding Tianhong wrote:
>>> On 2013/8/13 17:20, Krisztian Ivancso wrote:
>>>> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>>>>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>>>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>>>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>>>>> 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.
> 
> 	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.
> 
>> 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.
> 
> 	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.

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

> 
> 	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.

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Ding Tianhong]

On 2013/8/14 7:34, Krisztian Ivancso wrote:
> On 08/13/2013 08:25 PM, Jay Vosburgh wrote:
>> Krisztian Ivancso <github-ivan@ivancso.net> wrote:
>>
>>> On 08/13/2013 11:39 AM, Ding Tianhong wrote:
>>>> On 2013/8/13 17:20, Krisztian Ivancso wrote:
>>>>> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>>>>>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>>>>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>>>>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>>>>>> 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.
>>
>> 	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.
>>
>>> 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.
>>
>> 	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.
> 
> Thanks for these great comments. I'll soon fix sysfs related bug and
> rework patch.
> 
>>
>> 	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

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

Re: [PATCH net-next] bonding: lacp_port_id setting for 802.3ad

[Krisztian Ivancso]

On 08/14/2013 03:46 AM, Ding Tianhong wrote:
> On 2013/8/14 7:34, Krisztian Ivancso wrote:
>> On 08/13/2013 08:25 PM, Jay Vosburgh wrote:
>>> Krisztian Ivancso <github-ivan@ivancso.net> wrote:
>>>
>>>> On 08/13/2013 11:39 AM, Ding Tianhong wrote:
>>>>> On 2013/8/13 17:20, Krisztian Ivancso wrote:
>>>>>> On 08/13/2013 03:07 AM, Ding Tianhong wrote:
>>>>>>> On 2013/8/12 19:19, Krisztian Ivancso wrote:
>>>>>>>> >From 472fffa5a8f170daed9e4cc677af8e2560b86be2 Mon Sep 17 00:00:00 2001
>>>>>>>> From: Krisztian Ivancso <github-ivan@ivancso.net>
>>>>>>>> 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.
>>>
>>> 	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.
>>>
>>>> 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.
>>>
>>> 	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.

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;
        }
)

>>>
>>> 	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

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;)

>>>
>>> 	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);)

>>
>> Thanks for these great comments. I'll soon fix sysfs related bug and
>> rework patch.
>>
>>>
>>> 	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
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.




>From 974763f179cf741a8b9b0459be2e9c84d23e8989 Mon Sep 17 00:00:00 2001
From: Krisztian Ivancso <github-ivan@ivancso.net>
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(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4264a76..3580866 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 20a6ee2..96977d5 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -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);
 }
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 0f539de..35b4bdd 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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,
 };

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..8a078d6 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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 {
-- 
1.8.3.1