[PATCH net-next] tcp: ack immediately when a cwr packet arrives

[PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Lawrence Brakmo]

We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
problem is triggered when the last packet of a request arrives CE
marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
to 1 (because there are no packets in flight). When the 1st packet of
the next request arrives, the ACK was sometimes delayed even though it
is CWR marked, adding up to 40ms to the RPC latency.

This patch insures that CWR marked data packets arriving will be acked
immediately.

Packetdrill script to reproduce the problem:

0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
0.000 setsockopt(3, SOL_TCP, TCP_CONGESTION, "dctcp", 5) = 0
0.000 bind(3, ..., ...) = 0
0.000 listen(3, 1) = 0

0.100 < [ect0] SEW 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
0.100 > SE. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>
0.110 < [ect0] . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4

0.200 < [ect0] . 1:1001(1000) ack 1 win 257
0.200 > [ect01] . 1:1(0) ack 1001

0.200 write(4, ..., 1) = 1
0.200 > [ect01] P. 1:2(1) ack 1001

0.200 < [ect0] . 1001:2001(1000) ack 2 win 257
0.200 write(4, ..., 1) = 1
0.200 > [ect01] P. 2:3(1) ack 2001

0.200 < [ect0] . 2001:3001(1000) ack 3 win 257
0.200 < [ect0] . 3001:4001(1000) ack 3 win 257
0.200 > [ect01] . 3:3(0) ack 4001

0.210 < [ce] P. 4001:4501(500) ack 3 win 257

+0.001 read(4, ..., 4500) = 4500
+0 write(4, ..., 1) = 1
+0 > [ect01] PE. 3:4(1) ack 4501

+0.010 < [ect0] W. 4501:5501(1000) ack 4 win 257
// Previously the ACK sequence below would be 4501, causing a long RTO
+0.040~+0.045 > [ect01] . 4:4(0) ack 5501   // delayed ack

+0.311 < [ect0] . 5501:6501(1000) ack 4 win 257  // More data
+0 > [ect01] . 4:4(0) ack 6501     // now acks everything

+0.500 < F. 9501:9501(0) ack 4 win 257

Modified based on comments by Neal Cardwell <ncardwell@google.com>

Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
 net/ipv4/tcp_input.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 91dbb9afb950..2370fd79c5c5 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -246,8 +246,15 @@ static void tcp_ecn_queue_cwr(struct tcp_sock *tp)
 
 static void tcp_ecn_accept_cwr(struct tcp_sock *tp, const struct sk_buff *skb)
 {
-	if (tcp_hdr(skb)->cwr)
+	if (tcp_hdr(skb)->cwr) {
 		tp->ecn_flags &= ~TCP_ECN_DEMAND_CWR;
+
+		/* If the sender is telling us it has entered CWR, then its
+		 * cwnd may be very low (even just 1 packet), so we should ACK
+		 * immediately.
+		 */
+		tcp_enter_quickack_mode((struct sock *)tp, 2);
+	}
 }
 
 static void tcp_ecn_withdraw_cwr(struct tcp_sock *tp)
-- 
2.17.1

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Neal Cardwell]

On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
> problem is triggered when the last packet of a request arrives CE
> marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
> to 1 (because there are no packets in flight). When the 1st packet of
> the next request arrives, the ACK was sometimes delayed even though it
> is CWR marked, adding up to 40ms to the RPC latency.
>
> This patch insures that CWR marked data packets arriving will be acked
> immediately.
...
> Modified based on comments by Neal Cardwell <ncardwell@google.com>
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
>  net/ipv4/tcp_input.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Seems like a nice mechanism to have, IMHO.

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

Thanks!
neal

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Daniel Borkmann]

On 07/24/2018 04:15 AM, Neal Cardwell wrote:
> On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>>
>> We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
>> problem is triggered when the last packet of a request arrives CE
>> marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
>> to 1 (because there are no packets in flight). When the 1st packet of
>> the next request arrives, the ACK was sometimes delayed even though it
>> is CWR marked, adding up to 40ms to the RPC latency.
>>
>> This patch insures that CWR marked data packets arriving will be acked
>> immediately.
> ...
>> Modified based on comments by Neal Cardwell <ncardwell@google.com>
>>
>> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
>> ---
>>  net/ipv4/tcp_input.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> Seems like a nice mechanism to have, IMHO.
> 
> Acked-by: Neal Cardwell <ncardwell@google.com>

Should this go to net tree instead where all the other fixes went?

Thanks,
Daniel

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Yuchung Cheng]

On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 07/24/2018 04:15 AM, Neal Cardwell wrote:
> > On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo <brakmo@fb.com> wrote:
> >>
> >> We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
> >> problem is triggered when the last packet of a request arrives CE
> >> marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
> >> to 1 (because there are no packets in flight). When the 1st packet of
> >> the next request arrives, the ACK was sometimes delayed even though it
> >> is CWR marked, adding up to 40ms to the RPC latency.
> >>
> >> This patch insures that CWR marked data packets arriving will be acked
> >> immediately.
> > ...
> >> Modified based on comments by Neal Cardwell <ncardwell@google.com>
> >>
> >> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> >> ---
> >>  net/ipv4/tcp_input.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > Seems like a nice mechanism to have, IMHO.
> >
> > Acked-by: Neal Cardwell <ncardwell@google.com>
>
> Should this go to net tree instead where all the other fixes went?
I am neutral but this feels more like a feature improvement

Acked-by: Yuchung Cheng <ycheng@google.com>

btw this should also help the classic ECN case upon timeout that
triggers one packet retransmission.
>
> Thanks,
> Daniel

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Neal Cardwell]

On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
>
> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Should this go to net tree instead where all the other fixes went?
> I am neutral but this feels more like a feature improvement

I agree this feels like a feature improvement rather than a bug fix.

neal

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Eric Dumazet]

On 07/24/2018 10:12 AM, Neal Cardwell wrote:
> On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
>>
>> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> Should this go to net tree instead where all the other fixes went?
>> I am neutral but this feels more like a feature improvement
> 
> I agree this feels like a feature improvement rather than a bug fix.

Agreed

Signed-off-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Lawrence Brakmo]

Note that without this fix the 99% latencies when doing 10KB RPCs in a congested network using DCTCP are 40ms vs. 190us with the patch. Also note that these 40ms high tail latencies started after commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 in Jul 2015, which triggered the bugs/features we are fixing/adding. I agree it is a debatable whether it is a bug fix or a feature improvement and I am fine either way.

Lawrence

On 7/24/18, 10:13 AM, "Neal Cardwell" <ncardwell@google.com> wrote:

    On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
    >
    > On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
    > > Should this go to net tree instead where all the other fixes went?
    > I am neutral but this feels more like a feature improvement
    
    I agree this feels like a feature improvement rather than a bug fix.
    
    neal
    

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[Neal Cardwell]

On Tue, Jul 24, 2018 at 1:42 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> Note that without this fix the 99% latencies when doing 10KB RPCs
> in a congested network using DCTCP are 40ms vs. 190us with the patch.
> Also note that these 40ms high tail latencies started after commit
> 3759824da87b30ce7a35b4873b62b0ba38905ef5 in Jul 2015,
> which triggered the bugs/features we are fixing/adding. I agree it is a
> debatable whether it is a bug fix or a feature improvement and I am
> fine either way.

Good point. The fact that this greatly mitigates a regression in DCTCP
performance resulting from 3759824da87b30ce7a35b4873b62b0ba38905ef5
("tcp: PRR uses CRB mode by default and SS mode conditionally") IMHO
seems to be a good argument for putting this patch ("tcp: ack
immediately when a cwr packet arrives") in the "net" branch and stable
releases.

thanks,
neal

Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Tue, 24 Jul 2018 21:57:27 -0400

> On Tue, Jul 24, 2018 at 1:42 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>>
>> Note that without this fix the 99% latencies when doing 10KB RPCs
>> in a congested network using DCTCP are 40ms vs. 190us with the patch.
>> Also note that these 40ms high tail latencies started after commit
>> 3759824da87b30ce7a35b4873b62b0ba38905ef5 in Jul 2015,
>> which triggered the bugs/features we are fixing/adding. I agree it is a
>> debatable whether it is a bug fix or a feature improvement and I am
>> fine either way.
> 
> Good point. The fact that this greatly mitigates a regression in DCTCP
> performance resulting from 3759824da87b30ce7a35b4873b62b0ba38905ef5
> ("tcp: PRR uses CRB mode by default and SS mode conditionally") IMHO
> seems to be a good argument for putting this patch ("tcp: ack
> immediately when a cwr packet arrives") in the "net" branch and stable
> releases.

Thus, applied to 'net' and queued up for -stable.