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 ?