On Thu, 28 Feb 2019 at 21:06, Hangbin Liu <liuhangbin(a)gmail.com> wrote:
> diff --git a/teamd/teamd_runner_lacp.c
b/teamd/teamd_runner_lacp.c
> index 7b8f0a7..5e763ad 100644
> --- a/teamd/teamd_runner_lacp.c
> +++ b/teamd/teamd_runner_lacp.c
> @@ -333,7 +333,8 @@ static int lacp_port_should_be_enabled(struct lacp_port
*lacp_port)
> struct lacp *lacp = lacp_port->lacp;
>
> if (lacp_port_selected(lacp_port) &&
> - lacp_port->agg_lead == lacp->selected_agg_lead)
> + lacp_port->agg_lead == lacp->selected_agg_lead &&
> + lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
First is here. I want to enable the port when partner enters COLLECTING state.
In this patch, We will set state to "SYNC & COLLECTING & DISTRIBUTIN"
and enable port
when partner enter SYNCHRONIZATION state. But as talked in [1],
Source Peer
S:0 C:0 D:0 S:0 C:0 D:0
S:1 C:0 D:0 ->
<- S:1 C:0 D:0 ?> At this time, peer LACP members have been chosen
S:1 C:1 D:0 -> ?> Source port indicates that it is ready to accept traffic
<- S:1 C:1 D:0 ?> Destination port indicates that it is ready to accept traffic
S:1 C:1 D:1 -> ?> Source port indicates that it is ready to send traffic
<- S:1 C:1 D:1 ?> Destination port indicates that it is ready to send traffic
After we enabling port and start transmitting data, partner is in SYNC
state but not in
COLLECTING. There will be a moment that partner not ready to receive
data. The data
will be dropped.
So enable port after partner ready to receive trafice(in COLLECTING
state) seems better.
OK, based on IEEE standard 8021AX-2014, Figure 6-22
"the coupled control state machine does not wait for the Partner to signal
that collection has started before enabling both collection and distribution."
So there is no need to wait for partner entering COLLECTING state
before enable slave.
> @@ -914,9 +916,13 @@ static void lacp_port_actor_update(struct
lacp_port *lacp_port)
> if (lacp_port->lacp->cfg.fast_rate)
> state |= INFO_STATE_LACP_TIMEOUT;
> if (lacp_port_selected(lacp_port) &&
> - lacp_port_agg_selected(lacp_port))
> - state |= INFO_STATE_SYNCHRONIZATION |
> - INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
> + lacp_port_agg_selected(lacp_port)) {
> + state |= INFO_STATE_SYNCHRONIZATION;
> + state &= ~(INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING);
> + if (lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
Same here, should we set state to COLLECTING & DISTRIBUTING after
partner enters
INFO_STATE_COLLECTING state?
no
The second question is about commit a8e7d65f64b ("teamd: lacp:
"expired" port
is not selectable, only "current""). Is there a line in the standard
that clarify we should
not select the port in PORT_STATE_EXPIRED state?
The reason I ask is, as my patch describe, we should enter EXPIRED state when
we receive lacp without sync bit, which means we will not select the
port based on
commit a8e7d65f64b, then in function lacp_port_actor_update() we will never set
local state to SYNCHRONIZATION. So we will never send lacp message with sync
bit before partner send sync. Cisco has an option 'no lacp
graceful-convergence',
which did a similar thing. With this option and my patch, libteam and
Cisco switch will
never go to COLLECTING & DISTRIBUTING as both sides are waiting for
the sync bit.
I reconsider this sync and found the corrent patch is incorrect.
The current patch logic looks like
INIT -> send and receive "E:1 G:1 A:1" - > EXPIRED -> recive "S:1
G:1
A:1" - CURRENT.
But if both sides are in EXPIRED state, they will only send and
receive "E:1 G:1 A:1" messages.
Both sides will never receive "S:1" bit and never go in CURRENT state.
The correct logic should be like
INIT -> port link up && lacp enable -> EXPIRED -> send "E:1 G:1
A:1"
-> receive "E:1 G:1 A:1" -> CURRENT -> send "S:1 C:1 D:1"
Which is the current libteam logic. I will remove the first part of my patch
Thanks
Hangbin