Re: [PATCH]: bonding: Fix 802.3ad no carrier on "no partner found" instance

[Jay Vosburgh <fubar@us.ibm.com>]

Laurent Chavey <chavey@google.com> wrote:

>On 6/1/07, Jay Vosburgh <fubar@us.ibm.com> wrote:
[...]
>>         Prior to the change in question, the carrier state of the master
>> device was always on, regardless of the state of the slaves (so even if
>> things didn't work, bonding would claim to be up).
>
>looking at the code, this will only work if there is an actual active
>aggregator. If that is not the case, and as you mention there could
>be an active aggregator before completing all LACP pahses (on all the slaves)
>then we would need to add a check (not for the mac) but for the churned state.
>(I added a churned state machine that could be use there, I did not submit
>the patch yet)

	I'm not exactly sure what you mean, but I'm guessing that you're
saying that the current code won't work properly when there isn't an
active aggregator.  That's true, but the only times there isn't an
active aggregator is when there are no slaves, and temporarily during
802.3ad negoitation (if there are no 802.3ad responses, an aggregator is
chosen from the set after a timeout).  During those times, I believe it
to be correct for the bonding master to be down, as no transmission or
reception is possible.

	I'm not at all sure what you mean by a churned state, but I
don't think any additional state checks are needed here, for the
following reason.

	I checked the 802.3ad standard, and I believe that the switches
I'm testing against are not in compliance with the standard,
particularly the following:

43.3.9 Attaching a link to an Aggregator

Links that are not successful candidates for aggregation (e.g., links
that are attached to other devices that cannot perform aggregation or
links that have been manually configured to be non-aggregatable) are
enabled to operate as individual IEEE 802.3 links. For consistency of
modeling, such a link is regarded as being attached to a compatible
Aggregator that can only be associated with a single link. That is, from
the perspective of Link Aggregation, non-aggregated links are not a
special case; they compose an aggregation with a maximum membership of
one link.

	The switches I have (Cisco 2960, 2970) do not appear to enable
links that are not successful candidates for aggregation as individual
802.3 links, rather, those links are disabled entirely.  

	Anyway, in light of this, I don't see a problem with your patch,
I believe it would produce results in compliance with the standard.  It
may result in false "carrier up" for cases of, e.g., negotiation failure
with 802.3ad switches such as I test with, but the cause there appears
to be due to noncompliance on the switch.

	Can you update your patch to change the comments above
bond_3ad_set_carrier() to remove the word "partnered" and note that the
code is implementing compliance with section 43.3.9 of IEEE 802.3, and
then resubmit your patch with the proper "Signed-off-by" attribution?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com