[PATCH net] bonding: fix accounting of active ports in 3ad

[PATCH net] bonding: fix accounting of active ports in 3ad

[Jarod Wilson]

As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
removed from the aggregator when they are down, and the active slave count
is NOT equal to number of ports in the aggregator, but rather the number
of ports in the aggregator that are still enabled. The sysfs spew for
bonding_show_ad_num_ports() has a comment that says "Show number of active
802.3ad ports.", but it's currently showing total number of ports, both
active and inactive. Remedy it by using the same logic introduced in
7bb11dc9f59d in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
netlink all report the number of active ports. Note that this means that
IFLA_BOND_AD_INFO_NUM_PORTS really means NUM_ACTIVE_PORTS instead of
NUM_PORTS, and thus perhaps should be renamed for clarity.

Lightly tested on a dual i40e lacp bond, simulating link downs with an ip
link set dev <slave2> down, was able to produce the state where I could
see both in the same aggregator, but a number of ports count of 1.

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c5fd4259da33..b44a6aeb346d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2577,7 +2577,7 @@ int __bond_3ad_get_active_agg_info(struct bonding *bond,
 		return -1;
 
 	ad_info->aggregator_id = aggregator->aggregator_identifier;
-	ad_info->ports = aggregator->num_of_ports;
+	ad_info->ports = __agg_active_ports(aggregator);
 	ad_info->actor_key = aggregator->actor_oper_aggregator_key;
 	ad_info->partner_key = aggregator->partner_oper_aggregator_key;
 	ether_addr_copy(ad_info->partner_system,
-- 
2.12.1

Re: [PATCH net] bonding: fix accounting of active ports in 3ad

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Wed, 17 May 2017 11:11:44 -0400

> As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
> removed from the aggregator when they are down, and the active slave count
> is NOT equal to number of ports in the aggregator, but rather the number
> of ports in the aggregator that are still enabled.
 ...
> Remedy it by using the same logic introduced in
> 7bb11dc9f59d in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
  ^^^^^^^^^^^^
> netlink all report the number of active ports.

I think you mean to reference commit 0622cab0341c here not 7bb11dc9f59d.

Re: [PATCH net] bonding: fix accounting of active ports in 3ad

[Jarod Wilson]

On 2017-05-19 5:14 PM, David Miller wrote:
> From: Jarod Wilson <jarod@redhat.com>
> Date: Wed, 17 May 2017 11:11:44 -0400
> 
>> As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
>> removed from the aggregator when they are down, and the active slave count
>> is NOT equal to number of ports in the aggregator, but rather the number
>> of ports in the aggregator that are still enabled.
>   ...
>> Remedy it by using the same logic introduced in
>> 7bb11dc9f59d in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
>    ^^^^^^^^^^^^
>> netlink all report the number of active ports.
> 
> I think you mean to reference commit 0622cab0341c here not 7bb11dc9f59d.

D'oh, yes, you are entirely correct. Should I submit a v2 with that 
correction?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH net] bonding: fix accounting of active ports in 3ad

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Fri, 19 May 2017 18:15:57 -0400

> On 2017-05-19 5:14 PM, David Miller wrote:
>> From: Jarod Wilson <jarod@redhat.com>
>> Date: Wed, 17 May 2017 11:11:44 -0400
>> 
>>> As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
>>> removed from the aggregator when they are down, and the active slave
>>> count
>>> is NOT equal to number of ports in the aggregator, but rather the
>>> number
>>> of ports in the aggregator that are still enabled.
>>   ...
>>> Remedy it by using the same logic introduced in
>>> 7bb11dc9f59d in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
>>    ^^^^^^^^^^^^
>>> netlink all report the number of active ports.
>> I think you mean to reference commit 0622cab0341c here not
>> 7bb11dc9f59d.
> 
> D'oh, yes, you are entirely correct. Should I submit a v2 with that
> correction?

Yes, please.

[PATCH net v2] bonding: fix accounting of active ports in 3ad

[Jarod Wilson]

As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
removed from the aggregator when they are down, and the active slave count
is NOT equal to number of ports in the aggregator, but rather the number
of ports in the aggregator that are still enabled. The sysfs spew for
bonding_show_ad_num_ports() has a comment that says "Show number of active
802.3ad ports.", but it's currently showing total number of ports, both
active and inactive. Remedy it by using the same logic introduced in
0622cab0341c in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
netlink all report the number of active ports. Note that this means that
IFLA_BOND_AD_INFO_NUM_PORTS really means NUM_ACTIVE_PORTS instead of
NUM_PORTS, and thus perhaps should be renamed for clarity.

Lightly tested on a dual i40e lacp bond, simulating link downs with an ip
link set dev <slave2> down, was able to produce the state where I could
see both in the same aggregator, but a number of ports count of 1.

MII Status: up
Active Aggregator Info:
        Aggregator ID: 1
        Number of ports: 2 <---
Slave Interface: ens10
MII Status: up <---
Aggregator ID: 1
Slave Interface: ens11
MII Status: up
Aggregator ID: 1

MII Status: up
Active Aggregator Info:
        Aggregator ID: 1
        Number of ports: 1 <---
Slave Interface: ens10
MII Status: down <---
Aggregator ID: 1
Slave Interface: ens11
MII Status: up
Aggregator ID: 1

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
v2: fix incorrect git sha reference, add more testing data

 drivers/net/bonding/bond_3ad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c5fd4259da33..b44a6aeb346d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2577,7 +2577,7 @@ int __bond_3ad_get_active_agg_info(struct bonding *bond,
 		return -1;
 
 	ad_info->aggregator_id = aggregator->aggregator_identifier;
-	ad_info->ports = aggregator->num_of_ports;
+	ad_info->ports = __agg_active_ports(aggregator);
 	ad_info->actor_key = aggregator->actor_oper_aggregator_key;
 	ad_info->partner_key = aggregator->partner_oper_aggregator_key;
 	ether_addr_copy(ad_info->partner_system,
-- 
2.12.1

Re: [PATCH net v2] bonding: fix accounting of active ports in 3ad

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Fri, 19 May 2017 19:43:45 -0400

> As of 7bb11dc9f59d and 0622cab0341c, bond slaves in a 3ad bond are not
> removed from the aggregator when they are down, and the active slave count
> is NOT equal to number of ports in the aggregator, but rather the number
> of ports in the aggregator that are still enabled. The sysfs spew for
> bonding_show_ad_num_ports() has a comment that says "Show number of active
> 802.3ad ports.", but it's currently showing total number of ports, both
> active and inactive. Remedy it by using the same logic introduced in
> 0622cab0341c in __bond_3ad_get_active_agg_info(), so sysfs, procfs and
> netlink all report the number of active ports. Note that this means that
> IFLA_BOND_AD_INFO_NUM_PORTS really means NUM_ACTIVE_PORTS instead of
> NUM_PORTS, and thus perhaps should be renamed for clarity.
> 
> Lightly tested on a dual i40e lacp bond, simulating link downs with an ip
> link set dev <slave2> down, was able to produce the state where I could
> see both in the same aggregator, but a number of ports count of 1.
 ...
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
> v2: fix incorrect git sha reference, add more testing data

Applied and queued up for -stable, thanks Jarod!