On 9/11/2015 9:03 AM, Jiri Pirko wrote:
Fri, Sep 11, 2015 at 03:24:07PM CEST, fbl(a)sysclose.org wrote:
> On Fri, Sep 11, 2015 at 09:46:45AM +0200, Jiri Pirko wrote:
>> Fri, Sep 11, 2015 at 01:14:09AM CEST, sridhar.samudrala(a)intel.com wrote:
>>> Currently AGGREGATION flag is only set when there is a change in port state.
>>> Adding a 2nd port should cause the AGGREGATION flag of the first port to be
>>> changed to 'Yes' and removing the last but one port should cause the
>>> AGGREGATION flag of the remaining port to be changed to 'No'.
>>> A simple fix is set AGGREGATION flag before sending LACP frames based on
>>> the number of ports in the team.
>>
>> INFO_STATE_AGGREGATION is set in lacp_port_actor_update. Why that is not
>> enough?
> It seems we only do that if the state of a port changes.
>
> 920 static int lacp_port_set_state(struct lacp_port *lacp_port,
> 921 enum lacp_port_state new_state)
> 922 {
> 923 int err;
> 924
> *925 if (new_state == lacp_port->state)
> 926 return 0;
>
> fbl
So the problematic codepath is lacp_callback_periodic->lacpdu_send where
the sate is not updated.
Yes. the patch looks good and the actor.state gets
updated in the
periodic path before sending the
LACPDU.
How about following patch:
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 09899ce..1cfef36 100644
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -888,9 +888,7 @@ static void lacp_port_actor_init(struct lacp_port *lacp_port)
lacp_port_actor_system_update(lacp_port);
}
-static int lacpdu_send(struct lacp_port *lacp_port);
-
-static int lacp_port_actor_update(struct lacp_port *lacp_port)
+static void lacp_port_actor_update(struct lacp_port *lacp_port)
{
uint8_t state = 0;
@@ -910,13 +908,10 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port)
teamd_log_dbg("%s: lacp info state: 0x%02X.",
lacp_port->tdport->ifname,
state);
lacp_port->actor.state = state;
-
- if (lacp_port->periodic_on)
- return 0;
-
- return lacpdu_send(lacp_port);
}
+static int lacpdu_send(struct lacp_port *lacp_port);
+
static int lacp_port_set_state(struct lacp_port *lacp_port,
enum lacp_port_state new_state)
{
@@ -971,7 +966,10 @@ static int lacp_port_set_state(struct lacp_port *lacp_port,
if (err)
return err;
- return lacp_port_actor_update(lacp_port);
+ lacp_port_actor_update(lacp_port);
+ if (lacp_port->periodic_on)
+ return 0;
+ return lacpdu_send(lacp_port);
}
static enum lacp_port_state lacp_port_get_state(struct lacp_port *lacp_port)
@@ -1114,6 +1112,7 @@ static int lacp_callback_periodic(struct teamd_context *ctx, int
events,
{
struct lacp_port *lacp_port = priv;
+ lacp_port_actor_update(lacp_port);
return lacpdu_send(lacp_port);
}