[PATCH] fix limited slow start bug

(no body)

On Thu, 22 Feb 2007, John Heffner wrote:

ACK. I was going to track this down tomorrow as I noticed strange
behavior during slow start while testing tcp-2.6 today but I don't have to 
anymore :-). The problem I saw is now obvious, whole congestion control 
was practically disabled due to underflow of the unsigned type that occurs 
without this patch, causing TCP to be limited only by the receiver 
advertized window during slow-start.


BTW, while looking this patch, I noticed that snd_cwnd_clamp is only u16 
while snd_cwnd is u32, which seems rather strange since snd_cwnd is being 
limited by the clamp value here and there?!?! And tcp_highspeed.c is 
clearly assuming even more than this (but the problem is hidden as 
snd_cwnd_clamp is feed back to the min_t and the used 32-bit constant 
could be safely cut to 16-bits anyway):

  tp->snd_cwnd_clamp = min_t(u32, tp->snd_cwnd_clamp, 0xffffffff/128);

Has the type being changed somewhere in the past or why is this so?


-- 
 i.

Ilpo Järvinen wrote:

BTW, while looking this patch, I noticed that snd_cwnd_clamp is only u16 
while snd_cwnd is u32, which seems rather strange since snd_cwnd is being 
limited by the clamp value here and there?!?! And tcp_highspeed.c is 
clearly assuming even more than this (but the problem is hidden as 
snd_cwnd_clamp is feed back to the min_t and the used 32-bit constant 
could be safely cut to 16-bits anyway):

  tp->snd_cwnd_clamp = min_t(u32, tp->snd_cwnd_clamp, 0xffffffff/128);

Has the type being changed somewhere in the past or why is this so?

It's been that way as long as I can remember.  It's always been a 
mystery to me as well.  I suspect the tcp_highspeed code is that way 
because this patch originally came out of the Web100-patched kernel, 
which at one point was using a 32 bit snd_cwnd_clamp IIRC.

I think it's not unreasonable to change clamp to 32 bits now, since with 
1500 byte packets, this corresponds to a max cwnd of ~94MB.  This is 
pretty big, but we are currently right at this limit with 10 GigE.

   -John

John, what tree did you diff this against?

I can tell you didn't create this patch against anything
actually in any of my trees, because:

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 7fd2910..a0c894f 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c

@@ -303,9 +303,9 @@ void tcp_slow_start(struct tcp_sock *tp)
 	
 	tp->snd_cwnd_cnt += cnt;

See that line right before the snd_cwnd_cnt increment?

There is a tab there in your patch on that empty line.

Yoshifuji eliminated all extraneous trailing whitespaces,
and spaces that should be tabs, across the entire networking,
several weeks ago.  Including the tab on that empty line in
your patch.

This means that you applied the YeaH and your own patch to
another source tree, perhaps 2.6.20 or similar, and then
generated your fix against that.

Please don't do things like that.  Instead please produce patches
against the tree they will end up being applied to so that they will
apply cleanly for me.

I can't believe how much time I spend getting people to produce
correct patches :-/

Anyways, I applied this by hand, but next time I definitely
am not going to.

From: John Heffner <redacted>
Date: Thu, 22 Feb 2007 16:52:03 -0500

I think it's not unreasonable to change clamp to 32 bits now, since with 
1500 byte packets, this corresponds to a max cwnd of ~94MB.  This is 
pretty big, but we are currently right at this limit with 10 GigE.

Agreed, and done in tcp-2.6.git as below.

What should we do about that 65535 assignment in hybla?

commit cedfa95566512730202bb4abed5d9118e74bab30
Author: David S. Miller [off-list ref]
Date:   Thu Feb 22 22:52:59 2007 -0800

    [TCP]: Make snd_cwnd_clamp a u32.
    
    Signed-off-by: David S. Miller [off-list ref]

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 415193e..18a468d 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h

@@ -302,7 +302,7 @@ struct tcp_sock {
  	u32	snd_ssthresh;	/* Slow start size threshold		*/
  	u32	snd_cwnd;	/* Sending congestion window		*/
  	u16	snd_cwnd_cnt;	/* Linear increase counter		*/
-	u16	snd_cwnd_clamp; /* Do not allow snd_cwnd to grow above this */
+	u32	snd_cwnd_clamp; /* Do not allow snd_cwnd to grow above this */
 	u32	snd_cwnd_used;
 	u32	snd_cwnd_stamp;