Send LACP PDU right after Actor state has been changed
diff mbox series

Message ID 1587150486-12634-1-git-send-email-pavel.contrib@gmail.com
State New
Headers show
Series
  • Send LACP PDU right after Actor state has been changed
Related show

Commit Message

Pavel Shirshov April 17, 2020, 7:08 p.m. UTC
Fix a bug in libteam LACP protocol implementation.

According to the LACP standard "LACP daemon must send LACP PDU
packets with updates "when the Actor’s state changes or when it is
apparent from the Partner’s LACPDUs that the Partner does not know
the Actor’s current state."

The current libteam implementation sends periodic updates only, so
in LACP rate slow mode the update will be sent within 30 seconds
which doesn't follow the LACP standard.

To fix the issue:
1. The following patch was reverted:
https://github.com/jpirko/libteam/commit/b2de61b39696c9158e725a691aee5a6f16a64137
2. LACP actor state recalculated before the comparison what the LACP
partner thinks about the LACP actor state.

Signed-off-by: Pavel Shirshov <pavel.contrib@gmail.com>
---
 teamd/teamd_runner_lacp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jiri Pirko April 25, 2020, 6:26 a.m. UTC | #1
Fri, Apr 17, 2020 at 09:08:06PM CEST, pavel.contrib@gmail.com wrote:
>Fix a bug in libteam LACP protocol implementation.

Avoid this line here.


>
>According to the LACP standard "LACP daemon must send LACP PDU
>packets with updates "when the Actor’s state changes or when it is

There's an extra " here I believe.


>apparent from the Partner’s LACPDUs that the Partner does not know
>the Actor’s current state."

Please don't use "’". Use "'" instead (I see 3 occurencies here).


>
>The current libteam implementation sends periodic updates only, so
>in LACP rate slow mode the update will be sent within 30 seconds
>which doesn't follow the LACP standard.
>
>To fix the issue:
>1. The following patch was reverted:
>https://github.com/jpirko/libteam/commit/b2de61b39696c9158e725a691aee5a6f16a64137

Dont use link. Just the hash and commit name in following format:
b2de61b39696 ("Fix sending duplicate LACP frames at the start of establishing a logical channel.")


>2. LACP actor state recalculated before the comparison what the LACP
>partner thinks about the LACP actor state.

Be imperative to the codebase in 1) and 2)

>
>Signed-off-by: Pavel Shirshov <pavel.contrib@gmail.com>
>---
> teamd/teamd_runner_lacp.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
>index ec01237..11d02f1 100644
>--- a/teamd/teamd_runner_lacp.c
>+++ b/teamd/teamd_runner_lacp.c
>@@ -996,8 +996,7 @@ static int lacp_port_set_state(struct lacp_port *lacp_port,
> 		return err;
> 
> 	lacp_port_actor_update(lacp_port);
>-	if (lacp_port->periodic_on)
>-		return 0;
>+
> 	return lacpdu_send(lacp_port);
> }
> 
>@@ -1110,9 +1109,10 @@ static int lacpdu_recv(struct lacp_port *lacp_port)
> 	if (err)
> 		return err;
> 
>+	lacp_port_actor_update(lacp_port);
>+
> 	/* Check if the other side has correct info about us */
>-	if (!lacp_port->periodic_on &&
>-	    memcmp(&lacpdu.partner, &lacp_port->actor,
>+	if (memcmp(&lacpdu.partner, &lacp_port->actor,
> 		   sizeof(struct lacpdu_info))) {
> 		err = lacpdu_send(lacp_port);
> 		if (err)
>-- 
>2.7.4
>

The code looks fine. Please send v2.

Patch
diff mbox series

diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index ec01237..11d02f1 100644
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -996,8 +996,7 @@  static int lacp_port_set_state(struct lacp_port *lacp_port,
 		return err;
 
 	lacp_port_actor_update(lacp_port);
-	if (lacp_port->periodic_on)
-		return 0;
+
 	return lacpdu_send(lacp_port);
 }
 
@@ -1110,9 +1109,10 @@  static int lacpdu_recv(struct lacp_port *lacp_port)
 	if (err)
 		return err;
 
+	lacp_port_actor_update(lacp_port);
+
 	/* Check if the other side has correct info about us */
-	if (!lacp_port->periodic_on &&
-	    memcmp(&lacpdu.partner, &lacp_port->actor,
+	if (memcmp(&lacpdu.partner, &lacp_port->actor,
 		   sizeof(struct lacpdu_info))) {
 		err = lacpdu_send(lacp_port);
 		if (err)