Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Neal Cardwell
Date: Tue, 24 Jul 2018 21:57:27 -0400
> On Tue, Jul 24, 2018 at 1:42 PM Lawrence Brakmo 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.
Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
On Tue, Jul 24, 2018 at 1:42 PM Lawrence Brakmo 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
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" wrote: On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng wrote: > > On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann 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
On 07/24/2018 10:12 AM, Neal Cardwell wrote: > On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng wrote: >> >> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann >> 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
Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng wrote: > > On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann 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
On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann wrote: > > On 07/24/2018 04:15 AM, Neal Cardwell wrote: > > On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo 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 > >> > >> Signed-off-by: Lawrence Brakmo > >> --- > >> 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 > > 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 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
On 07/24/2018 04:15 AM, Neal Cardwell wrote: > On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo 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 >> >> Signed-off-by: Lawrence Brakmo >> --- >> 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 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
On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo 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 > > Signed-off-by: Lawrence Brakmo > --- > 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 Thanks! neal
[PATCH net-next] tcp: ack immediately when a cwr packet arrives
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
0.100 > SE. 0:0(0) ack 1
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
Signed-off-by: Lawrence Brakmo
---
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
