From mboxrd@z Thu Jan 1 00:00:00 1970 From: "=?ISO-8859-15?Q?Ilpo_J=E4rvinen?=" Subject: Re: [PATCH] tcp: frto should not set snd_cwnd to 0 Date: Mon, 4 Feb 2013 14:14:25 +0200 (EET) Message-ID: References: <20130123161238.GE8912@reaktio.net> <20130123214445.GA16641@order.stressinduktion.org> <20130123215151.GF8912@reaktio.net> <20130123152642.4a8389ba@nehalam.linuxnetplumber.net> <20130123234116.GC16641@order.stressinduktion.org> <1358984831.12374.1227.camel@edumazet-glaptop> <20130124135120.GD16641@order.stressinduktion.org> <1359777110.30177.58.camel@edumazet-glaptop> <20130202142832.GP8912@reaktio.net> <1359818075.30177.78.camel@edumazet-glaptop> <1359826377.30177.86.camel@edumazet-glaptop> <1359918785.30177.111.camel@edumazet-glaptop> Mime-Version: 1.0 Content-Type: MULTIPART/MIXED; boundary="8323329-296175798-1359979150=:4561" Cc: Neal Cardwell , "=?ISO-8859-15?Q?Pasi_K=E4rkk=E4inen?=" , David Miller , Hannes Frederic Sowa , Stephen Hemminger , Netdev , Yuchung Cheng To: Eric Dumazet Return-path: Received: from courier.cs.helsinki.fi ([128.214.9.1]:42706 "EHLO mail.cs.helsinki.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753854Ab3BDMTd (ORCPT ); Mon, 4 Feb 2013 07:19:33 -0500 In-Reply-To: <1359918785.30177.111.camel@edumazet-glaptop> Content-ID: Sender: netdev-owner@vger.kernel.org List-ID: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-296175798-1359979150=:4561 Content-Type: TEXT/PLAIN; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Sun, 3 Feb 2013, Eric Dumazet wrote: > From: Eric Dumazet > > Commit 9dc274151a548 (tcp: fix ABC in tcp_slow_start()) > uncovered a bug in FRTO code : > tcp_process_frto() is setting snd_cwnd to 0 if the number > of in flight packets is 0. > > As Neal pointed out, if no packet is in flight we lost our > chance to disambiguate whether a loss timeout was spurious. > > We should assume it was a proper loss. > > Reported-by: Pasi Kärkkäinen > Signed-off-by: Neal Cardwell > Signed-off-by: Eric Dumazet > Cc: Ilpo Järvinen > Cc: Yuchung Cheng > --- > net/ipv4/tcp_input.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 8aca4ee..680c422 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3484,7 +3484,8 @@ static bool tcp_process_frto(struct sock *sk, int flag) > ((tp->frto_counter >= 2) && (flag & FLAG_RETRANS_DATA_ACKED))) > tp->undo_marker = 0; > > - if (!before(tp->snd_una, tp->frto_highmark)) { > + if (!before(tp->snd_una, tp->frto_highmark) || > + !tcp_packets_in_flight(tp)) { I think this condition becomes now too broad because there is transient during FRTO. I think the patch below would be enough to resolve this, what do you think? -- [PATCH 1/1] tcp: fix for zero packets_in_flight was too broad There are transients during normal FRTO procedure during which the packets_in_flight can go to zero between write_queue state updates and firing the resulting segments out. As FRTO processing occurs during that window the check must be more precise to not match "spuriously" :-). More specificly, e.g., when packets_in_flight is zero but FLAG_DATA_ACKED is true the problematic branch that set cwnd into zero would not be taken and new segments might be sent out later. Only compile tested. Signed-off-by: Ilpo Järvinen Cc: Neal Cardwell Cc: Eric Dumazet Cc: Pasi Kärkkäinen --- net/ipv4/tcp_input.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 680c422..500c2da 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3484,8 +3484,7 @@ static bool tcp_process_frto(struct sock *sk, int flag) ((tp->frto_counter >= 2) && (flag & FLAG_RETRANS_DATA_ACKED))) tp->undo_marker = 0; - if (!before(tp->snd_una, tp->frto_highmark) || - !tcp_packets_in_flight(tp)) { + if (!before(tp->snd_una, tp->frto_highmark)) { tcp_enter_frto_loss(sk, (tp->frto_counter == 1 ? 2 : 3), flag); return true; } @@ -3505,6 +3504,11 @@ static bool tcp_process_frto(struct sock *sk, int flag) } } else { if (!(flag & FLAG_DATA_ACKED) && (tp->frto_counter == 1)) { + if (!tcp_packets_in_flight(tp)) { + tcp_enter_frto_loss(sk, 2, flag); + return true; + } + /* Prevent sending of new data. */ tp->snd_cwnd = min(tp->snd_cwnd, tcp_packets_in_flight(tp)); -- 1.7.0.4 --8323329-296175798-1359979150=:4561--