[PATCH] teamd: lacp: Set COLLECTING & DISTRIBUTING flags only if the port state is CURRENT.
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com --- teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT) + 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)
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
So, without more details for this change, NACK.
fbl
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
So, without more details for this change, NACK.
fbl
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
So, without more details for this change, NACK.
fbl
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
On Fri, Sep 18, 2015 at 10:05:54AM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
He has a valid point. Teamd shouldn't push frames if the other side hasn't set COLLECTING bit otherwise there might be packet loss. Also, teamd shouldn't tell the other side to send when it isn't ready.
But as it is today, we might have packet loss but still the packets are flowing. With the patch applied, we have less issues with packet loss but there is no connectivity in the other states.
To follow the standard and fix that and other issues, teamd needs the MUX state machine which relies on selected and sync states.
fbl
So, without more details for this change, NACK.
fbl
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote:
Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com
teamd/teamd_runner_lacp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c index 09899ce..f663016 100644 --- a/teamd/teamd_runner_lacp.c +++ b/teamd/teamd_runner_lacp.c @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) 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->state == PORT_STATE_CURRENT)
state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;-- 2.1.0
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
Fri, Sep 18, 2015 at 02:39:10PM CEST, fbl@sysclose.org wrote:
On Fri, Sep 18, 2015 at 10:05:54AM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote:
Can you please provide some more info about this change, like when in the spec this is decribed? Thanks!
Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
He has a valid point. Teamd shouldn't push frames if the other side hasn't set COLLECTING bit otherwise there might be packet loss. Also, teamd shouldn't tell the other side to send when it isn't ready.
But as it is today, we might have packet loss but still the packets are flowing. With the patch applied, we have less issues with packet loss but there is no connectivity in the other states.
To follow the standard and fix that and other issues, teamd needs the MUX state machine which relies on selected and sync states.
fbl
Fine, so you are ok with the patch as it is?
So, without more details for this change, NACK.
fbl
Thanks Sridhar
Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote: >Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com >--- >teamd/teamd_runner_lacp.c | 3 ++- >1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c >index 09899ce..f663016 100644 >--- a/teamd/teamd_runner_lacp.c >+++ b/teamd/teamd_runner_lacp.c >@@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) > 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->state == PORT_STATE_CURRENT) >+ 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.1.0 >
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
On Fri, Sep 18, 2015 at 02:50:45PM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 02:39:10PM CEST, fbl@sysclose.org wrote:
On Fri, Sep 18, 2015 at 10:05:54AM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote:
On 9/17/2015 12:42 AM, Jiri Pirko wrote: >Can you please provide some more info about this change, like when in >the spec this is decribed? Thanks! Section 5.3.15 in 802.1ax spec defines these flags http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf Here is another link that describes the LACP flags. http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-...
Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
He has a valid point. Teamd shouldn't push frames if the other side hasn't set COLLECTING bit otherwise there might be packet loss. Also, teamd shouldn't tell the other side to send when it isn't ready.
But as it is today, we might have packet loss but still the packets are flowing. With the patch applied, we have less issues with packet loss but there is no connectivity in the other states.
To follow the standard and fix that and other issues, teamd needs the MUX state machine which relies on selected and sync states.
fbl
Fine, so you are ok with the patch as it is?
Sorry, NACK, unless I am missing something. IMO, we are better off without the patch.
fbl
So, without more details for this change, NACK.
fbl
Thanks Sridhar
> >Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote: >>Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com >>--- >>teamd/teamd_runner_lacp.c | 3 ++- >>1 file changed, 2 insertions(+), 1 deletion(-) >> >>diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c >>index 09899ce..f663016 100644 >>--- a/teamd/teamd_runner_lacp.c >>+++ b/teamd/teamd_runner_lacp.c >>@@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) >> 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->state == PORT_STATE_CURRENT) >>+ 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.1.0 >> _______________________________________________ libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
On 9/18/2015 7:21 AM, Flavio Leitner wrote:
On Fri, Sep 18, 2015 at 02:50:45PM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 02:39:10PM CEST, fbl@sysclose.org wrote:
On Fri, Sep 18, 2015 at 10:05:54AM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote:
On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote: > On 9/17/2015 12:42 AM, Jiri Pirko wrote: >> Can you please provide some more info about this change, like when in >> the spec this is decribed? Thanks! > Section 5.3.15 in 802.1ax spec defines these flags > http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf > Here is another link that describes the LACP flags. > http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-... Are you actually seeing a bug without this patch?
COLLECTING and DISTRIBUTING are informative bits about the port's state, but they are secondary to other states like SYNCHRONIZATION, so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
With the proposed patch applied, there could be an outage if the state moves to EXPIRED or DEFAULTED.
With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
He has a valid point. Teamd shouldn't push frames if the other side hasn't set COLLECTING bit otherwise there might be packet loss. Also, teamd shouldn't tell the other side to send when it isn't ready.
But as it is today, we might have packet loss but still the packets are flowing. With the patch applied, we have less issues with packet loss but there is no connectivity in the other states.
It is not clear to me why the connectivity is lost in other states with this patch. Does the partner not send LACPDUs if COLLECTING/DISTRIBUTING flags are not set? IIUC, these bits only effect the data packets. So LACP frames will continue to flow and once the LACPDU is received from the partner, the port state goes to CURRENT and we start setting COLLECTING/DISTRIBUTING flags.
To follow the standard and fix that and other issues, teamd needs the MUX state machine which relies on selected and sync states.
fbl
Fine, so you are ok with the patch as it is?
Sorry, NACK, unless I am missing something. IMO, we are better off without the patch.
fbl
So, without more details for this change, NACK.
fbl
> Thanks > Sridhar > >> Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote: >>> Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com >>> --- >>> teamd/teamd_runner_lacp.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c >>> index 09899ce..f663016 100644 >>> --- a/teamd/teamd_runner_lacp.c >>> +++ b/teamd/teamd_runner_lacp.c >>> @@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) >>> 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->state == PORT_STATE_CURRENT) >>> + 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.1.0 >>> > _______________________________________________ > libteam mailing list > libteam@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
On Fri, Sep 18, 2015 at 08:53:49AM -0700, Samudrala, Sridhar wrote:
On 9/18/2015 7:21 AM, Flavio Leitner wrote:
On Fri, Sep 18, 2015 at 02:50:45PM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 02:39:10PM CEST, fbl@sysclose.org wrote:
On Fri, Sep 18, 2015 at 10:05:54AM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 12:00:51AM CEST, sridhar.samudrala@intel.com wrote:
On 9/17/2015 2:10 PM, Flavio Leitner wrote: >On Thu, Sep 17, 2015 at 11:45:20AM -0700, Samudrala, Sridhar wrote: >>On 9/17/2015 12:42 AM, Jiri Pirko wrote: >>>Can you please provide some more info about this change, like when in >>>the spec this is decribed? Thanks! >>Section 5.3.15 in 802.1ax spec defines these flags >>http://varlyakov.com/diplomna/diplomna/802.1ax/802.1ax.pdf >>Here is another link that describes the LACP flags. >>http://networkingbodges.blogspot.com/2012/12/all-sorts-of-things-about-lacp-... >Are you actually seeing a bug without this patch? > >COLLECTING and DISTRIBUTING are informative bits about the port's >state, but they are secondary to other states like SYNCHRONIZATION, >so I don't see how that can cause an issue.
IIRC, I originally decided to set those 2 bits always, because there is not an actual state machine inside teamd_lacp as it is described in the standard, so the internal status does not make a lot of sense.
>With the proposed patch applied, there could be an outage if >the state moves to EXPIRED or DEFAULTED. With the current code, these bits are always set even if the partner is not present. Is this OK? When the state moves to EXPIRED or DEFAULTED, isn't it correct to mark these bits as FALSE? It looks like teamd implementation is not really looking at these bits in the received LACPDUs.
This is more of an issue detected by our validation team looking at the LACP frames generated by teamd rather than an actual interoperability issue with other implementations.
If it does not break interoperability, I would leave this as it is. Unless there is an argument againt it. Thoughts?
He has a valid point. Teamd shouldn't push frames if the other side hasn't set COLLECTING bit otherwise there might be packet loss. Also, teamd shouldn't tell the other side to send when it isn't ready.
But as it is today, we might have packet loss but still the packets are flowing. With the patch applied, we have less issues with packet loss but there is no connectivity in the other states.
It is not clear to me why the connectivity is lost in other states with this patch. Does the partner not send LACPDUs if COLLECTING/DISTRIBUTING flags are not set? IIUC, these bits only effect the data packets. So LACP frames will continue to flow and once
That is correct.
the LACPDU is received from the partner, the port state goes to CURRENT and we start setting COLLECTING/DISTRIBUTING flags.
Also, correct. However, when we don't set COLLECTING, the other peer should stop sending data packets. The LACPDUs can be exchanged in slow rate, so there will be an interval of 30 seconds (assuming it doesn't get lost) in the worse case scenario where there will be no data connectivity for no good reason.
fbl
To follow the standard and fix that and other issues, teamd needs the MUX state machine which relies on selected and sync states.
fbl
Fine, so you are ok with the patch as it is?
Sorry, NACK, unless I am missing something. IMO, we are better off without the patch.
fbl
>So, without more details for this change, NACK. > >fbl > >>Thanks >>Sridhar >> >>>Thu, Sep 17, 2015 at 12:47:36AM CEST, sridhar.samudrala@intel.com wrote: >>>>Signed-off-by: Sridhar Samudrala sridhar.samudrala@intel.com >>>>--- >>>>teamd/teamd_runner_lacp.c | 3 ++- >>>>1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>>diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c >>>>index 09899ce..f663016 100644 >>>>--- a/teamd/teamd_runner_lacp.c >>>>+++ b/teamd/teamd_runner_lacp.c >>>>@@ -900,7 +900,8 @@ static int lacp_port_actor_update(struct lacp_port *lacp_port) >>>> 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->state == PORT_STATE_CURRENT) >>>>+ 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.1.0 >>>> >>_______________________________________________ >>libteam mailing list >>libteam@lists.fedorahosted.org >>https://lists.fedorahosted.org/mailman/listinfo/libteam _______________________________________________ libteam mailing list libteam@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/libteam
libteam@lists.fedorahosted.org
-
Flavio Leitner -
Jiri Pirko -
Samudrala, Sridhar