[PATCH net] tcp: grow window for OOO packets only for SACK flows

[PATCH net] tcp: grow window for OOO packets only for SACK flows

[Eric Dumazet]

Back in 2013, we made a change that broke fast retransmit
for non SACK flows.

Indeed, for these flows, a sender needs to receive three duplicate
ACK before starting fast retransmit. Sending ACK with different
receive window do not count.

Even if enabling SACK is strongly recommended these days,
there still are some cases where it has to be disabled.

Not increasing the window seems better than having to
rely on RTO.

After the fix, following packetdrill test gives :

// Initialize connection
    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 32792 <mss 1000,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,wscale 8>
   +0 < . 1:1(0) ack 1 win 514

   +0 accept(3, ..., ...) = 4

   +0 < . 1:1001(1000) ack 1 win 514
// Quick ack
   +0 > . 1:1(0) ack 1001 win 264

   +0 < . 2001:3001(1000) ack 1 win 514
// DUPACK : Normally we should not change the window
   +0 > . 1:1(0) ack 1001 win 264

   +0 < . 3001:4001(1000) ack 1 win 514
// DUPACK : Normally we should not change the window
   +0 > . 1:1(0) ack 1001 win 264

   +0 < . 4001:5001(1000) ack 1 win 514
// DUPACK : Normally we should not change the window
    +0 > . 1:1(0) ack 1001 win 264

   +0 < . 1001:2001(1000) ack 1 win 514
// Hole is repaired.
   +0 > . 1:1(0) ack 5001 win 272

Fixes: 4e4f1fc22681 ("tcp: properly increase rcv_ssthresh for ofo packets")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
---
 net/ipv4/tcp_input.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 83330a6cb24209cf6ac60d634245b7bc151ee6ac..12fda8f27b08bdf5c9f3bad422734f6b1965cef9 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4605,7 +4605,11 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
 	if (tcp_ooo_try_coalesce(sk, tp->ooo_last_skb,
 				 skb, &fragstolen)) {
 coalesce_done:
-		tcp_grow_window(sk, skb);
+		/* For non sack flows, do not grow window to force DUPACK
+		 * and trigger fast retransmit.
+		 */
+		if (tcp_is_sack(tp))
+			tcp_grow_window(sk, skb);
 		kfree_skb_partial(skb, fragstolen);
 		skb = NULL;
 		goto add_sack;
@@ -4689,7 +4693,11 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
 		tcp_sack_new_ofo_skb(sk, seq, end_seq);
 end:
 	if (skb) {
-		tcp_grow_window(sk, skb);
+		/* For non sack flows, do not grow window to force DUPACK
+		 * and trigger fast retransmit.
+		 */
+		if (tcp_is_sack(tp))
+			tcp_grow_window(sk, skb);
 		skb_condense(skb);
 		skb_set_owner_r(skb, sk);
 	}
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH net] tcp: grow window for OOO packets only for SACK flows

[Neal Cardwell]

On Mon, Jun 15, 2020 at 11:37 PM Eric Dumazet <edumazet@google.com> wrote:
>
> Back in 2013, we made a change that broke fast retransmit
> for non SACK flows.
>
> Indeed, for these flows, a sender needs to receive three duplicate
> ACK before starting fast retransmit. Sending ACK with different
> receive window do not count.
>
> Even if enabling SACK is strongly recommended these days,
> there still are some cases where it has to be disabled.
>
> Not increasing the window seems better than having to
> rely on RTO.
...
> Fixes: 4e4f1fc22681 ("tcp: properly increase rcv_ssthresh for ofo packets")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
> ---

Thanks for the fix, Eric!

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH net] tcp: grow window for OOO packets only for SACK flows

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 15 Jun 2020 20:37:07 -0700

> Back in 2013, we made a change that broke fast retransmit
> for non SACK flows.
> 
> Indeed, for these flows, a sender needs to receive three duplicate
> ACK before starting fast retransmit. Sending ACK with different
> receive window do not count.
> 
> Even if enabling SACK is strongly recommended these days,
> there still are some cases where it has to be disabled.
> 
> Not increasing the window seems better than having to
> rely on RTO.
> 
> After the fix, following packetdrill test gives :
 ...
> Fixes: 4e4f1fc22681 ("tcp: properly increase rcv_ssthresh for ofo packets")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>

Applied and queued up for -stable, thanks Eric.