From patchwork Mon Feb 4 12:14:25 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ilpo_J=C3=A4rvinen?= X-Patchwork-Id: 217890 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id EF54A2C02A8 for ; Mon, 4 Feb 2013 23:19:38 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754207Ab3BDMTf (ORCPT ); Mon, 4 Feb 2013 07:19:35 -0500 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 X-Greylist: delayed 306 seconds by postgrey-1.27 at vger.kernel.org; Mon, 04 Feb 2013 07:19:33 EST Received: from wel-95.cs.helsinki.fi (wel-95.cs.helsinki.fi [128.214.10.211]) (TLS: TLSv1/SSLv3,256bits,AES256-SHA) by mail.cs.helsinki.fi with esmtp; Mon, 04 Feb 2013 14:14:25 +0200 id 0008C5D8.510FA621.000064B2 Date: Mon, 4 Feb 2013 14:14:25 +0200 (EET) From: "=?ISO-8859-15?Q?Ilpo_J=E4rvinen?=" X-X-Sender: ijjarvin@wel-95.cs.helsinki.fi To: Eric Dumazet cc: Neal Cardwell , "=?ISO-8859-15?Q?Pasi_K=E4rkk=E4inen?=" , David Miller , Hannes Frederic Sowa , Stephen Hemminger , Netdev , Yuchung Cheng Subject: Re: [PATCH] tcp: frto should not set snd_cwnd to 0 In-Reply-To: <1359918785.30177.111.camel@edumazet-glaptop> 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> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-ID: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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? Tested-by: Eric Dumazet Acked-by: Neal Cardwell --- [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));