[PATCH net-next 0/5] bonding: various 802.3ad fixes

[PATCH net-next 0/5] bonding: various 802.3ad fixes

[Jonathan Toppins]

This patch series is a forward porting of patches we (Cumulus) are shipping
in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
more predictable in certian state machine transtions in addition to enhancing
802.3ad bond carrier determination based on acutal number of peered ports plus
if the bond has an active aggregrator. Specific notes are contained within each
patch.

For this patch series there are no userspace facing changes, a diff between
the modinfo output showed no difference. However, there are behavioral
facing changes, primarily in the bond carrier state. Please make sure to
review carefully.

Jonathan Toppins (1):
  bonding: cleanup and remove dead code

Satish Ashok (1):
  bonding: fix LACP PDU not sent on slave port sometimes

Scott Feldman (1):
  bonding: keep bond interface carrier off until at least one active
    member

Wilson Kok (2):
  bonding: fix bond_open() don't always set slave active flag
  bonding: fix incorrect lacp mux state when agg not active

 drivers/net/bonding/bond_3ad.c     |   73 ++++++++++++++++++++++++++++--------
 drivers/net/bonding/bond_main.c    |    6 +--
 drivers/net/bonding/bond_options.c |    1 +
 include/net/bond_3ad.h             |    1 -
 include/net/bonding.h              |    1 +
 5 files changed, 62 insertions(+), 20 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jonathan Toppins]

From: Scott Feldman <sfeldma@cumulusnetworks.com>

Bonding driver parameter min_links is now used to signal upper-level
protocols of bond status. The way it works is if the total number of
active members in slaves drops below min_links, the bond link carrier
will go down, signaling upper levels that bond is inactive.  When active
members returns to >= min_links, bond link carrier will go up (RUNNING),
and protocols can resume.  When bond is carrier down, member ports are
in stp fwd state blocked (rather than normal disabled state), so
low-level ctrl protocols (LACP) can still get in and be processed by
bonding driver.

LACP will still do it's job while bond is carrier off, and if bond members
become active, bond carrier will be turned back on, signaling higher-level
protocols that bond is viable.

Suggested setting of min_links is 1, rather than the default of zero.
Using min_links=1 says that at least 1 slave must be active within bond
for bond to be carrier on.

Finally, when min_links bonding option is changed update carrier status.

Cc: Scott Feldman <sfeldma@gmail.com>
Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_3ad.c     |   18 ++++++++++++++----
 drivers/net/bonding/bond_main.c    |    2 +-
 drivers/net/bonding/bond_options.c |    1 +
 include/net/bonding.h              |    1 +
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 8baa87d..e9b706f 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
 static inline void __disable_port(struct port *port)
 {
 	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
+	bond_3ad_set_carrier(port->slave->bond);
 }
 
 /**
@@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port)
 {
 	struct slave *slave = port->slave;
 
-	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave))
+	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) {
 		bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER);
+		bond_3ad_set_carrier(slave->bond);
+	}
 }
 
 /**
@@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
 int bond_3ad_set_carrier(struct bonding *bond)
 {
 	struct aggregator *active;
-	struct slave *first_slave;
+	struct slave *first_slave, *slave;
+	struct list_head *iter;
 	int ret = 1;
+	int active_slaves = 0;
 
 	rcu_read_lock();
 	first_slave = bond_first_slave_rcu(bond);
@@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
 		ret = 0;
 		goto out;
 	}
+
+	bond_for_each_slave_rcu(bond, slave, iter)
+		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
+			active_slaves++;
+
 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
-	if (active) {
+	if (active && __agg_has_partner(active)) {
 		/* are enough slaves available to consider link up? */
-		if (active->num_of_ports < bond->params.min_links) {
+		if (active_slaves < bond->params.min_links) {
 			if (netif_carrier_ok(bond->dev)) {
 				netif_carrier_off(bond->dev);
 				goto out;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0dceba1..02ffedb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
  *
  * Returns zero if carrier state does not change, nonzero if it does.
  */
-static int bond_set_carrier(struct bonding *bond)
+int bond_set_carrier(struct bonding *bond)
 {
 	struct list_head *iter;
 	struct slave *slave;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9bd538d4..4df2894 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
 	netdev_info(bond->dev, "Setting min links value to %llu\n",
 		    newval->value);
 	bond->params.min_links = newval->value;
+	bond_set_carrier(bond);
 
 	return 0;
 }
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 983a94b..29f53ea 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave);
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
 u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
+int bond_set_carrier(struct bonding *bond);
 void bond_select_active_slave(struct bonding *bond);
 void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
 void bond_create_debugfs(void);
-- 
1.7.10.4

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Nikolay Aleksandrov]

On 01/16/2015 04:57 PM, Jonathan Toppins wrote:
> From: Scott Feldman <sfeldma@cumulusnetworks.com>
> 
> Bonding driver parameter min_links is now used to signal upper-level
> protocols of bond status. The way it works is if the total number of
> active members in slaves drops below min_links, the bond link carrier
> will go down, signaling upper levels that bond is inactive.  When active
> members returns to >= min_links, bond link carrier will go up (RUNNING),
> and protocols can resume.  When bond is carrier down, member ports are
> in stp fwd state blocked (rather than normal disabled state), so
> low-level ctrl protocols (LACP) can still get in and be processed by
> bonding driver.
> 
> LACP will still do it's job while bond is carrier off, and if bond members
> become active, bond carrier will be turned back on, signaling higher-level
> protocols that bond is viable.
> 
> Suggested setting of min_links is 1, rather than the default of zero.
> Using min_links=1 says that at least 1 slave must be active within bond
> for bond to be carrier on.
> 
> Finally, when min_links bonding option is changed update carrier status.
> 
> Cc: Scott Feldman <sfeldma@gmail.com>
> Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>  drivers/net/bonding/bond_3ad.c     |   18 ++++++++++++++----
>  drivers/net/bonding/bond_main.c    |    2 +-
>  drivers/net/bonding/bond_options.c |    1 +
>  include/net/bonding.h              |    1 +
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 8baa87d..e9b706f 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>  static inline void __disable_port(struct port *port)
>  {
>  	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
> +	bond_3ad_set_carrier(port->slave->bond);
>  }
>  
>  /**
> @@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port)
>  {
>  	struct slave *slave = port->slave;
>  
> -	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave))
> +	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) {
While at it please remove the extra ( ) around slave->link == BOND_LINK_UP.

>  		bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER);
> +		bond_3ad_set_carrier(slave->bond);
> +	}
>  }
>  
>  /**
> @@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>  int bond_3ad_set_carrier(struct bonding *bond)
>  {
>  	struct aggregator *active;
> -	struct slave *first_slave;
> +	struct slave *first_slave, *slave;
> +	struct list_head *iter;
>  	int ret = 1;
> +	int active_slaves = 0;
>  
>  	rcu_read_lock();
>  	first_slave = bond_first_slave_rcu(bond);
> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>  		ret = 0;
>  		goto out;
>  	}
> +
> +	bond_for_each_slave_rcu(bond, slave, iter)
> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
> +			active_slaves++;
> +
>  	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
> -	if (active) {
> +	if (active && __agg_has_partner(active)) {
>  		/* are enough slaves available to consider link up? */
> -		if (active->num_of_ports < bond->params.min_links) {
> +		if (active_slaves < bond->params.min_links) {
>  			if (netif_carrier_ok(bond->dev)) {
>  				netif_carrier_off(bond->dev);
>  				goto out;
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 0dceba1..02ffedb 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
>   *
>   * Returns zero if carrier state does not change, nonzero if it does.
>   */
> -static int bond_set_carrier(struct bonding *bond)
> +int bond_set_carrier(struct bonding *bond)
>  {
>  	struct list_head *iter;
>  	struct slave *slave;
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 9bd538d4..4df2894 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
>  	netdev_info(bond->dev, "Setting min links value to %llu\n",
>  		    newval->value);
>  	bond->params.min_links = newval->value;
> +	bond_set_carrier(bond);
>  
>  	return 0;
>  }
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 983a94b..29f53ea 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave);
>  int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
>  int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
>  u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
> +int bond_set_carrier(struct bonding *bond);
>  void bond_select_active_slave(struct bonding *bond);
>  void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
>  void bond_create_debugfs(void);
> 

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jonathan Toppins]

On 1/19/15 2:27 PM, Nikolay Aleksandrov wrote:
> On 01/16/2015 04:57 PM, Jonathan Toppins wrote:
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 8baa87d..e9b706f 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>>   static inline void __disable_port(struct port *port)
>>   {
>>   	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
>> +	bond_3ad_set_carrier(port->slave->bond);
>>   }
>>
>>   /**
>> @@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port)
>>   {
>>   	struct slave *slave = port->slave;
>>
>> -	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave))
>> +	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) {
> While at it please remove the extra ( ) around slave->link == BOND_LINK_UP.
>
Ack.

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>From: Scott Feldman <sfeldma@cumulusnetworks.com>
>
>Bonding driver parameter min_links is now used to signal upper-level
>protocols of bond status. The way it works is if the total number of
>active members in slaves drops below min_links, the bond link carrier
>will go down, signaling upper levels that bond is inactive.  When active
>members returns to >= min_links, bond link carrier will go up (RUNNING),
>and protocols can resume.  When bond is carrier down, member ports are
>in stp fwd state blocked (rather than normal disabled state), so
>low-level ctrl protocols (LACP) can still get in and be processed by
>bonding driver.

	Presuming that "stp" is Spanning Tree, is the last sentence
above actually describing the behavior of a bridge port when a bond is
the member of the bridge?  I'm not sure I understand what "member ports"
refers to (bridge ports or bonding slaves).

	One code comment, below...

>LACP will still do it's job while bond is carrier off, and if bond members
>become active, bond carrier will be turned back on, signaling higher-level
>protocols that bond is viable.
>
>Suggested setting of min_links is 1, rather than the default of zero.
>Using min_links=1 says that at least 1 slave must be active within bond
>for bond to be carrier on.
>
>Finally, when min_links bonding option is changed update carrier status.
>
>Cc: Scott Feldman <sfeldma@gmail.com>
>Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
>Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>---
> drivers/net/bonding/bond_3ad.c     |   18 ++++++++++++++----
> drivers/net/bonding/bond_main.c    |    2 +-
> drivers/net/bonding/bond_options.c |    1 +
> include/net/bonding.h              |    1 +
> 4 files changed, 17 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 8baa87d..e9b706f 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
> static inline void __disable_port(struct port *port)
> {
> 	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
>+	bond_3ad_set_carrier(port->slave->bond);
> }
> 
> /**
>@@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port)
> {
> 	struct slave *slave = port->slave;
> 
>-	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave))
>+	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) {
> 		bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER);
>+		bond_3ad_set_carrier(slave->bond);
>+	}
> }
> 
> /**
>@@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
> int bond_3ad_set_carrier(struct bonding *bond)
> {
> 	struct aggregator *active;
>-	struct slave *first_slave;
>+	struct slave *first_slave, *slave;
>+	struct list_head *iter;
> 	int ret = 1;
>+	int active_slaves = 0;
> 
> 	rcu_read_lock();
> 	first_slave = bond_first_slave_rcu(bond);
>@@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
> 		ret = 0;
> 		goto out;
> 	}
>+
>+	bond_for_each_slave_rcu(bond, slave, iter)
>+		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>+			active_slaves++;
>+
> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>-	if (active) {
>+	if (active && __agg_has_partner(active)) {

	Why "__agg_has_partner"?  Since the "else" of this clause is:

        } else if (netif_carrier_ok(bond->dev)) {
                netif_carrier_off(bond->dev);
        }

	I'm wondering if this will do the right thing for the case that
there are no LACP partners at all (e.g., the switch ports do not have
LACP enabled), in which case the active aggregator should be a single
"individual" port as a fallback, but will not have a partner.

	-J


> 		/* are enough slaves available to consider link up? */
>-		if (active->num_of_ports < bond->params.min_links) {
>+		if (active_slaves < bond->params.min_links) {
> 			if (netif_carrier_ok(bond->dev)) {
> 				netif_carrier_off(bond->dev);
> 				goto out;
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 0dceba1..02ffedb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
>  *
>  * Returns zero if carrier state does not change, nonzero if it does.
>  */
>-static int bond_set_carrier(struct bonding *bond)
>+int bond_set_carrier(struct bonding *bond)
> {
> 	struct list_head *iter;
> 	struct slave *slave;
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 9bd538d4..4df2894 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
> 	netdev_info(bond->dev, "Setting min links value to %llu\n",
> 		    newval->value);
> 	bond->params.min_links = newval->value;
>+	bond_set_carrier(bond);
> 
> 	return 0;
> }
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 983a94b..29f53ea 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave);
> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
> int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
> u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
>+int bond_set_carrier(struct bonding *bond);
> void bond_select_active_slave(struct bonding *bond);
> void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
> void bond_create_debugfs(void);
>-- 
>1.7.10.4

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

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jonathan Toppins]

On 1/19/15 4:16 PM, Jay Vosburgh wrote:
> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>
>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>
>> Bonding driver parameter min_links is now used to signal upper-level
>> protocols of bond status. The way it works is if the total number of
>> active members in slaves drops below min_links, the bond link carrier
>> will go down, signaling upper levels that bond is inactive.  When active
>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>> and protocols can resume.  When bond is carrier down, member ports are
>> in stp fwd state blocked (rather than normal disabled state), so
>> low-level ctrl protocols (LACP) can still get in and be processed by
>> bonding driver.
>
> 	Presuming that "stp" is Spanning Tree, is the last sentence
> above actually describing the behavior of a bridge port when a bond is
> the member of the bridge?  I'm not sure I understand what "member ports"
> refers to (bridge ports or bonding slaves).

Ack, maybe replacing the last sentence with something like:
   When bond is carrier down, the slave ports are only forwarding
   low-level control protocols (e.g. LACP PDU) and discarding all other
   packets.

>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>> 		ret = 0;
>> 		goto out;
>> 	}
>> +
>> +	bond_for_each_slave_rcu(bond, slave, iter)
>> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>> +			active_slaves++;
>> +
>> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>> -	if (active) {
>> +	if (active && __agg_has_partner(active)) {
>
> 	Why "__agg_has_partner"?  Since the "else" of this clause is:
>
>          } else if (netif_carrier_ok(bond->dev)) {
>                  netif_carrier_off(bond->dev);
>          }
>
> 	I'm wondering if this will do the right thing for the case that
> there are no LACP partners at all (e.g., the switch ports do not have
> LACP enabled), in which case the active aggregator should be a single
> "individual" port as a fallback, but will not have a partner.
>
> 	-J
>

I see your point. The initial thinking was the logical bond carrier 
should not be brought up until the bond has a partner and is ready to 
pass traffic, otherwise we start blackholing frames. Looking over the 
code it seems the aggregator.is_individual flag is only set to true when 
a slave is in half-duplex, this seems odd?

My initial thinking to alleviate the concern is something like the 
following:

if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
     __agg_has_partner(active)) {
     /* set carrier based on min_links */
} else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
     /* set bond carrier state according to carrier state of slave */
} else if (netif_carrier_ok(bond->dev)) {
     netif_carrier_off(bond->dev);
}

Maybe I am missing something and there is a simpler option.

Thinking about how to validate this, it seems having a bond with two 
slaves and both slaves in half-duplex will force an aggregator that is 
individual to be selected.

Thoughts?

-Jon

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>
>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>
>>> Bonding driver parameter min_links is now used to signal upper-level
>>> protocols of bond status. The way it works is if the total number of
>>> active members in slaves drops below min_links, the bond link carrier
>>> will go down, signaling upper levels that bond is inactive.  When active
>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>> and protocols can resume.  When bond is carrier down, member ports are
>>> in stp fwd state blocked (rather than normal disabled state), so
>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>> bonding driver.
>>
>> 	Presuming that "stp" is Spanning Tree, is the last sentence
>> above actually describing the behavior of a bridge port when a bond is
>> the member of the bridge?  I'm not sure I understand what "member ports"
>> refers to (bridge ports or bonding slaves).
>
>Ack, maybe replacing the last sentence with something like:
>  When bond is carrier down, the slave ports are only forwarding
>  low-level control protocols (e.g. LACP PDU) and discarding all other
>  packets.

	Ah, are you actually referring to the fact that slaves that are
up will still deliver packets to listeners that bind directly to the
slave or hook in through a rx_handler?  This is, in part, the
"RX_HANDLER_EXACT" business in bond_handle_frame and
__netif_receive_skb_core.

	The decision for that has nothing to do with the protocol; I
seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
reception this way (although via dev_add_pack, not an rx_handler) so it
can run traffic regardless of the bonding master's state.

>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>> 		ret = 0;
>>> 		goto out;
>>> 	}
>>> +
>>> +	bond_for_each_slave_rcu(bond, slave, iter)
>>> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>> +			active_slaves++;
>>> +
>>> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>> -	if (active) {
>>> +	if (active && __agg_has_partner(active)) {
>>
>> 	Why "__agg_has_partner"?  Since the "else" of this clause is:
>>
>>          } else if (netif_carrier_ok(bond->dev)) {
>>                  netif_carrier_off(bond->dev);
>>          }
>>
>> 	I'm wondering if this will do the right thing for the case that
>> there are no LACP partners at all (e.g., the switch ports do not have
>> LACP enabled), in which case the active aggregator should be a single
>> "individual" port as a fallback, but will not have a partner.
>>
>> 	-J
>>
>
>I see your point. The initial thinking was the logical bond carrier should
>not be brought up until the bond has a partner and is ready to pass
>traffic, otherwise we start blackholing frames. Looking over the code it
>seems the aggregator.is_individual flag is only set to true when a slave
>is in half-duplex, this seems odd?

	The agg.is_individual flag and an "individual" aggregator are
subtly different things.  

	The is_individual flag is part of the implementation of the
standard requirement that half duplex ports are not allowed to enable
LACP (and thus cannot aggregate, and end up as "Individual" in
standard-ese).  The standard capitalizes "Individual" when it describes
the cannot-aggregate property of a port (note that half duplex is only
one reason of many for a port being Individual).

	An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
aggregator containing exactly one port that is Individual.  A port can
end up as Individual (for purposes of this discussion) either through
the is_individual business, or because the bonding port does run LACP,
but the link partner does not, and thus no LACPDUs are ever received.

	For either of the above cases (is_individual or no-LACP-parter),
then the active aggregator will be an "individual" aggregator, but will
not have a parter (__agg_has_partner() will be false).  The standard has
a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.

>My initial thinking to alleviate the concern is something like the
>following:
>
>if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
>    __agg_has_partner(active)) {
>    /* set carrier based on min_links */
>} else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
>    /* set bond carrier state according to carrier state of slave */
>} else if (netif_carrier_ok(bond->dev)) {
>    netif_carrier_off(bond->dev);
>}

	I'm not sure you need to care about is_individual or
__agg_has_partner at all.  If either of those conditions is true for the
active aggregator, it will contain exactly one port, and so if min_links
is 2, you'll have carrier off, and if min_links is 1 or less you'll have
carrier on.

	If I'm reading the patch right, the real point (which isn't
really described very well in the change log) is that you're changing
the carrier decision to count only active ports in the active
aggregator, not the total number of ports as is currently done.

	I'm not sure why this change is needed:

@@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
 		ret = 0;
 		goto out;
 	}
+
+	bond_for_each_slave_rcu(bond, slave, iter)
+		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
+			active_slaves++;
+
 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
-	if (active) {
+	if (active && __agg_has_partner(active)) {
 		/* are enough slaves available to consider link up? */
-		if (active->num_of_ports < bond->params.min_links) {
+		if (active_slaves < bond->params.min_links) {
 			if (netif_carrier_ok(bond->dev)) {
 				netif_carrier_off(bond->dev);
 				goto out;

	because a port (slave) that loses carrier or whose link partner
becomes unresponsive to LACPDUs will be removed from the aggregator.  As
I recall, there are no "inactive" ports in an aggregator; all of them
have to match in terms of capabilities.

	In other words, I'm unsure of when the count of is_active ports
will not match active->num_of_ports.

	Also, the other parts of the patch add some extra updates to the
carrier state when a port is enabled or disabled, e.g.,

@@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
 static inline void __disable_port(struct port *port)
 {
 	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
+	bond_3ad_set_carrier(port->slave->bond);
 }

	Again, I'm not sure why this is necessary, as the cases that
disable or enable a port will eventually call bond_3ad_set_carrier.  For
example, ad_agg_selection_logic will, when changing active aggregator,
individually disable all ports of the old active and then may
individually enable ports of the new active if necessary, and then
finally call bond_3ad_set_carrier.

	In what situations is the patch's behavior an improvement (i.e.,
is there a situation I'm missing that doesn't do it right)?

	The last portion of the patch:

--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
 	netdev_info(bond->dev, "Setting min links value to %llu\n",
 		    newval->value);
 	bond->params.min_links = newval->value;
+	bond_set_carrier(bond);
 
 	return 0;
 }

	does seem to fix a legitimate bug, in that when min_links is
changed, it does not take effect in real time.

>Maybe I am missing something and there is a simpler option.
>
>Thinking about how to validate this, it seems having a bond with two
>slaves and both slaves in half-duplex will force an aggregator that is
>individual to be selected.
>
>Thoughts?

	That's one way, yes.  You'll also get an "individual" aggregator
if none of the link partners enable LACP.

	-J

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

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jonathan Toppins]

On 1/21/15 2:14 AM, Jay Vosburgh wrote:
> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>
>> On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>>
>>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>>
>>>> Bonding driver parameter min_links is now used to signal upper-level
>>>> protocols of bond status. The way it works is if the total number of
>>>> active members in slaves drops below min_links, the bond link carrier
>>>> will go down, signaling upper levels that bond is inactive.  When active
>>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>>> and protocols can resume.  When bond is carrier down, member ports are
>>>> in stp fwd state blocked (rather than normal disabled state), so
>>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>>> bonding driver.
>>>
>>> 	Presuming that "stp" is Spanning Tree, is the last sentence
>>> above actually describing the behavior of a bridge port when a bond is
>>> the member of the bridge?  I'm not sure I understand what "member ports"
>>> refers to (bridge ports or bonding slaves).
>>
>> Ack, maybe replacing the last sentence with something like:
>>   When bond is carrier down, the slave ports are only forwarding
>>   low-level control protocols (e.g. LACP PDU) and discarding all other
>>   packets.
>
> 	Ah, are you actually referring to the fact that slaves that are
> up will still deliver packets to listeners that bind directly to the
> slave or hook in through a rx_handler?  This is, in part, the
> "RX_HANDLER_EXACT" business in bond_handle_frame and
> __netif_receive_skb_core.
>
> 	The decision for that has nothing to do with the protocol; I
> seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
> reception this way (although via dev_add_pack, not an rx_handler) so it
> can run traffic regardless of the bonding master's state.

I see, it seems you are basically saying; the slaves are up but when the 
logical bond interface is carrier down there was no code changed in 
bond_handle_frame() to actually drop frames other than LACPDUs. So 
basically having this statement makes no sense until there is code to 
actually drop those additional frames.

>
>>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>>> 		ret = 0;
>>>> 		goto out;
>>>> 	}
>>>> +
>>>> +	bond_for_each_slave_rcu(bond, slave, iter)
>>>> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>>> +			active_slaves++;
>>>> +
>>>> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>>> -	if (active) {
>>>> +	if (active && __agg_has_partner(active)) {
>>>
>>> 	Why "__agg_has_partner"?  Since the "else" of this clause is:
>>>
>>>           } else if (netif_carrier_ok(bond->dev)) {
>>>                   netif_carrier_off(bond->dev);
>>>           }
>>>
>>> 	I'm wondering if this will do the right thing for the case that
>>> there are no LACP partners at all (e.g., the switch ports do not have
>>> LACP enabled), in which case the active aggregator should be a single
>>> "individual" port as a fallback, but will not have a partner.
>>>
>>> 	-J
>>>
>>
>> I see your point. The initial thinking was the logical bond carrier should
>> not be brought up until the bond has a partner and is ready to pass
>> traffic, otherwise we start blackholing frames. Looking over the code it
>> seems the aggregator.is_individual flag is only set to true when a slave
>> is in half-duplex, this seems odd?
>
> 	The agg.is_individual flag and an "individual" aggregator are
> subtly different things.
>
> 	The is_individual flag is part of the implementation of the
> standard requirement that half duplex ports are not allowed to enable
> LACP (and thus cannot aggregate, and end up as "Individual" in
> standard-ese).  The standard capitalizes "Individual" when it describes
> the cannot-aggregate property of a port (note that half duplex is only
> one reason of many for a port being Individual).
>
> 	An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
> aggregator containing exactly one port that is Individual.  A port can
> end up as Individual (for purposes of this discussion) either through
> the is_individual business, or because the bonding port does run LACP,
> but the link partner does not, and thus no LACPDUs are ever received.
>
> 	For either of the above cases (is_individual or no-LACP-parter),
> then the active aggregator will be an "individual" aggregator, but will
> not have a parter (__agg_has_partner() will be false).  The standard has
> a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.
>
>> My initial thinking to alleviate the concern is something like the
>> following:
>>
>> if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
>>     __agg_has_partner(active)) {
>>     /* set carrier based on min_links */
>> } else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
>>     /* set bond carrier state according to carrier state of slave */
>> } else if (netif_carrier_ok(bond->dev)) {
>>     netif_carrier_off(bond->dev);
>> }
>
> 	I'm not sure you need to care about is_individual or
> __agg_has_partner at all.  If either of those conditions is true for the
> active aggregator, it will contain exactly one port, and so if min_links
> is 2, you'll have carrier off, and if min_links is 1 or less you'll have
> carrier on.
>
> 	If I'm reading the patch right, the real point (which isn't
> really described very well in the change log) is that you're changing
> the carrier decision to count only active ports in the active
> aggregator, not the total number of ports as is currently done.
>
> 	I'm not sure why this change is needed:
>
> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>   		ret = 0;
>   		goto out;
>   	}
> +
> +	bond_for_each_slave_rcu(bond, slave, iter)
> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
> +			active_slaves++;
> +
>   	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
> -	if (active) {
> +	if (active && __agg_has_partner(active)) {
>   		/* are enough slaves available to consider link up? */
> -		if (active->num_of_ports < bond->params.min_links) {
> +		if (active_slaves < bond->params.min_links) {
>   			if (netif_carrier_ok(bond->dev)) {
>   				netif_carrier_off(bond->dev);
>   				goto out;
>
> 	because a port (slave) that loses carrier or whose link partner
> becomes unresponsive to LACPDUs will be removed from the aggregator.  As
> I recall, there are no "inactive" ports in an aggregator; all of them
> have to match in terms of capabilities.
>
> 	In other words, I'm unsure of when the count of is_active ports
> will not match active->num_of_ports.
>
> 	Also, the other parts of the patch add some extra updates to the
> carrier state when a port is enabled or disabled, e.g.,
>
> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>   static inline void __disable_port(struct port *port)
>   {
>   	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
> +	bond_3ad_set_carrier(port->slave->bond);
>   }
>
> 	Again, I'm not sure why this is necessary, as the cases that
> disable or enable a port will eventually call bond_3ad_set_carrier.  For
> example, ad_agg_selection_logic will, when changing active aggregator,
> individually disable all ports of the old active and then may
> individually enable ports of the new active if necessary, and then
> finally call bond_3ad_set_carrier.
>
> 	In what situations is the patch's behavior an improvement (i.e.,
> is there a situation I'm missing that doesn't do it right)?

I think the addition of bond_3ad_set_carrier() to both __enable_port() 
and __disable_port() were optimizations so the bond carrier transition 
would happen faster, though I am not certain.

>
> 	The last portion of the patch:
>
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
>   	netdev_info(bond->dev, "Setting min links value to %llu\n",
>   		    newval->value);
>   	bond->params.min_links = newval->value;
> +	bond_set_carrier(bond);
>
>   	return 0;
>   }
>
> 	does seem to fix a legitimate bug, in that when min_links is
> changed, it does not take effect in real time.
>
>> Maybe I am missing something and there is a simpler option.
>>
>> Thinking about how to validate this, it seems having a bond with two
>> slaves and both slaves in half-duplex will force an aggregator that is
>> individual to be selected.
>>
>> Thoughts?
>
> 	That's one way, yes.  You'll also get an "individual" aggregator
> if none of the link partners enable LACP.
>

It seems it might be better to drop the changes to __enable/disable_port 
and bond_3ad_set_carrier from this patch until more testing can be done 
from me, especially if you agree the other changes in this series are of 
benefit.

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>On 1/21/15 2:14 AM, Jay Vosburgh wrote:
>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>
>>> On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>>>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>>>
>>>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>>>
>>>>> Bonding driver parameter min_links is now used to signal upper-level
>>>>> protocols of bond status. The way it works is if the total number of
>>>>> active members in slaves drops below min_links, the bond link carrier
>>>>> will go down, signaling upper levels that bond is inactive.  When active
>>>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>>>> and protocols can resume.  When bond is carrier down, member ports are
>>>>> in stp fwd state blocked (rather than normal disabled state), so
>>>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>>>> bonding driver.
>>>>
>>>> 	Presuming that "stp" is Spanning Tree, is the last sentence
>>>> above actually describing the behavior of a bridge port when a bond is
>>>> the member of the bridge?  I'm not sure I understand what "member ports"
>>>> refers to (bridge ports or bonding slaves).
>>>
>>> Ack, maybe replacing the last sentence with something like:
>>>   When bond is carrier down, the slave ports are only forwarding
>>>   low-level control protocols (e.g. LACP PDU) and discarding all other
>>>   packets.
>>
>> 	Ah, are you actually referring to the fact that slaves that are
>> up will still deliver packets to listeners that bind directly to the
>> slave or hook in through a rx_handler?  This is, in part, the
>> "RX_HANDLER_EXACT" business in bond_handle_frame and
>> __netif_receive_skb_core.
>>
>> 	The decision for that has nothing to do with the protocol; I
>> seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
>> reception this way (although via dev_add_pack, not an rx_handler) so it
>> can run traffic regardless of the bonding master's state.
>
>I see, it seems you are basically saying; the slaves are up but when the
>logical bond interface is carrier down there was no code changed in
>bond_handle_frame() to actually drop frames other than LACPDUs. So
>basically having this statement makes no sense until there is code to
>actually drop those additional frames.

	What I'm saying is that the fact that bond_handle_frame() does
not outright drop anything is a feature, not an oversight.  It's done
this way on purpose so that the slave device can be utilized separately
from its participation in the bond.  Off the top of my head, as I recall
this is used by LLDP, FCoE, and probably other "converged" Ethernet
facilities.  I believe some network monitoring tools use this property
as well, but I don't recall the details.

	Frames received on inactive slaves (which these active slaves
under a carrier-off bond are not; more on that in a bit) are marked as
such by the RX_HANDLER_EXACT return, and do not propagate up the stack
in the usual way, but are delivered only to packet listeners that bind
directly to the slave (typically via a dev_add_pack handler, which is
how LACPDUs were received prior to the rx_handler logic being
implemented).

	The bonding inactive slave receive logic used to work the other
way around; anything not explicitly needed by the bond itself would be
dropped in this particular case.  The "deliver to exact match" logic was
added later.

	Now, the possible hole here that I think you're alluding to is
that if the bond sets itself carrier down due to a min_links violation,
the slaves in the active aggregator are not inactive, and there's
nothing in the receive path to prevent incoming packets from being
processed normally if a receiving slave is still up.

	A quick test suggests that this is indeed the case; if I set up
a 802.3ad bond with min_links=3 and two slaves, incoming ICMP ECHOs to
the bond's IP appear to be received by icmp_echo, which calls
icmp_reply, which apparently fails.  I'm basing this conclusion on the
IcmpInErrors, IcmpInEchoReps, IcmpOutErrors, IcmpMsgInType8 and
IcmpMsgOutType0 stat counters all incrementing more or less in lock
step.  I haven't traced the code to see where it fails.

	I'm not sure exactly what ought to be done in that case; one
thought (which I have not tested) is bond_should_deliver_exact_match()
always returns true if the bonding master is carrier down.  

	Transmit appears to not function (at the bond level), so that
part doesn't appear to be an issue.

>>>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>>>> 		ret = 0;
>>>>> 		goto out;
>>>>> 	}
>>>>> +
>>>>> +	bond_for_each_slave_rcu(bond, slave, iter)
>>>>> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>>>> +			active_slaves++;
>>>>> +
>>>>> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>>>> -	if (active) {
>>>>> +	if (active && __agg_has_partner(active)) {
>>>>
>>>> 	Why "__agg_has_partner"?  Since the "else" of this clause is:
>>>>
>>>>           } else if (netif_carrier_ok(bond->dev)) {
>>>>                   netif_carrier_off(bond->dev);
>>>>           }
>>>>
>>>> 	I'm wondering if this will do the right thing for the case that
>>>> there are no LACP partners at all (e.g., the switch ports do not have
>>>> LACP enabled), in which case the active aggregator should be a single
>>>> "individual" port as a fallback, but will not have a partner.
>>>>
>>>> 	-J
>>>>
>>>
>>> I see your point. The initial thinking was the logical bond carrier should
>>> not be brought up until the bond has a partner and is ready to pass
>>> traffic, otherwise we start blackholing frames. Looking over the code it
>>> seems the aggregator.is_individual flag is only set to true when a slave
>>> is in half-duplex, this seems odd?
>>
>> 	The agg.is_individual flag and an "individual" aggregator are
>> subtly different things.
>>
>> 	The is_individual flag is part of the implementation of the
>> standard requirement that half duplex ports are not allowed to enable
>> LACP (and thus cannot aggregate, and end up as "Individual" in
>> standard-ese).  The standard capitalizes "Individual" when it describes
>> the cannot-aggregate property of a port (note that half duplex is only
>> one reason of many for a port being Individual).
>>
>> 	An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
>> aggregator containing exactly one port that is Individual.  A port can
>> end up as Individual (for purposes of this discussion) either through
>> the is_individual business, or because the bonding port does run LACP,
>> but the link partner does not, and thus no LACPDUs are ever received.
>>
>> 	For either of the above cases (is_individual or no-LACP-parter),
>> then the active aggregator will be an "individual" aggregator, but will
>> not have a parter (__agg_has_partner() will be false).  The standard has
>> a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.
>>
>>> My initial thinking to alleviate the concern is something like the
>>> following:
>>>
>>> if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
>>>     __agg_has_partner(active)) {
>>>     /* set carrier based on min_links */
>>> } else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
>>>     /* set bond carrier state according to carrier state of slave */
>>> } else if (netif_carrier_ok(bond->dev)) {
>>>     netif_carrier_off(bond->dev);
>>> }
>>
>> 	I'm not sure you need to care about is_individual or
>> __agg_has_partner at all.  If either of those conditions is true for the
>> active aggregator, it will contain exactly one port, and so if min_links
>> is 2, you'll have carrier off, and if min_links is 1 or less you'll have
>> carrier on.
>>
>> 	If I'm reading the patch right, the real point (which isn't
>> really described very well in the change log) is that you're changing
>> the carrier decision to count only active ports in the active
>> aggregator, not the total number of ports as is currently done.
>>
>> 	I'm not sure why this change is needed:
>>
>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>   		ret = 0;
>>   		goto out;
>>   	}
>> +
>> +	bond_for_each_slave_rcu(bond, slave, iter)
>> +		if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>> +			active_slaves++;
>> +
>>   	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>> -	if (active) {
>> +	if (active && __agg_has_partner(active)) {
>>   		/* are enough slaves available to consider link up? */
>> -		if (active->num_of_ports < bond->params.min_links) {
>> +		if (active_slaves < bond->params.min_links) {
>>   			if (netif_carrier_ok(bond->dev)) {
>>   				netif_carrier_off(bond->dev);
>>   				goto out;
>>
>> 	because a port (slave) that loses carrier or whose link partner
>> becomes unresponsive to LACPDUs will be removed from the aggregator.  As
>> I recall, there are no "inactive" ports in an aggregator; all of them
>> have to match in terms of capabilities.
>>
>> 	In other words, I'm unsure of when the count of is_active ports
>> will not match active->num_of_ports.
>>
>> 	Also, the other parts of the patch add some extra updates to the
>> carrier state when a port is enabled or disabled, e.g.,
>>
>> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>>   static inline void __disable_port(struct port *port)
>>   {
>>   	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
>> +	bond_3ad_set_carrier(port->slave->bond);
>>   }
>>
>> 	Again, I'm not sure why this is necessary, as the cases that
>> disable or enable a port will eventually call bond_3ad_set_carrier.  For
>> example, ad_agg_selection_logic will, when changing active aggregator,
>> individually disable all ports of the old active and then may
>> individually enable ports of the new active if necessary, and then
>> finally call bond_3ad_set_carrier.
>>
>> 	In what situations is the patch's behavior an improvement (i.e.,
>> is there a situation I'm missing that doesn't do it right)?
>
>I think the addition of bond_3ad_set_carrier() to both __enable_port() and
>__disable_port() were optimizations so the bond carrier transition would
>happen faster, though I am not certain.

	I get the impression that these patches have been around for a
while internally; it would be good to validate that there is an actual
performance change in the current mainline, since there does not seem to
be any functionality change.

>>
>> 	The last portion of the patch:
>>
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
>>   	netdev_info(bond->dev, "Setting min links value to %llu\n",
>>   		    newval->value);
>>   	bond->params.min_links = newval->value;
>> +	bond_set_carrier(bond);
>>
>>   	return 0;
>>   }
>>
>> 	does seem to fix a legitimate bug, in that when min_links is
>> changed, it does not take effect in real time.
>>
>>> Maybe I am missing something and there is a simpler option.
>>>
>>> Thinking about how to validate this, it seems having a bond with two
>>> slaves and both slaves in half-duplex will force an aggregator that is
>>> individual to be selected.
>>>
>>> Thoughts?
>>
>> 	That's one way, yes.  You'll also get an "individual" aggregator
>> if none of the link partners enable LACP.
>>
>
>It seems it might be better to drop the changes to __enable/disable_port
>and bond_3ad_set_carrier from this patch until more testing can be done
>from me, especially if you agree the other changes in this series are of
>benefit.

	I haven't looked at patch 3 in detail yet; patches 2, 4 and 5
appear to be ok.

	For this patch, the patch fragment immediately above (min_links
take effect immediately) looks good.  I think the rest of it still needs
some evaluation.

	-J

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

Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member

[Jonathan Toppins]

On 1/19/15 5:15 PM, Scott Feldman wrote:
>
> On Jan 16, 2015 7:57 AM, "Jonathan Toppins"
> <jtoppins@cumulusnetworks.com <mailto:jtoppins@cumulusnetworks.com>> wrote:
>  >
>  > From: Scott Feldman <sfeldma@cumulusnetworks.com
> <mailto:sfeldma@cumulusnetworks.com>>
>
> Hmmmm, this feels a little creepy, sending a patch from my past.  I do
> appreciate the attribution, but I can't ACK or NACK or even comment as
> I've lost the context around this patch.  And I'm afraid I'll break out
> in hives if I look at the bonding code again, so if you don't mind,
> please remove me from this patch.  Thank you.
>

Have removed you for v2 of series, should we consider this a blanket 
statement for any other patches we might be releasing that you had a 
hand in? There were a few ;)

[PATCH net-next 2/5] bonding: fix bond_open() don't always set slave active flag

[Jonathan Toppins]

From: Wilson Kok <wkok@cumulusnetworks.com>

Mode 802.3ad, fix incorrect bond slave active state when slave is not in
active aggregator. During bond_open(), the bonding driver always sets
the slave active flag to true if the bond is not in active-backup, alb,
or tlb modes. Bonding should let the aggregator selection logic set the
active flag when in 802.3ad mode.

Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 02ffedb..c475d90 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3066,7 +3066,7 @@ static int bond_open(struct net_device *bond_dev)
 			    slave != rcu_access_pointer(bond->curr_active_slave)) {
 				bond_set_slave_inactive_flags(slave,
 							      BOND_SLAVE_NOTIFY_NOW);
-			} else {
+			} else if (BOND_MODE(bond) != BOND_MODE_8023AD) {
 				bond_set_slave_active_flags(slave,
 							    BOND_SLAVE_NOTIFY_NOW);
 			}
-- 
1.7.10.4

[PATCH net-next 3/5] bonding: fix incorrect lacp mux state when agg not active

[Jonathan Toppins]

From: Wilson Kok <wkok@cumulusnetworks.com>

This patch attempts to fix the following problems when an actor or
partner's aggregator is not active:
    1. a slave's lacp port state is marked as AD_STATE_SYNCHRONIZATION
       even if it is attached to an inactive aggregator. LACP advertises
       this state to the partner, making the partner think he can move
       into COLLECTING_DISTRIBUTING state even though this link will not
       pass traffic on the local side

    2. a slave goes into COLLECTING_DISTRIBUTING state without checking
       if the aggregator is actually active

    3. when in COLLECTING_DISTRIBUTING state, the partner parameters may
       change, e.g. the partner_oper_port_state.SYNCHRONIZATION. The
       local mux machine is not reacting to the change and continue to
       keep the slave and bond up

    4. When bond slave leaves an inactive aggregator and joins an active
       aggregator, the actor oper port state need to update to SYNC state.

Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_3ad.c |   44 ++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index e9b706f..52a8772 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -471,10 +471,13 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
 		 * and the port is matched
 		 */
 		if ((port->sm_vars & AD_PORT_MATCHED)
-		    && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
+			&& (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION)) {
 			partner->port_state |= AD_STATE_SYNCHRONIZATION;
-		else
+			pr_debug("%s partner sync=1\n", port->slave->dev->name);
+		} else {
 			partner->port_state &= ~AD_STATE_SYNCHRONIZATION;
+			pr_debug("%s partner sync=0\n", port->slave->dev->name);
+		}
 	}
 }
 
@@ -729,6 +732,8 @@ static inline void __update_lacpdu_from_port(struct port *port)
 	lacpdu->actor_port_priority = htons(port->actor_port_priority);
 	lacpdu->actor_port = htons(port->actor_port_number);
 	lacpdu->actor_state = port->actor_oper_port_state;
+	pr_debug("update lacpdu: %s, actor port state %x\n",
+		 port->slave->dev->name, port->actor_oper_port_state);
 
 	/* lacpdu->reserved_3_1              initialized
 	 * lacpdu->tlv_type_partner_info     initialized
@@ -901,7 +906,9 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 			if ((port->sm_vars & AD_PORT_SELECTED) &&
 			    (port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION) &&
 			    !__check_agg_selection_timer(port)) {
-				port->sm_mux_state = AD_MUX_COLLECTING_DISTRIBUTING;
+				if (port->aggregator->is_active)
+					port->sm_mux_state =
+					    AD_MUX_COLLECTING_DISTRIBUTING;
 			} else if (!(port->sm_vars & AD_PORT_SELECTED) ||
 				   (port->sm_vars & AD_PORT_STANDBY)) {
 				/* if UNSELECTED or STANDBY */
@@ -913,12 +920,18 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 				 */
 				__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
 				port->sm_mux_state = AD_MUX_DETACHED;
+			} else if (port->aggregator->is_active) {
+				port->actor_oper_port_state |=
+				    AD_STATE_SYNCHRONIZATION;
 			}
 			break;
 		case AD_MUX_COLLECTING_DISTRIBUTING:
 			if (!(port->sm_vars & AD_PORT_SELECTED) ||
 			    (port->sm_vars & AD_PORT_STANDBY) ||
-			    !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION)) {
+			    !(port->partner_oper.port_state &
+				AD_STATE_SYNCHRONIZATION) ||
+			    !(port->actor_oper_port_state &
+				AD_STATE_SYNCHRONIZATION)) {
 				port->sm_mux_state = AD_MUX_ATTACHED;
 			} else {
 				/* if port state hasn't changed make
@@ -940,8 +953,10 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 
 	/* check if the state machine was changed */
 	if (port->sm_mux_state != last_state) {
-		pr_debug("Mux Machine: Port=%d, Last State=%d, Curr State=%d\n",
-			 port->actor_port_number, last_state,
+		pr_debug("Mux Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
+			 port->actor_port_number,
+			 port->slave->dev->name,
+			 last_state,
 			 port->sm_mux_state);
 		switch (port->sm_mux_state) {
 		case AD_MUX_DETACHED:
@@ -956,7 +971,12 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 			port->sm_mux_timer_counter = __ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
 			break;
 		case AD_MUX_ATTACHED:
-			port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
+			if (port->aggregator->is_active)
+				port->actor_oper_port_state |=
+				    AD_STATE_SYNCHRONIZATION;
+			else
+				port->actor_oper_port_state &=
+				    ~AD_STATE_SYNCHRONIZATION;
 			port->actor_oper_port_state &= ~AD_STATE_COLLECTING;
 			port->actor_oper_port_state &= ~AD_STATE_DISTRIBUTING;
 			ad_disable_collecting_distributing(port,
@@ -966,6 +986,7 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 		case AD_MUX_COLLECTING_DISTRIBUTING:
 			port->actor_oper_port_state |= AD_STATE_COLLECTING;
 			port->actor_oper_port_state |= AD_STATE_DISTRIBUTING;
+			port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
 			ad_enable_collecting_distributing(port,
 							  update_slave_arr);
 			port->ntt = true;
@@ -1047,8 +1068,10 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 
 	/* check if the State machine was changed or new lacpdu arrived */
 	if ((port->sm_rx_state != last_state) || (lacpdu)) {
-		pr_debug("Rx Machine: Port=%d, Last State=%d, Curr State=%d\n",
-			 port->actor_port_number, last_state,
+		pr_debug("Rx Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
+			 port->actor_port_number,
+			 port->slave->dev->name,
+			 last_state,
 			 port->sm_rx_state);
 		switch (port->sm_rx_state) {
 		case AD_RX_INITIALIZE:
@@ -1397,6 +1420,9 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
 
 	aggregator = __get_first_agg(port);
 	ad_agg_selection_logic(aggregator, update_slave_arr);
+
+	if (!port->aggregator->is_active)
+		port->actor_oper_port_state &= ~AD_STATE_SYNCHRONIZATION;
 }
 
 /* Decide if "agg" is a better choice for the new active aggregator that
-- 
1.7.10.4

Re: [PATCH net-next 3/5] bonding: fix incorrect lacp mux state when agg not active

[Nikolay Aleksandrov]

On 01/16/2015 04:57 PM, Jonathan Toppins wrote:
> From: Wilson Kok <wkok@cumulusnetworks.com>
> 
> This patch attempts to fix the following problems when an actor or
> partner's aggregator is not active:
>     1. a slave's lacp port state is marked as AD_STATE_SYNCHRONIZATION
>        even if it is attached to an inactive aggregator. LACP advertises
>        this state to the partner, making the partner think he can move
>        into COLLECTING_DISTRIBUTING state even though this link will not
>        pass traffic on the local side
> 
>     2. a slave goes into COLLECTING_DISTRIBUTING state without checking
>        if the aggregator is actually active
> 
>     3. when in COLLECTING_DISTRIBUTING state, the partner parameters may
>        change, e.g. the partner_oper_port_state.SYNCHRONIZATION. The
>        local mux machine is not reacting to the change and continue to
>        keep the slave and bond up
> 
>     4. When bond slave leaves an inactive aggregator and joins an active
>        aggregator, the actor oper port state need to update to SYNC state.
> 
> Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>  drivers/net/bonding/bond_3ad.c |   44 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index e9b706f..52a8772 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -471,10 +471,13 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
>  		 * and the port is matched
>  		 */
>  		if ((port->sm_vars & AD_PORT_MATCHED)
> -		    && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
> +			&& (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION)) {
In net/ it's preferred to have the logical operators at the end of the
previous line. It'd be nice if we start fixing these in bond_3ad.c since
they're being touched by the patch anyhow.

>  			partner->port_state |= AD_STATE_SYNCHRONIZATION;
> -		else
> +			pr_debug("%s partner sync=1\n", port->slave->dev->name);
> +		} else {
>  			partner->port_state &= ~AD_STATE_SYNCHRONIZATION;
> +			pr_debug("%s partner sync=0\n", port->slave->dev->name);
> +		}
>  	}
>  }
>  
> @@ -729,6 +732,8 @@ static inline void __update_lacpdu_from_port(struct port *port)
>  	lacpdu->actor_port_priority = htons(port->actor_port_priority);
>  	lacpdu->actor_port = htons(port->actor_port_number);
>  	lacpdu->actor_state = port->actor_oper_port_state;
> +	pr_debug("update lacpdu: %s, actor port state %x\n",
> +		 port->slave->dev->name, port->actor_oper_port_state);
>  
>  	/* lacpdu->reserved_3_1              initialized
>  	 * lacpdu->tlv_type_partner_info     initialized
> @@ -901,7 +906,9 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>  			if ((port->sm_vars & AD_PORT_SELECTED) &&
>  			    (port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION) &&
>  			    !__check_agg_selection_timer(port)) {
> -				port->sm_mux_state = AD_MUX_COLLECTING_DISTRIBUTING;
> +				if (port->aggregator->is_active)
> +					port->sm_mux_state =
> +					    AD_MUX_COLLECTING_DISTRIBUTING;
>  			} else if (!(port->sm_vars & AD_PORT_SELECTED) ||
>  				   (port->sm_vars & AD_PORT_STANDBY)) {
>  				/* if UNSELECTED or STANDBY */
> @@ -913,12 +920,18 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>  				 */
>  				__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
>  				port->sm_mux_state = AD_MUX_DETACHED;
> +			} else if (port->aggregator->is_active) {
> +				port->actor_oper_port_state |=
> +				    AD_STATE_SYNCHRONIZATION;
>  			}
>  			break;
>  		case AD_MUX_COLLECTING_DISTRIBUTING:
>  			if (!(port->sm_vars & AD_PORT_SELECTED) ||
>  			    (port->sm_vars & AD_PORT_STANDBY) ||
> -			    !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION)) {
> +			    !(port->partner_oper.port_state &
> +				AD_STATE_SYNCHRONIZATION) ||
> +			    !(port->actor_oper_port_state &
> +				AD_STATE_SYNCHRONIZATION)) {
IMO this one looks a bit confusing when broken up like that.

>  				port->sm_mux_state = AD_MUX_ATTACHED;
>  			} else {
>  				/* if port state hasn't changed make
> @@ -940,8 +953,10 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>  
>  	/* check if the state machine was changed */
>  	if (port->sm_mux_state != last_state) {
> -		pr_debug("Mux Machine: Port=%d, Last State=%d, Curr State=%d\n",
> -			 port->actor_port_number, last_state,
> +		pr_debug("Mux Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
> +			 port->actor_port_number,
> +			 port->slave->dev->name,
> +			 last_state,
>  			 port->sm_mux_state);
>  		switch (port->sm_mux_state) {
>  		case AD_MUX_DETACHED:
> @@ -956,7 +971,12 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>  			port->sm_mux_timer_counter = __ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
>  			break;
>  		case AD_MUX_ATTACHED:
> -			port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
> +			if (port->aggregator->is_active)
> +				port->actor_oper_port_state |=
> +				    AD_STATE_SYNCHRONIZATION;
> +			else
> +				port->actor_oper_port_state &=
> +				    ~AD_STATE_SYNCHRONIZATION;
>  			port->actor_oper_port_state &= ~AD_STATE_COLLECTING;
>  			port->actor_oper_port_state &= ~AD_STATE_DISTRIBUTING;
>  			ad_disable_collecting_distributing(port,
> @@ -966,6 +986,7 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>  		case AD_MUX_COLLECTING_DISTRIBUTING:
>  			port->actor_oper_port_state |= AD_STATE_COLLECTING;
>  			port->actor_oper_port_state |= AD_STATE_DISTRIBUTING;
> +			port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
>  			ad_enable_collecting_distributing(port,
>  							  update_slave_arr);
>  			port->ntt = true;
> @@ -1047,8 +1068,10 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
>  
>  	/* check if the State machine was changed or new lacpdu arrived */
>  	if ((port->sm_rx_state != last_state) || (lacpdu)) {
> -		pr_debug("Rx Machine: Port=%d, Last State=%d, Curr State=%d\n",
> -			 port->actor_port_number, last_state,
> +		pr_debug("Rx Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
> +			 port->actor_port_number,
> +			 port->slave->dev->name,
> +			 last_state,
>  			 port->sm_rx_state);
>  		switch (port->sm_rx_state) {
>  		case AD_RX_INITIALIZE:
> @@ -1397,6 +1420,9 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
>  
>  	aggregator = __get_first_agg(port);
>  	ad_agg_selection_logic(aggregator, update_slave_arr);
> +
> +	if (!port->aggregator->is_active)
> +		port->actor_oper_port_state &= ~AD_STATE_SYNCHRONIZATION;
>  }
>  
>  /* Decide if "agg" is a better choice for the new active aggregator that
> 

Re: [PATCH net-next 3/5] bonding: fix incorrect lacp mux state when agg not active

[Jonathan Toppins]

On 1/19/15 2:26 PM, Nikolay Aleksandrov wrote:
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index e9b706f..52a8772 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -471,10 +471,13 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
>>   		 * and the port is matched
>>   		 */
>>   		if ((port->sm_vars & AD_PORT_MATCHED)
>> -		    && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
>> +			&& (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION)) {
> In net/ it's preferred to have the logical operators at the end of the
> previous line. It'd be nice if we start fixing these in bond_3ad.c since
> they're being touched by the patch anyhow.

Ack, I prefer at the end too. Question, would it be acceptable to do the 
cleanup of the entire bond_3ad.c code in a separate patch? That way the 
fix vs. cleanup is clear.


>>   		case AD_MUX_COLLECTING_DISTRIBUTING:
>>   			if (!(port->sm_vars & AD_PORT_SELECTED) ||
>>   			    (port->sm_vars & AD_PORT_STANDBY) ||
>> -			    !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION)) {
>> +			    !(port->partner_oper.port_state &
>> +				AD_STATE_SYNCHRONIZATION) ||
>> +			    !(port->actor_oper_port_state &
>> +				AD_STATE_SYNCHRONIZATION)) {
> IMO this one looks a bit confusing when broken up like that.

Ack, it seems in this case making checkpatch.pl happy should be secondary.

Re: [PATCH net-next 3/5] bonding: fix incorrect lacp mux state when agg not active

[David Miller]

From: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Date: Mon, 19 Jan 2015 15:50:48 -0500

> On 1/19/15 2:26 PM, Nikolay Aleksandrov wrote:
>>> diff --git a/drivers/net/bonding/bond_3ad.c
>>> b/drivers/net/bonding/bond_3ad.c
>>> index e9b706f..52a8772 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -471,10 +471,13 @@ static void __record_pdu(struct lacpdu *lacpdu,
>>> struct port *port)
>>>   		 * and the port is matched
>>>   		 */
>>>   		if ((port->sm_vars & AD_PORT_MATCHED)
>>> -		    && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
>>> + && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION)) {
>> In net/ it's preferred to have the logical operators at the end of the
>> previous line. It'd be nice if we start fixing these in bond_3ad.c
>> since
>> they're being touched by the patch anyhow.
> 
> Ack, I prefer at the end too. Question, would it be acceptable to do
> the cleanup of the entire bond_3ad.c code in a separate patch? That
> way the fix vs. cleanup is clear.

If you're touching this line, fix it's style in-situ.

[PATCH net-next 4/5] bonding: fix LACP PDU not sent on slave port sometimes

[Jonathan Toppins]

From: Satish Ashok <sashok@cumulusnetworks.com>

When a slave is added to a bond and it is not in full duplex mode,
AD_PORT_LACP_ENABLED flag is cleared, due to this LACP PDU is not sent
on slave. When the duplex is changed to full, the flag needs to be set
to send LACP PDU.

Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_3ad.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 52a8772..43bb0b0 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2224,8 +2224,10 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave,
 		switch (lacpdu->subtype) {
 		case AD_TYPE_LACPDU:
 			ret = RX_HANDLER_CONSUMED;
-			netdev_dbg(slave->bond->dev, "Received LACPDU on port %d\n",
-				   port->actor_port_number);
+			netdev_dbg(slave->bond->dev,
+				   "Received LACPDU on port %d slave %s\n",
+				   port->actor_port_number,
+				   slave->dev->name);
 			/* Protect against concurrent state machines */
 			spin_lock(&slave->bond->mode_lock);
 			ad_rx_machine(lacpdu, port);
@@ -2317,7 +2319,10 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
 	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
 	port->actor_oper_port_key = port->actor_admin_port_key |=
 		__get_duplex(port);
-	netdev_dbg(slave->bond->dev, "Port %d changed duplex\n", port->actor_port_number);
+	netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n",
+		   port->actor_port_number, slave->dev->name);
+	if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
+		port->sm_vars |= AD_PORT_LACP_ENABLED;
 	/* there is no need to reselect a new aggregator, just signal the
 	 * state machines to reinitialize
 	 */
-- 
1.7.10.4

[PATCH net-next 5/5] bonding: cleanup and remove dead code

[Jonathan Toppins]

fix sparse warning about non-static function

drivers/net/bonding/bond_main.c:3737:5: warning: symbol
'bond_3ad_xor_xmit' was not declared. Should it be static?

Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 include/net/bond_3ad.h          |    1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c475d90..67437f3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3734,7 +3734,7 @@ out:
  * usable slave array is formed in the control path. The xmit function
  * just calculates hash and sends the packet out.
  */
-int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
+static int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bonding *bond = netdev_priv(dev);
 	struct slave *slave;
diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
index e01d903..f04cdbb 100644
--- a/include/net/bond_3ad.h
+++ b/include/net/bond_3ad.h
@@ -274,7 +274,6 @@ void bond_3ad_handle_link_change(struct slave *slave, char link);
 int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
 int  __bond_3ad_get_active_agg_info(struct bonding *bond,
 				    struct ad_info *ad_info);
-int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
 int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
 			 struct slave *slave);
 int bond_3ad_set_carrier(struct bonding *bond);
-- 
1.7.10.4

Re: [PATCH net-next 0/5] bonding: various 802.3ad fixes

[Nikolay Aleksandrov]

On 01/16/2015 04:57 PM, Jonathan Toppins wrote:
> This patch series is a forward porting of patches we (Cumulus) are shipping
> in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
> more predictable in certian state machine transtions in addition to enhancing
> 802.3ad bond carrier determination based on acutal number of peered ports plus
> if the bond has an active aggregrator. Specific notes are contained within each
> patch.
> 
> For this patch series there are no userspace facing changes, a diff between
> the modinfo output showed no difference. However, there are behavioral
> facing changes, primarily in the bond carrier state. Please make sure to
> review carefully.
> 
> Jonathan Toppins (1):
>   bonding: cleanup and remove dead code
> 
> Satish Ashok (1):
>   bonding: fix LACP PDU not sent on slave port sometimes
> 
> Scott Feldman (1):
>   bonding: keep bond interface carrier off until at least one active
>     member
> 
> Wilson Kok (2):
>   bonding: fix bond_open() don't always set slave active flag
>   bonding: fix incorrect lacp mux state when agg not active
> 
>  drivers/net/bonding/bond_3ad.c     |   73 ++++++++++++++++++++++++++++--------
>  drivers/net/bonding/bond_main.c    |    6 +--
>  drivers/net/bonding/bond_options.c |    1 +
>  include/net/bond_3ad.h             |    1 -
>  include/net/bonding.h              |    1 +
>  5 files changed, 62 insertions(+), 20 deletions(-)
> 

Hi Jonathan,
For all patches you should also CC the other maintainers of the bonding -
Jay and Veaceslav, I'm adding them now just in case they've missed them.
I have a few cosmetic nits which I don't have a strong feeling about i.e.
if the others are okay with them - then ignore them, I've replied to the
patches directly, but other than that you can add for the set my:

Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>

Re: [PATCH net-next 0/5] bonding: various 802.3ad fixes

[Jonathan Toppins]

On 1/19/15 2:29 PM, Nikolay Aleksandrov wrote:
> On 01/16/2015 04:57 PM, Jonathan Toppins wrote:
>> This patch series is a forward porting of patches we (Cumulus) are shipping
>> in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
>> more predictable in certian state machine transtions in addition to enhancing
>> 802.3ad bond carrier determination based on acutal number of peered ports plus
>> if the bond has an active aggregrator. Specific notes are contained within each
>> patch.
>>
>> For this patch series there are no userspace facing changes, a diff between
>> the modinfo output showed no difference. However, there are behavioral
>> facing changes, primarily in the bond carrier state. Please make sure to
>> review carefully.
>>
>> Jonathan Toppins (1):
>>    bonding: cleanup and remove dead code
>>
>> Satish Ashok (1):
>>    bonding: fix LACP PDU not sent on slave port sometimes
>>
>> Scott Feldman (1):
>>    bonding: keep bond interface carrier off until at least one active
>>      member
>>
>> Wilson Kok (2):
>>    bonding: fix bond_open() don't always set slave active flag
>>    bonding: fix incorrect lacp mux state when agg not active
>>
>>   drivers/net/bonding/bond_3ad.c     |   73 ++++++++++++++++++++++++++++--------
>>   drivers/net/bonding/bond_main.c    |    6 +--
>>   drivers/net/bonding/bond_options.c |    1 +
>>   include/net/bond_3ad.h             |    1 -
>>   include/net/bonding.h              |    1 +
>>   5 files changed, 62 insertions(+), 20 deletions(-)
>>
>
> Hi Jonathan,
> For all patches you should also CC the other maintainers of the bonding -
> Jay and Veaceslav, I'm adding them now just in case they've missed them.
> I have a few cosmetic nits which I don't have a strong feeling about i.e.
> if the others are okay with them - then ignore them, I've replied to the
> patches directly, but other than that you can add for the set my:
>
> Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
>

Hi Nikolay,
Appreciate the review... looking over comments now. Apologies for not 
including other maintainers, will make sure to include them in v2 of 
patch series.

Re: [PATCH net-next 0/5] bonding: various 802.3ad fixes

[David Miller]

From: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Date: Fri, 16 Jan 2015 10:57:23 -0500

> This patch series is a forward porting of patches we (Cumulus) are shipping
> in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
> more predictable in certian state machine transtions in addition to enhancing
> 802.3ad bond carrier determination based on acutal number of peered ports plus
> if the bond has an active aggregrator. Specific notes are contained within each
> patch.
> 
> For this patch series there are no userspace facing changes, a diff between
> the modinfo output showed no difference. However, there are behavioral
> facing changes, primarily in the bond carrier state. Please make sure to
> review carefully.

There was some minor feedback given to you for this series, please address
it and repost the set.

Thanks.