On Fri, Sep 18, 2015 at 02:50:45PM +0200, Jiri Pirko wrote:
Fri, Sep 18, 2015 at 02:39:10PM CEST, fbl(a)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(a)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-and-lags.html
>> >>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(a)intel.com wrote:
>> >>>>>Signed-off-by: Sridhar Samudrala
<sridhar.samudrala(a)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(a)lists.fedorahosted.org
>> >>>https://lists.fedorahosted.org/mailman/listinfo/libteam
>> >
>> >_______________________________________________
>> >libteam mailing list
>> >libteam(a)lists.fedorahosted.org
>> >https://lists.fedorahosted.org/mailman/listinfo/libteam
>>
>