From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lawrence Brakmo Subject: Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives Date: Sun, 1 Jul 2018 01:46:38 +0000 Message-ID: References: <20180630014815.2881895-1-brakmo@fb.com> <20180630014815.2881895-3-brakmo@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-7" Content-Transfer-Encoding: quoted-printable Cc: Netdev , Kernel Team , "Blake Matheny" , Alexei Starovoitov , Yuchung Cheng , Steve Ibanez , Eric Dumazet To: Neal Cardwell Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:32942 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752015AbeGABrG (ORCPT ); Sat, 30 Jun 2018 21:47:06 -0400 In-Reply-To: Content-Language: en-US Content-ID: Sender: netdev-owner@vger.kernel.org List-ID: On 6/30/18, 11:23 AM, +ACI-Neal Cardwell+ACI- +ADw-ncardwell+AEA-google.com= +AD4- wrote: On Fri, Jun 29, 2018 at 9:48 PM Lawrence Brakmo +ADw-brakmo+AEA-fb.com+= AD4- wrote: +AD4- +AD4- We observed high 99 and 99.9+ACU- latencies when doing RPCs with = DCTCP. The +AD4- problem is triggered when the last packet of a request arrives CE +AD4- marked. The reply will carry the ECE mark causing TCP to shrink i= ts cwnd +AD4- to 1 (because there are no packets in flight). When the 1st packe= t of +AD4- the next request arrives it was sometimes delayed adding up to 40= ms to +AD4- the latency. +AD4- +AD4- This patch insures that CWR makred data packets arriving will be = acked +AD4- immediately. +AD4- +AD4- Signed-off-by: Lawrence Brakmo +ADw-brakmo+AEA-fb.com+AD4- +AD4- --- =20 Thanks, Larry. Ensuring that CWR-marked data packets arriving will be acked immediately sounds like a good goal to me. =20 I am wondering if we can have a simpler fix. =20 The dctcp+AF8-ce+AF8-state+AF8-0+AF8-to+AF8-1() code is setting the TCP= +AF8-ECN+AF8-DEMAND+AF8-CWR bit in ecn+AF8-flags, which disables the code in +AF8AXw-tcp+AF8-ecn+AF= 8-check+AF8-ce() that would have otherwise used the tcp+AF8-enter+AF8-quickack+AF8-mode() mec= hanism to force an ACK: =20 +AF8AXw-tcp+AF8-ecn+AF8-check+AF8-ce(): ... case INET+AF8-ECN+AF8-CE: if (tcp+AF8-ca+AF8-needs+AF8-ecn(sk)) tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-ECN+AF8-IS+AF8-CE)+ADs- // -+AD4- dctcp+AF8-ce+AF8-state+AF8-0+AF8-to+AF8-1() // -+AD4- tp-+AD4-ecn+AF8-flags +AHwAPQ- TCP+AF8-ECN+AF8-DE= MAND+AF8-CWR+ADs- =20 if (+ACE-(tp-+AD4-ecn+AF8-flags +ACY- TCP+AF8-ECN+AF8-DEMAND+AF8-CWR)= ) +AHs- /+ACo- Better not delay acks, sender can have a very low cwnd +ACo-= / tcp+AF8-enter+AF8-quickack+AF8-mode(sk, 1)+ADs- tp-+AD4-ecn+AF8-flags +AHwAPQ- TCP+AF8-ECN+AF8-DEMAND+AF8-CWR+ADs- +AH0- tp-+AD4-ecn+AF8-flags +AHwAPQ- TCP+AF8-ECN+AF8-SEEN+ADs- break+ADs- =20 So it seems like the bug here may be that dctcp+AF8-ce+AF8-state+AF8-0+= AF8-to+AF8-1() is setting the TCP+AF8-ECN+AF8-DEMAND+AF8-CWR bit in ecn+AF8-flags, when = really it should let its caller, +AF8AXw-tcp+AF8-ecn+AF8-check+AF8-ce() set TCP+A= F8-ECN+AF8-DEMAND+AF8-CWR, in which case the code would already properly force an immediate ACK. =20 So, what if we use this fix instead (not compiled, not tested): =20 diff --git a/net/ipv4/tcp+AF8-dctcp.c b/net/ipv4/tcp+AF8-dctcp.c index 5f5e5936760e..4fecd2824edb 100644 --- a/net/ipv4/tcp+AF8-dctcp.c +-+-+- b/net/ipv4/tcp+AF8-dctcp.c +AEAAQA- -152,8 +-152,6 +AEAAQA- static void dctcp+AF8-ce+AF8-state+AF8= -0+AF8-to+AF8-1(struct sock +ACo-sk) =20 ca-+AD4-prior+AF8-rcv+AF8-nxt +AD0- tp-+AD4-rcv+AF8-nxt+ADs- ca-+AD4-ce+AF8-state +AD0- 1+ADs- - - tp-+AD4-ecn+AF8-flags +AHwAPQ- TCP+AF8-ECN+AF8-DEMAND+AF8-CWR+A= Ds- +AH0- =20 static void dctcp+AF8-ce+AF8-state+AF8-1+AF8-to+AF8-0(struct sock +ACo= -sk) =20 What do you think? =20 neal I see two issues, one is that entering quickack mode as you mentioned does = not insure that it will still be on when the CWR arrives. The second issue = is that the problem occurs right after the receiver sends a small reply whi= ch results in entering pingpong mode right before the sender starts the new= request by sending just one packet (i.e. forces delayed ack). I compiled and tested your patch. Both 99 and 99.9 percentile latencies are= around 40ms. Looking at the packet traces shows that some CWR marked packe= ts are not being ack immediately (delayed by 40ms). Larry =20