[PATCH 2.6.27] tcp_htcp: last_cong bug fix

[PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Douglas Leith]

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:

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Lawrence Stewart]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

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:
> 
> 


[-- Attachment #2: switch_adaptive_backoff.patch --]
[-- Type: text/plain, Size: 849 bytes --]

--- /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;

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Douglas Leith]

> 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?
>
It looks like this is possible, but in a limited way that seems  
safe.  The call to the undo code is from tcp_undo_cwr() which in turn  
is called from one of the following

* tcp_try_undo_recovery() - this is only called when the state is  
TCP_CA_Loss or TCP_CA_Recovery and the code path then makes a state  
change to TCP_CA_Open after calling undo.
* tcp_try_undo_loss() - this is called when the state is TCP_CA_Loss   
and after undo also enters TCP_CA_Open
* tcp_try_undo_dsack() - this is called from state TCP_CA_Disorder
* tcp_undo_spur_to_response() - part of the FRTO code which also  
seems to occur in the TCP_CA_Disorder state.

So calls to the undo code from the states TCP_CA_Loss or  
TCP_CA_Recovery always exit with a transition to state TCP_CA_Open.   
Therefore multiple calls to the undo code from the loss and recovery  
states without a state change don't seem to be possible.

Multiple calls to the undo code do seem to be possible from within  
the TCP_CA_Disorder state without a state transition.  But this is  
ok.  Cwnd is not backed off in this state and so there is no undo  
action to take (at least with respect to cwnd, ssthresh and last_cong  
- the undo calls seem to be related to other book-keeping).   Both  
the original htcp code and the new code only store undo information  
on entering CP_CA_Loss, TCP_CA_Recovery or TCP_CWR but not on a  
transition into TCP_CA_Disorder.  The proposed patch disables the  
undo functionality in this situation since there is no valid undo  
information stored - this fixes a bug in the original code which  
would have attempted an undo using spurious undo information.

Hope that makes sense ;-)

> I will test your patch and confirm it resolves the issue we reported.
>
Thanks.

> 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?
>
Makes sense to me.   Why not submit it as a patch via a separate email.

Doug

> 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;

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[David Miller]

From: Douglas Leith <Doug.Leith@nuim.ie>
Date: Sun, 09 Nov 2008 19:09:24 +0000

> 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.

Thank you for this fix.  Could you fix a few things up for
me and resubmit this?

1) Please provide a proper Signed-off-by: line as per
   linux/Documentation/SubmittingPatches

2) Please -p1 root your patches at the top level of
   the kernel source tree, again the SubmittingPatches
   document helps explain ways to do this

3) Please fix up a few coding style errors, such as:

> +               ca->undo_last_cong=0; // flag that ca->last_cong is not to be reset when enter TCP_CA_Open state

   Please don't use C++ style comments, use normal C ones,
   and please don't let the line exceed much more than 80
   columns.

   Also, please put one space on each side of the equal sign in the
   assignment.

> +                       if (ca->undo_last_cong) {
> +                               ca->last_cong=jiffies;
> +                               ca->undo_last_cong=0;
> +                       }

   Again, please put spaces around the assignment equal sign.

Thanks.

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[David Miller]

From: Douglas Leith <Doug.Leith@nuim.ie>
Date: Mon, 10 Nov 2008 10:07:32 +0000

> Makes sense to me.   Why not submit it as a patch via a separate email.

Yes, and please submit it properly as per
linux/Documentation/SubmittingPatches

Thank you.

[PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Douglas Leith]

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.

Signed-off-by:  Doug Leith <doug.leith@nuim.ie>

---

Revised patch with requested style changes.

--- net/ipv4/tcp_htcp.c 2008-11-05 15:50:18.000000000 +0000
+++ net/ipv4/tcp_htcp.c.new     2008-11-11 15:20:31.000000000 +0000
@@ -71,5 +71,8 @@ static u32 htcp_cwnd_undo(struct sock *s

-       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;
+       }

@@ -267,8 +270,11 @@ static void htcp_state(struct sock *sk,
         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:

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[David Miller]

From: Douglas Leith <Doug.Leith@nuim.ie>
Date: Tue, 11 Nov 2008 15:33:33 +0000

> Revised patch with requested style changes.

Your email client corrupted the patch, turning tab characters
into spaces.

Please turn off content formatting in your email client,
or alternatively use an attachment for the patch itself.

There is some help in linux/Documentation/email-clients.txt

Please fix this up and resubmit, thank you very much.

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Douglas Leith]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Patch attached.

On 11 Nov 2008, at 22:16, David Miller wrote:

> From: Douglas Leith <Doug.Leith@nuim.ie>
> Date: Tue, 11 Nov 2008 15:33:33 +0000
>
>> Revised patch with requested style changes.
>
> Your email client corrupted the patch, turning tab characters
> into spaces.
>
> Please turn off content formatting in your email client,
> or alternatively use an attachment for the patch itself.
>
> There is some help in linux/Documentation/email-clients.txt
>
> Please fix this up and resubmit, thank you very much.

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 813 bytes --]

--- net/ipv4/tcp_htcp.c	2008-11-05 15:50:18.000000000 +0000
+++ net/ipv4/tcp_htcp.c.new	2008-11-11 15:20:31.000000000 +0000
@@ -71,5 +71,8 @@ static u32 htcp_cwnd_undo(struct sock *s
 
-	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;  
+	}
 
@@ -267,8 +270,11 @@ static void htcp_state(struct sock *sk, 
 	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:

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[David Miller]

From: Douglas Leith <Doug.Leith@nuim.ie>
Date: Wed, 12 Nov 2008 08:36:30 +0000

> Patch attached.

I wanted you to make a full resubmission with the patch fixed up.

Otherwise I don't have your commit message, and it isn't all collected
properly when your patch is logged to:

	http://patchwork.ozlabs.org/project/netdev/list

When you are requested to fixup a patch or make some modification,
always, and I do mean always, make a fresh new submission (changelog
and all) unless you are told otherwise.

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[Douglas Leith]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

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.

Signed-off-by:  Doug Leith <doug.leith@nuim.ie>

---

Resending with patch as attachment.   Hope this does the trick.


--- net/ipv4/tcp_htcp.c 2008-11-05 15:50:18.000000000 +0000
+++ net/ipv4/tcp_htcp.c.new     2008-11-11 15:20:31.000000000 +0000
@@ -71,5 +71,8 @@ static u32 htcp_cwnd_undo(struct sock *s

-       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;
+       }

@@ -267,8 +270,11 @@ static void htcp_state(struct sock *sk,
         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:



[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 813 bytes --]

--- net/ipv4/tcp_htcp.c	2008-11-05 15:50:18.000000000 +0000
+++ net/ipv4/tcp_htcp.c.new	2008-11-11 15:20:31.000000000 +0000
@@ -71,5 +71,8 @@ static u32 htcp_cwnd_undo(struct sock *s
 
-	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;  
+	}
 
@@ -267,8 +270,11 @@ static void htcp_state(struct sock *sk, 
 	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:

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: [PATCH 2.6.27] tcp_htcp: last_cong bug fix

[David Miller]

From: Douglas Leith <Doug.Leith@nuim.ie>
Date: Wed, 12 Nov 2008 08:58:18 +0000

> 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.
> 
> Signed-off-by:  Doug Leith <doug.leith@nuim.ie>

Applied, but I still had to fix up a bunch of details :-(

+		ca->undo_last_cong = 0;  

That line had trailing whitespace.

+	case TCP_CA_Open: 

That one too.

+        		if (ca->undo_last_cong) { 

That line had an initial set of space characters, instead
of pure leading tab characters.

+				ca->last_cong = jiffies;   

More trailing whitespace.

+		break;	

And some more trailing whitespace.