[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADVnQym6aoueiB-auSxgp5tp0rjjte+MaxRPWd3t44F5VueKdA@mail.gmail.com>
Date: Wed, 24 Jun 2020 09:43:00 -0400
From: Neal Cardwell <ncardwell@...gle.com>
To: Denis Kirjanov <kda@...ux-powerpc.org>
Cc: Netdev <netdev@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>,
Yuchung Cheng <ycheng@...gle.com>,
"Scheffenegger, Richard" <Richard.Scheffenegger@...app.com>,
Bob Briscoe <ietf@...briscoe.net>
Subject: Re: [PATCH v2] tcp: don't ignore ECN CWR on pure ACK
On Wed, Jun 24, 2020 at 5:58 AM Denis Kirjanov <kda@...ux-powerpc.org> wrote:
>
> there is a problem with the CWR flag set in an incoming ACK segment
> and it leads to the situation when the ECE flag is latched forever
>
> the following packetdrill script shows what happens:
>
> // Stack receives incoming segments with CE set
> +0.1 <[ect0] . 11001:12001(1000) ack 1001 win 65535
> +0.0 <[ce] . 12001:13001(1000) ack 1001 win 65535
> +0.0 <[ect0] P. 13001:14001(1000) ack 1001 win 65535
>
> // Stack repsonds with ECN ECHO
> +0.0 >[noecn] . 1001:1001(0) ack 12001
> +0.0 >[noecn] E. 1001:1001(0) ack 13001
> +0.0 >[noecn] E. 1001:1001(0) ack 14001
>
> // Write a packet
> +0.1 write(3, ..., 1000) = 1000
> +0.0 >[ect0] PE. 1001:2001(1000) ack 14001
>
> // Pure ACK received
> +0.01 <[noecn] W. 14001:14001(0) ack 2001 win 65535
>
> // Since CWR was sent, this packet should NOT have ECE set
>
> +0.1 write(3, ..., 1000) = 1000
> +0.0 >[ect0] P. 2001:3001(1000) ack 14001
> // but Linux will still keep ECE latched here, with packetdrill
> // flagging a missing ECE flag, expecting
> // >[ect0] PE. 2001:3001(1000) ack 14001
> // in the script
>
> In the situation above we will continue to send ECN ECHO packets
> and trigger the peer to reduce the congestion window. To avoid that
> we can check CWR on pure ACKs received.
>
> v2:
> - Adjusted the comment
> - move CWR check before checking for unacknowledged packets
>
> Signed-off-by: Denis Kirjanov <denis.kirjanov@...e.com>
> ---
> net/ipv4/tcp_input.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 12fda8f27b08..f1936c0cb684 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3665,6 +3665,15 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
> tcp_in_ack_event(sk, ack_ev_flags);
> }
>
> + /* This is a deviation from RFC3168 since it states that:
> + * "When the TCP data sender is ready to set the CWR bit after reducing
> + * the congestion window, it SHOULD set the CWR bit only on the first
> + * new data packet that it transmits."
> + * We accept CWR on pure ACKs to be more robust
> + * with widely-deployed TCP implementations that do this.
> + */
> + tcp_ecn_accept_cwr(sk, skb);
> +
> /* We passed data and got it acked, remove any soft error
> * log. Something worked...
> */
> @@ -4800,8 +4809,6 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
> skb_dst_drop(skb);
> __skb_pull(skb, tcp_hdr(skb)->doff * 4);
>
> - tcp_ecn_accept_cwr(sk, skb);
> -
> tp->rx_opt.dsack = 0;
>
> /* Queue data for delivery to the user.
> --
Thanks for the patch!
Acked-by: Neal Cardwell <ncardwell@...gle.com>
neal
Powered by blists - more mailing lists