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: Mon, 2 Jul 2018 21:24:27 +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 mx0b-00082601.pphosted.com ([67.231.153.30]:59406 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753605AbeGBVYh (ORCPT ); Mon, 2 Jul 2018 17:24:37 -0400 In-Reply-To: Content-Language: en-US Content-ID: <16F59732849A2C4A93FF9084F22C0C94@namprd15.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: On 7/2/18, 7:57 AM, +ACI-Neal Cardwell+ACI- +ADw-ncardwell+AEA-google.com+A= D4- wrote: On Sat, Jun 30, 2018 at 9:47 PM Lawrence Brakmo +ADw-brakmo+AEA-fb.com+= AD4- wrote: +AD4- I see two issues, one is that entering quickack mode as you +AD4- mentioned does not insure that it will still be on when the CWR +AD4- arrives. The second issue is that the problem occurs right after = the +AD4- receiver sends a small reply which results in entering pingpong m= ode +AD4- right before the sender starts the new request by sending just on= e +AD4- packet (i.e. forces delayed ack). +AD4- +AD4- I compiled and tested your patch. Both 99 and 99.9 percentile +AD4- latencies are around 40ms. Looking at the packet traces shows tha= t +AD4- some CWR marked packets are not being ack immediately (delayed by +AD4- 40ms). =20 Thanks, Larry+ACE- So your tests provide nice, specific evidence that i= t is good to force an immediate ACK when a receiver receives a packet with CWR marked. Given that, I am wondering what the simplest way is to achieve that goal. =20 What if, rather than plumbing a new specific signal into +AF8AXw-tcp+AF8-ack+AF8-snd+AF8-check(), we use the existing general qu= ick-ack mechanism, where various parts of the TCP stack (like +AF8AXw-tcp+AF8-ecn+AF8-check+AF8-ce()) are already using the quick-ac= k mechanism to +ACI-remotely+ACI- signal to +AF8AXw-tcp+AF8-ack+AF8-snd+AF8-check() th= at they want an immediate ACK. =20 For example, would it work to do something like: =20 diff --git a/net/ipv4/tcp+AF8-input.c b/net/ipv4/tcp+AF8-input.c index c53ae5fc834a5..8168d1938b376 100644 --- a/net/ipv4/tcp+AF8-input.c +-+-+- b/net/ipv4/tcp+AF8-input.c +AEAAQA- -262,6 +-262,12 +AEAAQA- static void +AF8AXw-tcp+AF8-ecn+AF8-c= heck+AF8-ce(struct sock +ACo-sk, const struct sk+AF8-buff +ACo-skb) +AHs- struct tcp+AF8-sock +ACo-tp +AD0- tcp+AF8-sk(sk)+ADs- =20 +- /+ACo- If the sender is telling us it has entered CWR, then it= s cwnd may be +- +ACo- very low (even just 1 packet), so we should ACK immedia= tely. +- +ACo-/ +- if (tcp+AF8-hdr(skb)-+AD4-cwr) +- tcp+AF8-enter+AF8-quickack+AF8-mode(sk, 2)+ADs- +- switch (TCP+AF8-SKB+AF8-CB(skb)-+AD4-ip+AF8-dsfield +ACY- INET+= AF8-ECN+AF8-MASK) +AHs- case INET+AF8-ECN+AF8-NOT+AF8-ECT: /+ACo- Insure that GCN will not continue to mark packet= s. +ACo-/ =20 And then since that broadens the mission of this function beyond checking just the ECT/CE bits, I supposed we could rename the +AF8AXw-tcp+AF8-ecn+AF8-check+AF8-ce() and tcp+AF8-ecn+AF8-check+AF8-ce= () functions to +AF8AXw-tcp+AF8-ecn+AF8-check() and tcp+AF8-ecn+AF8-check(), or somethi= ng like that. =20 Would that work for this particular issue? =20 Neal Thanks Neal, it does work and is cleaner than what I was doing. I will subm= it a revised patch set.=20 =20