On Sat, Apr 15, 2017 at 8:03 AM, Jamie Bainbridge
<jamie.bainbridge(a)gmail.com> wrote:
> On 14 April 2017 at 17:24, Xin Long <lucien.xin(a)gmail.com> wrote:
>> Patrick reported an issue about COLLECTING, DISTRIBUTING and
>> SYNCHRONIZATION flags when the ports with standby agg. :
>>
>> The ports associated with the non-selected (standby) aggregation
>> should not sent the Synchronising, Collecting, and Distributing flags
>> in their LACPDUs. So a port state of 7 for fast rate or 5 for slow
>> rate.
>>
>> It also causes team work different from bonding.
>>
>> I've noticed this thread:
>>
https://lists.fedorahosted.org/archives/list/libteam@lists.fedorahosted.o...
>>
>> But here we will talk more about 'the ports with standby agg'.
>>
>> --------------
>> He thinks:
>>
>> 1. Collecting & Distributing bits indicate to the partner that the
>> port can actively be used to both receive and transmit normal network
>> traffic. If a port is part of a *standby* aggregator, it just seems
>> wrong to set these bits. With the bits set, the switch is being told
>> that this port can be used to receive regular network traffic which is
>> not true.
>>
>> From The IEEE 802.1AX-2008 standard section 5.4.2.2 description of the
>> Collecting & Distributing bits:
>>
>> Collecting is encoded in bit 4. TRUE (encoded as a 1) means collection
>> of incoming frames on
>> this link is definitely enabled; i.e., collection is currently enabled
>> and is not expected to be
>> disabled in the absence of administrative changes or changes in
>> received protocol information.
>> Its value is otherwise FALSE (encoded as a 0).
>>
>> Distributing is encoded in bit 5. FALSE (encoded as a 0) means
>> distribution of outgoing frames
>> on this link is definitely disabled; i.e., distribution is currently
>> disabled and is not expected to be enabled in the absence of
>> administrative changes or changes in received protocol
>> information. Its value is otherwise TRUE (encoded as a 1).
>>
>>
>> 2. The bonding driver clearly agrees with me :p as it does not set
>> these bits on ports of an standby aggregator. It only sets bits 1 & 3
>> (& 2 depending on lacp rate) for ports in a standby aggregator.
>>
>>
>> 3. In 5.4.12.2 Selection Logic Requirements:
>>
>> If there are further constraints on the attachment of ports that have
>> selected an Aggregator, those
>> ports may be selected as standby in accordance with the rules
>> specified in 5.6.1. Selection or
>> deselection of that Aggregator can cause the Selection Logic to
>> re-evaluate the ports to be selected
>> as standby.
>>
>> .... and in 5.6.1 Use of system and port priorities:
>>
>> For each link that a given System cannot include in the aggregation,
>> the Selection Logic identifies
>> the Selection state of the corresponding port as STANDBY, preventing
>> the link from becoming
>> active. The synchronization state signaled in transmitted LACPDUs for
>> such links will be
>> OUT_OF_SYNC.
>>
>>
>> (out of sync means the Synchornization bit should not be set).
>>
>>
>>
>> Basically, there should be some logic where each port should check
>> whether it is a member of a 'selected' aggregator and if not, then it
>> should set its Synchronization, Collecting, and Distributing bits to
>> 0.
>
> From 5.6.1 the correct behaviour seems to be:
>
> * Standby ports MAY set Collecting and Distributing bits
> * Standby ports MUST NOT set Synchronization bit
>
> The lack of Synchronization bit should prevent the standby port being
> included in the selection logic.
Thanks Jamie.
I think we are on same page about that standby port shouldn't set
Synchronization bit.
As for Collecting and Distributing, if no other special reasons, to keep
consistent with bonding, I agree with Patrick to clear them as well.
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -913,9 +913,11 @@ static void lacp_port_actor_update(struct
lacp_port *lacp_port)
state |= INFO_STATE_LACP_ACTIVITY;
if (lacp_port->lacp->cfg.fast_rate)
state |= INFO_STATE_LACP_TIMEOUT;
- if (lacp_port_selected(lacp_port))
- state |= INFO_STATE_SYNCHRONIZATION;
- state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
+ if (lacp_port_selected(lacp_port) &&
+ lacp_port_agg_selected(lacp_port)) {
+ state |= INFO_STATE_SYNCHRONIZATION |
+ INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
+ }
Jiri, what do you think ?
Looks fine. Please submit this patch.