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

[Jonathan Toppins <jtoppins@cumulusnetworks.com>]

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