From patchwork Mon Nov 10 03:56:25 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lawrence Stewart X-Patchwork-Id: 7972 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.176.167]) by ozlabs.org (Postfix) with ESMTP id 2DBECDDE00 for ; Mon, 10 Nov 2008 15:11:35 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752931AbYKJEJl (ORCPT ); Sun, 9 Nov 2008 23:09:41 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752875AbYKJEJl (ORCPT ); Sun, 9 Nov 2008 23:09:41 -0500 Received: from lauren.room52.net ([210.50.193.198]:62062 "EHLO lauren.room52.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752681AbYKJEJk (ORCPT ); Sun, 9 Nov 2008 23:09:40 -0500 X-Greylist: delayed 758 seconds by postgrey-1.27 at vger.kernel.org; Sun, 09 Nov 2008 23:09:39 EST Received: from lstewart.caia.swin.edu.au (lstewart.caia.swin.edu.au [136.186.229.95]) (authenticated bits=0) by lauren.room52.net (8.14.3/8.14.3) with ESMTP id mAA3uUkH073098 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 10 Nov 2008 14:56:32 +1100 (EST) (envelope-from lstewart@room52.net) Message-ID: <4917B0E9.2030304@room52.net> Date: Mon, 10 Nov 2008 14:56:25 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.16 (X11/20080806) MIME-Version: 1.0 To: Douglas Leith CC: Netdev , Stephen Hemminger , Lachlan Andrew , grenville armitage Subject: Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix References: <0E5A87BC-8FA8-40A5-8032-9FA18D07D7CE@nuim.ie> In-Reply-To: <0E5A87BC-8FA8-40A5-8032-9FA18D07D7CE@nuim.ie> X-Spam-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_NEUTRAL autolearn=disabled version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on lauren.room52.net Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Doug, Thanks very much for looking at this. My familiarity with the Linux TCP stack is not up to scratch as you know. I'm curious about the "if (ca->undo_last_cong)" condition you added to htcp_cwnd_undo(). Is there a possibility of the stack calling into this function multiple times without at least one congestion control related state change in between? I will test your patch and confirm it resolves the issue we reported. Also, while we're at it, any chance you might consider adding the functionality in the attached patch so that we can control adaptive backoff via a module tunable? Cheers, Lawrence Douglas Leith wrote: > This patch fixes a minor bug in tcp_htcp.c which has been highlighted by > Lachlan Andrew and Lawrence Stewart. Currently, the time since the > last congestion event, which is stored in variable last_cong, is reset > whenever there is a state change into TCP_CA_Open. This includes > transitions of the type TCP_CA_Open->TCP_CA_Disorder->TCP_CA_Open which > are not associated with backoff of cwnd. The patch changes last_cong to > be updated only on transitions into TCP_CA_Open that occur after > experiencing the congestion-related states TCP_CA_Loss, TCP_CA_Recovery, > TCP_CA_CWR. > > Doug > > --- tcp_htcp.c 2008-11-05 15:50:18.000000000 +0000 > +++ tcp_htcp.c.new 2008-11-08 07:11:18.000000000 +0000 > @@ -69,9 +69,12 @@ > const struct tcp_sock *tp = tcp_sk(sk); > struct htcp *ca = inet_csk_ca(sk); > > - ca->last_cong = ca->undo_last_cong; > - ca->maxRTT = ca->undo_maxRTT; > - ca->old_maxB = ca->undo_old_maxB; > + if (ca->undo_last_cong) { > + ca->last_cong = ca->undo_last_cong; > + ca->maxRTT = ca->undo_maxRTT; > + ca->old_maxB = ca->undo_old_maxB; > + ca->undo_last_cong=0; // flag that ca->last_cong is not > to be reset when enter TCP_CA_Open state > + } > > return max(tp->snd_cwnd, (tp->snd_ssthresh << 7) / ca->beta); > } > @@ -265,12 +268,15 @@ > static void htcp_state(struct sock *sk, u8 new_state) > { > switch (new_state) { > - case TCP_CA_Open: > + case TCP_CA_Open: > { > struct htcp *ca = inet_csk_ca(sk); > - ca->last_cong = jiffies; > + if (ca->undo_last_cong) { > + ca->last_cong=jiffies; > + ca->undo_last_cong=0; > + } > } > - break; > + break; > case TCP_CA_CWR: > case TCP_CA_Recovery: > case TCP_CA_Loss: > > --- /usr/src/linux-source-2.6.25/net/ipv4/tcp_htcp.c 2008-04-17 12:49:44.000000000 +1000 +++ tcp_htcp.c 2008-10-24 17:02:27.000000000 +1100 @@ -22,6 +22,10 @@ module_param(use_bandwidth_switch, int, 0644); MODULE_PARM_DESC(use_bandwidth_switch, "turn on/off bandwidth switcher"); +static int use_adaptive_backoff __read_mostly = 1; +module_param(use_adaptive_backoff, int, 0644); +MODULE_PARM_DESC(use_adaptive_backoff, "turn on/off adaptive backoff"); + struct htcp { u32 alpha; /* Fixed point arith, << 7 */ u8 beta; /* Fixed point arith, << 7 */ @@ -155,7 +159,7 @@ } } - if (ca->modeswitch && minRTT > msecs_to_jiffies(10) && maxRTT) { + if (use_adaptive_backoff && ca->modeswitch && minRTT > msecs_to_jiffies(10) && maxRTT) { ca->beta = (minRTT << 7) / maxRTT; if (ca->beta < BETA_MIN) ca->beta = BETA_MIN;