[PATCH] teamd: lacp: update port state according to partner's sync bit

[Hangbin Liu]

1) According to 6.4.12 of IEEE 802.1AX-2008, Figure 6-18, if the partner's sync
bit is not set, the lacp port state should be set to EXPIRED.

Function lacpdu_recv() makes the state of the lacp port to PORT_STATE_CURRENT
when the teamd receives any valid lacp packet. Fix it by checking the parter's
sync bit.

2) According to 6.4.15 of IEEE 802.1AX-2008, Figure 6-22, the state that the
port is selected moves MUX state from DETACHED to ATTACHED.

But ATTACHED sate does not mean that the port can send and receive user frames.
COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
partner state should be sync as well as the port selected.

In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
true when INFO_STATE_SYNCHRONIZATION of partner.state is set.

In function lacp_port_should_be_{enabled, disabled}(), we also need to check
the INFO_STATE_SYNCHRONIZATION bit of partner.state.

Reported-by: Cisco Systems
Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
---
 teamd/teamd_runner_lacp.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

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)
 		return true;
 	return false;
 }
@@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(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))
 		return true;
 	return false;
 }
@@ -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)
+			state |= INFO_STATE_COLLECTING |
+				 INFO_STATE_DISTRIBUTING;
+	}
 	if (lacp_port->state == PORT_STATE_EXPIRED)
 		state |= INFO_STATE_EXPIRED;
 	if (lacp_port->state == PORT_STATE_DEFAULTED)
@@ -1095,7 +1101,10 @@ static int lacpdu_recv(struct lacp_port *lacp_port)
 			return err;
 	}
 
-	err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
+	if (lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
+		err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
+	else
+		err = lacp_port_set_state(lacp_port, PORT_STATE_EXPIRED);
 	if (err)
 		return err;
 
-- 
2.19.2

[Hangbin Liu]

On Wed, 30 Jan 2019 at 18:02, Hangbin Liu <liuhangbin(a)gmail.com&gt; wrote:
>  1) According to 6.4.12 of IEEE 802.1AX-2008, Figure 6-18, if the partner's sync
>  bit is not set, the lacp port state should be set to EXPIRED.
>
>  Function lacpdu_recv() makes the state of the lacp port to PORT_STATE_CURRENT
>  when the teamd receives any valid lacp packet. Fix it by checking the parter's
>  sync bit.
>
>  2) According to 6.4.15 of IEEE 802.1AX-2008, Figure 6-22, the state that the
>  port is selected moves MUX state from DETACHED to ATTACHED.
>
>  But ATTACHED sate does not mean that the port can send and receive user frames.
>  COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
>  user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
>  partner state should be sync as well as the port selected.
>
>  In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
>  be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
>  INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
>  true when INFO_STATE_SYNCHRONIZATION of partner.state is set.
>
>  In function lacp_port_should_be_{enabled, disabled}(), we also need to check
>  the INFO_STATE_SYNCHRONIZATION bit of partner.state.
>
>  Reported-by: Cisco Systems
>  Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
>  ---
>  
Hi Jiri,

Please hold on reviewing this patch. I may post a update later.

Thanks
Hangbin

[Hangbin Liu]

Hi Jiri,

There are two parts I want to update in this patch. Would you please help
have a look at first?

On Wed, 30 Jan 2019 at 18:02, Hangbin Liu <liuhangbin(a)gmail.com&gt; wrote:
>  1) According to 6.4.12 of IEEE 802.1AX-2008, Figure 6-18, if the partner's sync
>  bit is not set, the lacp port state should be set to EXPIRED.
>
>  Function lacpdu_recv() makes the state of the lacp port to PORT_STATE_CURRENT
>  when the teamd receives any valid lacp packet. Fix it by checking the parter's
>  sync bit.
>
>  2) According to 6.4.15 of IEEE 802.1AX-2008, Figure 6-22, the state that the
>  port is selected moves MUX state from DETACHED to ATTACHED.
>
>  But ATTACHED sate does not mean that the port can send and receive user frames.
>  COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
>  user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
>  partner state should be sync as well as the port selected.
>
>  In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
>  be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
>  INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
>  true when INFO_STATE_SYNCHRONIZATION of partner.state is set.
>
>  In function lacp_port_should_be_{enabled, disabled}(), we also need to check
>  the INFO_STATE_SYNCHRONIZATION bit of partner.state.
>
>  Reported-by: Cisco Systems
>  Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
>  ---
>   teamd/teamd_runner_lacp.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
>
>  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.

>                  return true;
>          return false;
>   }
>  @@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(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))
>                  return true;
>          return false;
>   }
>  @@ -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?

>  +                       state |= INFO_STATE_COLLECTING |
>  +                                INFO_STATE_DISTRIBUTING;
>  +       }
>          if (lacp_port->state == PORT_STATE_EXPIRED)
>                  state |= INFO_STATE_EXPIRED;
>          if (lacp_port->state == PORT_STATE_DEFAULTED)
>  @@ -1095,7 +1101,10 @@ static int lacpdu_recv(struct lacp_port *lacp_port)
>                          return err;
>          }
>
>  -       err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
>  +       if (lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
>  +               err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
>  +       else
>  +               err = lacp_port_set_state(lacp_port, PORT_STATE_EXPIRED);
>          if (err)
>                  return err;
>  
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.

So, should we select port with EXPIRED state?

Thanks
Hangbin

[Hangbin Liu]

On Thu, 28 Feb 2019 at 21:06, Hangbin Liu <liuhangbin(a)gmail.com&gt; 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

[patchv2 libteam] teamd: lacp: update port state according to partner's sync bit

[Hangbin Liu]

According to 6.4.15 of IEEE 802.1AX-2014, Figure 6-22, the state that the
port is selected moves MUX state from DETACHED to ATTACHED.

But ATTACHED sate does not mean that the port can send and receive user frames.
COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
partner state should be sync as well as the port selected.

In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
true when INFO_STATE_SYNCHRONIZATION of partner.state is set.

In function lacp_port_should_be_{enabled, disabled}(), we also need to check
the INFO_STATE_SYNCHRONIZATION bit of partner.state.

Reported-by: Cisco Systems
Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
---
 teamd/teamd_runner_lacp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 7b8f0a7..4de30f3 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)
 		return true;
 	return false;
 }
@@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(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))
 		return true;
 	return false;
 }
@@ -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)
+			state |= INFO_STATE_COLLECTING |
+				 INFO_STATE_DISTRIBUTING;
+	}
 	if (lacp_port->state == PORT_STATE_EXPIRED)
 		state |= INFO_STATE_EXPIRED;
 	if (lacp_port->state == PORT_STATE_DEFAULTED)
-- 
2.19.2

[patchv2 libteam] teamd: lacp: update port state according to partner's sync bit

[Jiri Pirko]

Thu, Mar 07, 2019 at 03:47:43AM CET, liuhangbin(a)gmail.com wrote:
> According to 6.4.15 of IEEE 802.1AX-2014, Figure 6-22, the state that
> the
> port is selected moves MUX state from DETACHED to ATTACHED.
>
> But ATTACHED sate does not mean that the port can send and receive user frames. 
s/sate/state/


> COLLECTING_DISTRIBUTION state is the sate that the port can send and
> receive 
s/sate/state/


> user frames. To move MUX state from ATTACHED to
> COLLECTING_DISTRIBUTION, the 
With text description lines should not be longer than 72 cols.


> partner state should be sync as well as the port selected.
>
> In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
> be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
> INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
> true when INFO_STATE_SYNCHRONIZATION of partner.state is set.
>
> In function lacp_port_should_be_{enabled, disabled}(), we also need to check
> the INFO_STATE_SYNCHRONIZATION bit of partner.state.
>
> Reported-by: Cisco Systems 
Should be person with email.


>Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
>---
> teamd/teamd_runner_lacp.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
>diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
>index 7b8f0a7..4de30f3 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)
> 		return true;
> 	return false;
> }
>@@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(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))
> 		return true;
> 	return false;
> }
>@@ -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)
>+			state |= INFO_STATE_COLLECTING |
>+				 INFO_STATE_DISTRIBUTING;
>+	}
> 	if (lacp_port->state == PORT_STATE_EXPIRED)
> 		state |= INFO_STATE_EXPIRED;
> 	if (lacp_port->state == PORT_STATE_DEFAULTED)
>-- 
>2.19.2
>

[patchv2 libteam] teamd: lacp: update port state according to partner's sync bit

[Hangbin Liu]

On Fri, Mar 08, 2019 at 09:59:28AM +0100, Jiri Pirko wrote:
>  Thu, Mar 07, 2019 at 03:47:43AM CET, liuhangbin(a)gmail.com wrote:
>  >According to 6.4.15 of IEEE 802.1AX-2014, Figure 6-22, the state that the
>  >port is selected moves MUX state from DETACHED to ATTACHED.
>  >
>  >But ATTACHED sate does not mean that the port can send and receive user frames.
>  
>  s/sate/state/
>  
>  
>  >COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
>  
>  s/sate/state/
>  
>  
>  >user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
>  
>  With text description lines should not be longer than 72 cols.
>  
>  
>  >partner state should be sync as well as the port selected.
>  >
>  >In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
>  >be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
>  >INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
>  >true when INFO_STATE_SYNCHRONIZATION of partner.state is set.
>  >
>  >In function lacp_port_should_be_{enabled, disabled}(), we also need to check
>  >the INFO_STATE_SYNCHRONIZATION bit of partner.state.
>  >
>  >Reported-by: Cisco Systems
>  
>  Should be person with email. 
OK, I will remove the Reported-by as I don't know the person's email..

[patchv3 libteam] teamd: lacp: update port state according to partner's sync bit

[Hangbin Liu]

According to 6.4.15 of IEEE 802.1AX-2014, Figure 6-22, the state that the
port is selected moves MUX state from DETACHED to ATTACHED.

But ATTACHED state does not mean that the port can send and receive user
frames. COLLECTING_DISTRIBUTION state is the state that the port can send
and receive user frames. To move MUX state from ATTACHED to
COLLECTING_DISTRIBUTION, the partner state should be sync as well as the
port selected.

In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION
should be set to the actor.state when the port is selected.
INFO_STATE_COLLECTING and INFO_STATE_DISTRIBUTING should be set to false
with ATTACHED mode and set to true when INFO_STATE_SYNCHRONIZATION of
partner.state is set.

In function lacp_port_should_be_{enabled, disabled}(), we also need to
check the INFO_STATE_SYNCHRONIZATION bit of partner.state.

Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt;
---
 teamd/teamd_runner_lacp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 7b8f0a7..4de30f3 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)
 		return true;
 	return false;
 }
@@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(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))
 		return true;
 	return false;
 }
@@ -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)
+			state |= INFO_STATE_COLLECTING |
+				 INFO_STATE_DISTRIBUTING;
+	}
 	if (lacp_port->state == PORT_STATE_EXPIRED)
 		state |= INFO_STATE_EXPIRED;
 	if (lacp_port->state == PORT_STATE_DEFAULTED)
-- 
2.19.2

[patchv3 libteam] teamd: lacp: update port state according to partner's sync bit

[Jiri Pirko]

Fri, Mar 08, 2019 at 12:28:55PM CET, liuhangbin(a)gmail.com wrote:
> According to 6.4.15 of IEEE 802.1AX-2014, Figure 6-22, the state that
> the
> port is selected moves MUX state from DETACHED to ATTACHED.
>
> But ATTACHED state does not mean that the port can send and receive user
> frames. COLLECTING_DISTRIBUTION state is the state that the port can send
> and receive user frames. To move MUX state from ATTACHED to
> COLLECTING_DISTRIBUTION, the partner state should be sync as well as the
> port selected.
>
> In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION
> should be set to the actor.state when the port is selected.
> INFO_STATE_COLLECTING and INFO_STATE_DISTRIBUTING should be set to false
> with ATTACHED mode and set to true when INFO_STATE_SYNCHRONIZATION of
> partner.state is set.
>
> In function lacp_port_should_be_{enabled, disabled}(), we also need to
> check the INFO_STATE_SYNCHRONIZATION bit of partner.state.
>
> Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com&gt; 
applied, thanks!