[PATCH v1 net-next 0/2] bonding: Introduce 4 AD link speed

[PATCH v1 net-next 0/2] bonding: Introduce 4 AD link speed

[Xie Jianhua]

From: Jianhua Xie <Jianhua.Xie@freescale.com>

User Key bits in Port key of AD mode are yet not used.  This series
expands speed type bits and shrinks unused user Key bits in AD mode
Port Key, introduces 4 AD link speed: 2.5G/20G/40G/56G, and fixes
aggregated link bandwidth calculation based on new link speed.

Jianhua Xie (2):
  bonding: Expand speed type bits of the AD Port Key
  bonding: Introduce 4 AD link speed to fix agg_bandwidth

 drivers/net/bonding/bond_3ad.c | 44 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

-- 
Changes in v1:
 * Compress RFC patches #2 to #5 into one single patch.
 * Fix inexact commit information.

2.1.0.27.g96db324

[PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[Xie Jianhua]

From: Jianhua Xie <Jianhua.Xie@freescale.com>

Port Key was determined as 16 bits according to the link speed,
duplex and user key (which is yet not supported), in which key
speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
--------------------------------------------------------------
Port key :|	User key	| Speed		|	Duplex|
--------------------------------------------------------------
16			6		1		0
This patch is expanding speed type from 5 bits to 9 bits for other
speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
bits to 6 bits.  New Port Key looks like below:
--------------------------------------------------------------
Port key :|	User key	| Speed		|	Duplex|
--------------------------------------------------------------
16			10		1		0

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: David S. Miller <davem@davemloft.net>

Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
---
 drivers/net/bonding/bond_3ad.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 2110215f..8e5ab5c 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -77,11 +77,11 @@
  * --------------------------------------------------------------
  * Port key :	| User key	| Speed		| Duplex	|
  * --------------------------------------------------------------
- * 16		  6		  1		  0
+ * 16			10		1		0
  */
 #define  AD_DUPLEX_KEY_BITS    0x1
-#define  AD_SPEED_KEY_BITS     0x3E
-#define  AD_USER_KEY_BITS      0xFFC0
+#define  AD_SPEED_KEY_BITS     0x3FE
+#define  AD_USER_KEY_BITS      0xFC00
 
 #define     AD_LINK_SPEED_BITMASK_1MBPS       0x1
 #define     AD_LINK_SPEED_BITMASK_10MBPS      0x2
-- 
2.1.0.27.g96db324

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[David Miller]

From: Xie Jianhua <Jianhua.Xie@freescale.com>
Date: Mon, 10 Nov 2014 15:16:40 +0800

> From: Jianhua Xie <Jianhua.Xie@freescale.com>
> 
> Port Key was determined as 16 bits according to the link speed,
> duplex and user key (which is yet not supported), in which key
> speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
> --------------------------------------------------------------
> Port key :|	User key	| Speed		|	Duplex|
> --------------------------------------------------------------
> 16			6		1		0
> This patch is expanding speed type from 5 bits to 9 bits for other
> speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
> bits to 6 bits.  New Port Key looks like below:
> --------------------------------------------------------------
> Port key :|	User key	| Speed		|	Duplex|
> --------------------------------------------------------------
> 16			10		1		0
> 
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: David S. Miller <davem@davemloft.net>
> 
> Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>

Do we determine the layout of this value all ourselves?

If not, then is it exported to anything user-visible that we
might be breaking?

If it is private, it makes no sense to use a bitmask for the speed.
We should instead change the field to be some numerically increasing
value.

Otherwise we'll run out of bits again and keep having to adjust the
field layout more often than we really need to.

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: Xie Jianhua <Jianhua.Xie@freescale.com>
>Date: Mon, 10 Nov 2014 15:16:40 +0800
>
>> From: Jianhua Xie <Jianhua.Xie@freescale.com>
>> 
>> Port Key was determined as 16 bits according to the link speed,
>> duplex and user key (which is yet not supported), in which key
>> speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
>> --------------------------------------------------------------
>> Port key :|	User key	| Speed		|	Duplex|
>> --------------------------------------------------------------
>> 16			6		1		0
>> This patch is expanding speed type from 5 bits to 9 bits for other
>> speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
>> bits to 6 bits.  New Port Key looks like below:
>> --------------------------------------------------------------
>> Port key :|	User key	| Speed		|	Duplex|
>> --------------------------------------------------------------
>> 16			10		1		0
>> 
>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>> CC: Veaceslav Falico <vfalico@gmail.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: David S. Miller <davem@davemloft.net>
>> 
>> Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
>
>Do we determine the layout of this value all ourselves?

	Yes, we do.  The precise format of the port key is not defined
by the standard; IEEE 802.1AX 5.3.5, "Capability identification":

"A given Key value is meaningful only in the context of the System that
allocates it; there is no global significance to Key values."

	and

"When a System assigns an operational Key value to a set of ports, it
signifies that, in the absence of other constraints, the current
operational state of the set of ports allows any subset of that set of
ports (including the entire set) to be aggregated together from the
perspective of the System making the assignment."

	So, basically, it's a magic cookie that indicates that all ports
on a particular system with the same key value are suitable to be
aggregated together.

>If not, then is it exported to anything user-visible that we
>might be breaking?

	The key values are not user-visible, and the "user" settable
portion of the key has never been implemented.

>If it is private, it makes no sense to use a bitmask for the speed.
>We should instead change the field to be some numerically increasing
>value.
>
>Otherwise we'll run out of bits again and keep having to adjust the
>field layout more often than we really need to.

	Agreed.

	Also note that there are some internal dependencies within
bonding on the format; in particular the duplex bit in the key is used
to determine if a port is LACP-capable, and that functionality needs to
be preserved.

	-J

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

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[Jianhua Xie]

Thanks you two for the valuable comments.

If my understanding is right,  it is encouraged to use a counter
rather than a bitmask for the speed field, right?

if yes, how many bits are better to use for current speed and
future speed (like 100Gbps/400Gbps and etc.)?  I am not sure
that 5 bits are enough (2**5=32) or not. And I am clear to keep
"the duplex bit in the key " in my mind.

if not, what's your recommendation please?

Thanks & Best Regards,
Jianhua

在 2014年11月12日 03:47, Jay Vosburgh 写道:
> David Miller <davem@davemloft.net> wrote:
>
>> From: Xie Jianhua <Jianhua.Xie@freescale.com>
>> Date: Mon, 10 Nov 2014 15:16:40 +0800
>>
>>> From: Jianhua Xie <Jianhua.Xie@freescale.com>
>>>
>>> Port Key was determined as 16 bits according to the link speed,
>>> duplex and user key (which is yet not supported), in which key
>>> speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
>>> --------------------------------------------------------------
>>> Port key :|	User key	| Speed		|	Duplex|
>>> --------------------------------------------------------------
>>> 16			6		1		0
>>> This patch is expanding speed type from 5 bits to 9 bits for other
>>> speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
>>> bits to 6 bits.  New Port Key looks like below:
>>> --------------------------------------------------------------
>>> Port key :|	User key	| Speed		|	Duplex|
>>> --------------------------------------------------------------
>>> 16			10		1		0
>>>
>> Do we determine the layout of this value all ourselves?
> 	Yes, we do.  The precise format of the port key is not defined
> by the standard; IEEE 802.1AX 5.3.5, "Capability identification":
>
> "A given Key value is meaningful only in the context of the System that
> allocates it; there is no global significance to Key values."
>
> 	and
>
> "When a System assigns an operational Key value to a set of ports, it
> signifies that, in the absence of other constraints, the current
> operational state of the set of ports allows any subset of that set of
> ports (including the entire set) to be aggregated together from the
> perspective of the System making the assignment."
>
> 	So, basically, it's a magic cookie that indicates that all ports
> on a particular system with the same key value are suitable to be
> aggregated together.
>
>> If not, then is it exported to anything user-visible that we
>> might be breaking?
> 	The key values are not user-visible, and the "user" settable
> portion of the key has never been implemented.
>
>> If it is private, it makes no sense to use a bitmask for the speed.
>> We should instead change the field to be some numerically increasing
>> value.
>>
>> Otherwise we'll run out of bits again and keep having to adjust the
>> field layout more often than we really need to.
> 	Agreed.
>
> 	Also note that there are some internal dependencies within
> bonding on the format; in particular the duplex bit in the key is used
> to determine if a port is LACP-capable, and that functionality needs to
> be preserved.
>
> 	-J
>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[Veaceslav Falico]

On Wed, Nov 12, 2014 at 05:53:41PM +0800, Jianhua Xie wrote:
>Thanks you two for the valuable comments.
>
>If my understanding is right,  it is encouraged to use a counter
>rather than a bitmask for the speed field, right?
>
>if yes, how many bits are better to use for current speed and
>future speed (like 100Gbps/400Gbps and etc.)?  I am not sure
>that 5 bits are enough (2**5=32) or not. And I am clear to keep
>"the duplex bit in the key " in my mind.
>
>if not, what's your recommendation please?

As it's visible to bonding only, I guess a simple enum should do the trick.
No need to invent something special, and it'll fit nicely with other enums
from AD.

>
>Thanks & Best Regards,
>Jianhua
>
>在 2014年11月12日 03:47, Jay Vosburgh 写道:
>>David Miller <davem@davemloft.net> wrote:
>>
>>>From: Xie Jianhua <Jianhua.Xie@freescale.com>
>>>Date: Mon, 10 Nov 2014 15:16:40 +0800
>>>
>>>>From: Jianhua Xie <Jianhua.Xie@freescale.com>
>>>>
>>>>Port Key was determined as 16 bits according to the link speed,
>>>>duplex and user key (which is yet not supported), in which key
>>>>speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
>>>>--------------------------------------------------------------
>>>>Port key :|	User key	| Speed		|	Duplex|
>>>>--------------------------------------------------------------
>>>>16			6		1		0
>>>>This patch is expanding speed type from 5 bits to 9 bits for other
>>>>speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
>>>>bits to 6 bits.  New Port Key looks like below:
>>>>--------------------------------------------------------------
>>>>Port key :|	User key	| Speed		|	Duplex|
>>>>--------------------------------------------------------------
>>>>16			10		1		0
>>>>
>>>Do we determine the layout of this value all ourselves?
>>	Yes, we do.  The precise format of the port key is not defined
>>by the standard; IEEE 802.1AX 5.3.5, "Capability identification":
>>
>>"A given Key value is meaningful only in the context of the System that
>>allocates it; there is no global significance to Key values."
>>
>>	and
>>
>>"When a System assigns an operational Key value to a set of ports, it
>>signifies that, in the absence of other constraints, the current
>>operational state of the set of ports allows any subset of that set of
>>ports (including the entire set) to be aggregated together from the
>>perspective of the System making the assignment."
>>
>>	So, basically, it's a magic cookie that indicates that all ports
>>on a particular system with the same key value are suitable to be
>>aggregated together.
>>
>>>If not, then is it exported to anything user-visible that we
>>>might be breaking?
>>	The key values are not user-visible, and the "user" settable
>>portion of the key has never been implemented.
>>
>>>If it is private, it makes no sense to use a bitmask for the speed.
>>>We should instead change the field to be some numerically increasing
>>>value.
>>>
>>>Otherwise we'll run out of bits again and keep having to adjust the
>>>field layout more often than we really need to.
>>	Agreed.
>>
>>	Also note that there are some internal dependencies within
>>bonding on the format; in particular the duplex bit in the key is used
>>to determine if a port is LACP-capable, and that functionality needs to
>>be preserved.
>>
>>	-J
>>
>>---
>>	-Jay Vosburgh, jay.vosburgh@canonical.com
>

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[Jianhua Xie]

在 2014年11月12日 19:20, Veaceslav Falico 写道:
> On Wed, Nov 12, 2014 at 05:53:41PM +0800, Jianhua Xie wrote:
>> Thanks you two for the valuable comments.
>>
>> If my understanding is right,  it is encouraged to use a counter
>> rather than a bitmask for the speed field, right?
>>
>> if yes, how many bits are better to use for current speed and
>> future speed (like 100Gbps/400Gbps and etc.)?  I am not sure
>> that 5 bits are enough (2**5=32) or not. And I am clear to keep
>> "the duplex bit in the key " in my mind.
>>
>> if not, what's your recommendation please?
>
> As it's visible to bonding only, I guess a simple enum should do the 
> trick.
> No need to invent something special, and it'll fit nicely with other 
> enums
> from AD.
Thanks comments from Jay Vosburgh and Veaceslav Falico.  However, my
method can also work, and also compatible with current bonding driver.
But Veaceslav Falico's method is better than mine.  I am glad to take his
advice. I will use an enum to instead of AD_LINK_SPEED_BITMASK micros
which are based on bitmask.

I also thank Miller for the kindly reminder on "please don't top-post".

Thank & Best Regards,
Jianhua
>
>>
>> Thanks & Best Regards,
>> Jianhua
>>
>> 在 2014年11月12日 03:47, Jay Vosburgh 写道:
>>> David Miller <davem@davemloft.net> wrote:
>>>
>>>> From: Xie Jianhua <Jianhua.Xie@freescale.com>
>>>> Date: Mon, 10 Nov 2014 15:16:40 +0800
>>>>
>>>>> From: Jianhua Xie <Jianhua.Xie@freescale.com>
>>>>>
>>>>> Port Key was determined as 16 bits according to the link speed,
>>>>> duplex and user key (which is yet not supported), in which key
>>>>> speed was 5 bits for 1Mbps/10Mbps/100Mbps/1Gbps/10Gbps as below:
>>>>> --------------------------------------------------------------
>>>>> Port key :|    User key    | Speed        |    Duplex|
>>>>> --------------------------------------------------------------
>>>>> 16            6        1        0
>>>>> This patch is expanding speed type from 5 bits to 9 bits for other
>>>>> speed 2.5Gbps/20Gbps/40Gbps/56Gbps and shrinking user key from 10
>>>>> bits to 6 bits.  New Port Key looks like below:
>>>>> --------------------------------------------------------------
>>>>> Port key :|    User key    | Speed        |    Duplex|
>>>>> --------------------------------------------------------------
>>>>> 16            10        1        0
>>>>>
>>>> Do we determine the layout of this value all ourselves?
>>>     Yes, we do.  The precise format of the port key is not defined
>>> by the standard; IEEE 802.1AX 5.3.5, "Capability identification":
>>>
>>> "A given Key value is meaningful only in the context of the System that
>>> allocates it; there is no global significance to Key values."
>>>
>>>     and
>>>
>>> "When a System assigns an operational Key value to a set of ports, it
>>> signifies that, in the absence of other constraints, the current
>>> operational state of the set of ports allows any subset of that set of
>>> ports (including the entire set) to be aggregated together from the
>>> perspective of the System making the assignment."
>>>
>>>     So, basically, it's a magic cookie that indicates that all ports
>>> on a particular system with the same key value are suitable to be
>>> aggregated together.
>>>
>>>> If not, then is it exported to anything user-visible that we
>>>> might be breaking?
>>>     The key values are not user-visible, and the "user" settable
>>> portion of the key has never been implemented.
>>>
>>>> If it is private, it makes no sense to use a bitmask for the speed.
>>>> We should instead change the field to be some numerically increasing
>>>> value.
>>>>
>>>> Otherwise we'll run out of bits again and keep having to adjust the
>>>> field layout more often than we really need to.
>>>     Agreed.
>>>
>>>     Also note that there are some internal dependencies within
>>> bonding on the format; in particular the duplex bit in the key is used
>>> to determine if a port is LACP-capable, and that functionality needs to
>>> be preserved.
>>>
>>>     -J
>>>
>>> ---
>>>     -Jay Vosburgh, jay.vosburgh@canonical.com
>>

Re: [PATCH v1 net-next 1/2] bonding: Expand speed type bits of the AD Port Key

[David Miller]

Please do not top-post.

[PATCH v1 net-next 2/2] bonding: Introduce 4 AD link speed to fix agg_bandwidth

[Xie Jianhua]

From: Jianhua Xie <Jianhua.Xie@freescale.com>

This patch adds 2.5Gbps/20Gbps/40Gbps/56Gbps bitmask micros, and
fixes aggregated bandwidth calculation based on 2.5Gbps/20Gbps/
40Gbps/56Gbps slave links.

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: David S. Miller <davem@davemloft.net>

Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
---
 drivers/net/bonding/bond_3ad.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 8e5ab5c..ee4fe05 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -87,7 +87,11 @@
 #define     AD_LINK_SPEED_BITMASK_10MBPS      0x2
 #define     AD_LINK_SPEED_BITMASK_100MBPS     0x4
 #define     AD_LINK_SPEED_BITMASK_1000MBPS    0x8
-#define     AD_LINK_SPEED_BITMASK_10000MBPS   0x10
+#define     AD_LINK_SPEED_BITMASK_2500MBPS    0x10
+#define     AD_LINK_SPEED_BITMASK_10000MBPS   0x20
+#define     AD_LINK_SPEED_BITMASK_20000MBPS   0x40
+#define     AD_LINK_SPEED_BITMASK_40000MBPS   0x80
+#define     AD_LINK_SPEED_BITMASK_56000MBPS   0x100
 
 /* compare MAC addresses */
 #define MAC_ADDRESS_EQUAL(A, B)	\
@@ -245,7 +249,11 @@ static inline int __check_agg_selection_timer(struct port *port)
  *     %AD_LINK_SPEED_BITMASK_10MBPS,
  *     %AD_LINK_SPEED_BITMASK_100MBPS,
  *     %AD_LINK_SPEED_BITMASK_1000MBPS,
+ *     %AD_LINK_SPEED_BITMASK_2500MBPS,
  *     %AD_LINK_SPEED_BITMASK_10000MBPS
+ *     %AD_LINK_SPEED_BITMASK_20000MBPS
+ *     %AD_LINK_SPEED_BITMASK_40000MBPS
+ *     %AD_LINK_SPEED_BITMASK_56000MBPS
  */
 static u16 __get_link_speed(struct port *port)
 {
@@ -273,10 +281,26 @@ static u16 __get_link_speed(struct port *port)
 			speed = AD_LINK_SPEED_BITMASK_1000MBPS;
 			break;
 
+		case SPEED_2500:
+			speed = AD_LINK_SPEED_BITMASK_2500MBPS;
+			break;
+
 		case SPEED_10000:
 			speed = AD_LINK_SPEED_BITMASK_10000MBPS;
 			break;
 
+		case SPEED_20000:
+			speed = AD_LINK_SPEED_BITMASK_20000MBPS;
+			break;
+
+		case SPEED_40000:
+			speed = AD_LINK_SPEED_BITMASK_40000MBPS;
+			break;
+
+		case SPEED_56000:
+			speed = AD_LINK_SPEED_BITMASK_56000MBPS;
+			break;
+
 		default:
 			/* unknown speed value from ethtool. shouldn't happen */
 			speed = 0;
@@ -637,9 +661,21 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
 		case AD_LINK_SPEED_BITMASK_1000MBPS:
 			bandwidth = aggregator->num_of_ports * 1000;
 			break;
+		case AD_LINK_SPEED_BITMASK_2500MBPS:
+			bandwidth = aggregator->num_of_ports * 2500;
+			break;
 		case AD_LINK_SPEED_BITMASK_10000MBPS:
 			bandwidth = aggregator->num_of_ports * 10000;
 			break;
+		case AD_LINK_SPEED_BITMASK_20000MBPS:
+			bandwidth = aggregator->num_of_ports * 20000;
+			break;
+		case AD_LINK_SPEED_BITMASK_40000MBPS:
+			bandwidth = aggregator->num_of_ports * 40000;
+			break;
+		case AD_LINK_SPEED_BITMASK_56000MBPS:
+			bandwidth = aggregator->num_of_ports * 56000;
+			break;
 		default:
 			bandwidth = 0; /* to silence the compiler */
 		}
-- 
2.1.0.27.g96db324