Skip to contentSkip to conversation

fix limited slow start bug

PATCH In review John Heffner 3 participants 5 messages Feb 22, 2007 – Feb 23, 2007

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

John Heffner

author
#0 lore raw

PATCH 1/1 fix limited slow start bug

Fix arithmetic order bug in limited slow start. The subtraction needs to be done before snd_cwnd is incremented.

Signed-off-by John Heffner <[email protected]>

---
commit 244e7411d99443df7b7ae849ba6ebbec4c2342bc
tree e6d5985a22448f59f8bef393542e1d5497ee5684
parent 97033fa201705e6cfc68ce66f34ede3277c3d645
author John Heffner <[email protected]> Thu, 22 Feb 2007 13:54:01 -0500 committer John Heffner <[email protected]> Thu, 22 Feb 2007 13:54:01 -0500

 net/ipv4/tcp_cong.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/ipv4/tcp_cong.c +1 -1
index 7fd2910..a0c894f 100644
@@ -303,9 +303,9 @@ void tcp_slow_start(struct tcp_sock *tp)
 	
 	tp->snd_cwnd_cnt += cnt;
 	while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
+		tp->snd_cwnd_cnt -= tp->snd_cwnd;
 		if (tp->snd_cwnd < tp->snd_cwnd_clamp)
 			tp->snd_cwnd++;
-		tp->snd_cwnd_cnt -= tp->snd_cwnd;
 	}
 }
 EXPORT_SYMBOL_GPL(tcp_slow_start);
#1 lore raw
Quoted 5 lines · John Heffner
net/ipv4/tcp_cong.c +0 -0
index 7fd2910..a0c894f 100644

net/ipv4/tcp_cong.c void tcp_slow_start(struct tcp_sock *tp) · line 311

Quoted 10 lines
net/ipv4/tcp_cong.c +1 -1
@@ -303,9 +303,9 @@ void tcp_slow_start(struct tcp_sock *tp)
 
 	tp->snd_cwnd_cnt += cnt;
 	while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
+		tp->snd_cwnd_cnt -= tp->snd_cwnd;
 		if (tp->snd_cwnd < tp->snd_cwnd_clamp)
 			tp->snd_cwnd++;
-		tp->snd_cwnd_cnt -= tp->snd_cwnd;
 	}
 }
 EXPORT_SYMBOL_GPL(tcp_slow_start);

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?

Signature · 1 line
 i.
author
#2 lore raw
Quoted 5 lines · Ilpo Järvinen

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

maintainer
#3 lore raw

From: John Heffner <[email protected]>
Date: Thu, 22 Feb 2007 16:52:03 -0500

Quoted 3 lines

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 <[email protected]>
Date:   Thu Feb 22 22:52:59 2007 -0800

[TCP]: Make snd_cwnd_clamp a u32.

Signed-off-by David S. Miller <[email protected]>
include/linux/tcp.h +1 -1
index 415193e..18a468d 100644
@@ -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;
maintainer
#4 lore raw
In reply to John Heffner

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:

net/ipv4/tcp_cong.c +0 -0
index 7fd2910..a0c894f 100644
@@ -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.