From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lawrence Brakmo Subject: Re: [PATCH net-next 1/2] tcp: notify when a delayed ack is sent Date: Mon, 2 Jul 2018 21:24:19 +0000 Message-ID: References: <20180630014815.2881895-1-brakmo@fb.com> <20180630014815.2881895-2-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]:43974 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753476AbeGBVYc (ORCPT ); Mon, 2 Jul 2018 17:24:32 -0400 In-Reply-To: Content-Language: en-US Content-ID: <0244EF1EE5A37F48BCD194AE6949DFD5@namprd15.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: On 7/2/18, 8:18 AM, +ACI-netdev-owner+AEA-vger.kernel.org on behalf of Neal= Cardwell+ACI- +ADw-netdev-owner+AEA-vger.kernel.org on behalf of 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- DCTCP depends on the CA+AF8-EVENT+AF8-NON+AF8-DELAYED+AF8-ACK and= CA+AF8-EVENT+AF8-DELAYED+AF8-ACK +AD4- notifications to keep track if it needs to send an ACK for packet= s that +AD4- were received with a particular ECN state but whose ACK was delay= ed. +AD4- +AD4- Under some circumstances, for example when a delayed ACK is sent = with a +AD4- data packet, DCTCP state was not being updated due to a lack of +AD4- notification that the previously delayed ACK was sent. As a resul= t, it +AD4- would sometimes send a duplicate ACK when a new data packet arriv= ed. +AD4- +AD4- This patch insures that DCTCP's state is correctly updated so it = will +AD4- not send the duplicate ACK. +AD4- +AD4- Signed-off-by: Lawrence Brakmo +ADw-brakmo+AEA-fb.com+AD4- +AD4- --- +AD4- net/ipv4/tcp+AF8-output.c +AHw- 2 +-+- +AD4- 1 file changed, 2 insertions(+-) +AD4- +AD4- diff --git a/net/ipv4/tcp+AF8-output.c b/net/ipv4/tcp+AF8-output.= c +AD4- index f8f6129160dd..41f6ad7a21e4 100644 +AD4- --- a/net/ipv4/tcp+AF8-output.c +AD4- +-+-+- b/net/ipv4/tcp+AF8-output.c +AD4- +AEAAQA- -172,6 +-172,8 +AEAAQA- static inline void tcp+AF8-event= +AF8-ack+AF8-sent(struct sock +ACo-sk, unsigned int pkts) +AD4- +AF8AXw-sock+AF8-put(sk)+ADs- +AD4- +AH0- +AD4- tcp+AF8-dec+AF8-quickack+AF8-mode(sk, pkts)+ADs- +AD4- +- if (inet+AF8-csk+AF8-ack+AF8-scheduled(sk)) +AD4- +- tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF= 8-DELAYED+AF8-ACK)+ADs- +AD4- inet+AF8-csk+AF8-clear+AF8-xmit+AF8-timer(sk, ICSK+AF8-TI= ME+AF8-DACK)+ADs- +AD4- +AH0- =20 Thanks for this fix+ACE- Seems like this would work, but if I am readin= g this correctly then it seems like this would cause a duplicate call to tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELAYED+AF8-ACK) when= we are sending a pure ACK (delayed or non-delayed): =20 (1) once from tcp+AF8-send+AF8-ack() before we send the ACK: =20 tcp+AF8-send+AF8-ack(struct sock +ACo-sk) -+AD4- tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELAYE= D+AF8-ACK)+ADs- =20 (2) then again from tcp+AF8-event+AF8-ack+AF8-sent() after we have sent= the ACK: =20 tcp+AF8-event+AF8-ack+AF8-sent() -+AD4- if (inet+AF8-csk+AF8-ack+AF8-scheduled(sk)) tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELA= YED+AF8-ACK)+ADs- =20 What if we remove the original CA+AF8-EVENT+AF8-NON+AF8-DELAYED+AF8-ACK= call and just replace it with your new one? (not compiled, not tested): =20 diff --git a/net/ipv4/tcp+AF8-output.c b/net/ipv4/tcp+AF8-output.c index 3889dcd4868d4..bddb49617d9be 100644 --- a/net/ipv4/tcp+AF8-output.c +-+-+- b/net/ipv4/tcp+AF8-output.c +AEAAQA- -184,6 +-184,8 +AEAAQA- static inline void tcp+AF8-event+AF8-a= ck+AF8-sent(struct sock +ACo-sk, unsigned int pkts) +AF8AXw-sock+AF8-put(sk)+ADs- +AH0- tcp+AF8-dec+AF8-quickack+AF8-mode(sk, pkts)+ADs- +- if (inet+AF8-csk+AF8-ack+AF8-scheduled(sk)) +- tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELA= YED+AF8-ACK)+ADs- inet+AF8-csk+AF8-clear+AF8-xmit+AF8-timer(sk, ICSK+AF8-TIME+AF8= -DACK)+ADs- +AH0- =20 +AEAAQA- -3836,8 +-3838,6 +AEAAQA- void tcp+AF8-send+AF8-ack(struct soc= k +ACo-sk) if (sk-+AD4-sk+AF8-state +AD0APQ- TCP+AF8-CLOSE) return+ADs- =20 - tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELAYED+AF8-A= CK)+ADs- - /+ACo- We are not putting this on the write queue, so +ACo- tcp+AF8-transmit+AF8-skb() will set the ownership to thi= s +ACo- sock. =20 Aside from lower CPU overhead, one nice benefit of that is that we then only call tcp+AF8-ca+AF8-event(sk, CA+AF8-EVENT+AF8-NON+AF8-DELAYE= D+AF8-ACK) in one place, which might be a little easier to reason about. =20 Does that work? =20 neal Thanks Neal, good catch+ACE- I will resubmit the patch. =20