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

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

[Laurent Chavey]

%diff -ru linux-2.6.21.3/drivers/net/bonding/bond_3ad.c
linux-2.6.21.3.new/drivers/net/bonding/bond_3ad.c
--- linux-2.6.21.3/drivers/net/bonding/bond_3ad.c       2007-05-24
14:22:47.000000000 -0700
+++ linux-2.6.21.3.new/drivers/net/bonding/bond_3ad.c   2007-06-01
09:28:07.000000000 -0700
@@ -2312,10 +2312,7 @@
  */
 int bond_3ad_set_carrier(struct bonding *bond)
 {
-       struct aggregator *agg;
-
-       agg = __get_active_agg(&(SLAVE_AD_INFO(bond->first_slave).aggregator));
-       if (agg && MAC_ADDRESS_COMPARE(&agg->partner_system, &null_mac_addr)) {
+       if (__get_active_agg(&(SLAVE_AD_INFO(bond->first_slave).aggregator))) {
                if (!netif_carrier_ok(bond->dev)) {
                        netif_carrier_on(bond->dev);
                        return 1;


On 5/31/07, Laurent Chavey <chavey@google.com> wrote:
> if a host configured with 802.3ad bond mode is connected to a switch
> that does not support 802.3ad,  then an aggregator is selected as the
> active aggregator (first link that has carrier in the slave list).
> This is perfectly fine, since it lets at least one of the link become active.
> (this was the behavior prior to 2.6.18)
>
> In 2.6.18 and above, a new check for the partner mac address was added
> before an aggregator's carrier is set on. If a host is  configured as
> previously
> described,  then no links will become active.
>
> is that the intended behavior ?
>
>
> -----
> in the scenario described here, the partner mac address is always set to NULL.
> so the statement is always false.
>
>         if (agg && MAC_ADDRESS_COMPARE(&agg->partner_system, &null_mac_addr)) {
>                 if (!netif_carrier_ok(bond->dev)) {
>                         netif_carrier_on(bond->dev);
>                         return 1;
>                 }
>                 return 0;
>         }
> ----
>

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

[Jay Vosburgh]

Laurent Chavey <chavey@google.com> wrote:

>On 5/31/07, Laurent Chavey <chavey@google.com> wrote:
>> if a host configured with 802.3ad bond mode is connected to a switch
>> that does not support 802.3ad,  then an aggregator is selected as the
>> active aggregator (first link that has carrier in the slave list).
>> This is perfectly fine, since it lets at least one of the link become active.
>> (this was the behavior prior to 2.6.18)
>>
>> In 2.6.18 and above, a new check for the partner mac address was added
>> before an aggregator's carrier is set on. If a host is  configured as
>> previously
>> described,  then no links will become active.
>>
>> is that the intended behavior ?

	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).

	My concern specifically was more with failures in negotiation
with 802.3ad capable peers, not for interoperability with non-802.3ad
devices (since bonding can always be run in a non-802.3ad mode).

	This behavior (don't pass traffic if no 802.3ad setup occurs)
also parallels the behavior of the Cisco switches I use to test bonding
(they will not pass traffic across ports of a lacp channel-group if the
802.3ad negotation does not occur), so it seemed appropriate.

	I'm checking the standard to see what it says, but I'm also
curious if this has some real-world impact, or is just something you
happened across?

	-J

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

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

[Laurent Chavey]

On 6/1/07, Jay Vosburgh <fubar@us.ibm.com> wrote:
> Laurent Chavey <chavey@google.com> wrote:
>
> >On 5/31/07, Laurent Chavey <chavey@google.com> wrote:
> >> if a host configured with 802.3ad bond mode is connected to a switch
> >> that does not support 802.3ad,  then an aggregator is selected as the
> >> active aggregator (first link that has carrier in the slave list).
> >> This is perfectly fine, since it lets at least one of the link become active.
> >> (this was the behavior prior to 2.6.18)
> >>
> >> In 2.6.18 and above, a new check for the partner mac address was added
> >> before an aggregator's carrier is set on. If a host is  configured as
> >> previously
> >> described,  then no links will become active.
> >>
> >> is that the intended behavior ?
>
>         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)

>
>         My concern specifically was more with failures in negotiation
> with 802.3ad capable peers, not for interoperability with non-802.3ad
> devices (since bonding can always be run in a non-802.3ad mode).
agree. This re-inforces that we should use the churned state

>
>         This behavior (don't pass traffic if no 802.3ad setup occurs)
> also parallels the behavior of the Cisco switches I use to test bonding
> (they will not pass traffic across ports of a lacp channel-group if the
> 802.3ad negotation does not occur), so it seemed appropriate.
yop, but the switch has to be configured with LACP support.
>
>         I'm checking the standard to see what it says, but I'm also
> curious if this has some real-world impact, or is just something you
> happened across?
this is the case when a switch is not configured with LACP support, and
a host is attached to it. this is something that happens in a pre release
environment where a host is tested  (using a switch that has no LACP support).
The host is then moved to its "real" rack (where the switch has LACP). The host
is not re-configured between the 2 steps (bonding is always enabled with
802.3ad mode)

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

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

[Jay Vosburgh]

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