Re: bad TSO performance in 2.6.9-rc2-BK

["David S. Miller" <davem@davemloft.net>]

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

On Tue, 28 Sep 2004 07:32:33 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, Sep 27, 2004 at 12:01:54PM -0700, David S. Miller wrote:
> >
> > > tcp_current_mss() doesn't call tcp_sync_mss() unless the PMTU changes.
> > 
> > Good catch, probably we should make it do so when sk_route_caps
> > indicates we are doing TSO.
> 
> Alternatively we could move the TSO code out of tcp_sync_mss() and
> put it in tcp_current_mss() instead.  It seems to be the only one
> using the factor anyway.

Ok, here are 2 patches incorporating all of the things
we discussed in this area:

1) Uninline tcp_current_mss(), fix tcp_sync_mss() return
   value to match tcp_current_mss()'s

2) Fix the do_large calculation bug in tcp_current_mss() as
   per Herbert's original patch.

3) Move TSO mss calculation work to tcp_current_mss().  We have
   to do something like this since tcp_sync_mss() is only invoked
   when the PMTU changes whereas the TSO MTU is dependant upon
   both the path and the current congestion window.

So, this patch should wrap up these issues.


[-- Attachment #2: diff1 --]
[-- Type: application/octet-stream, Size: 3591 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/09/28 13:26:54-07:00 davem@nuts.davemloft.net 
#   [TCP]: Uninline tcp_current_mss().
#   
#   Also fix the return value of tcp_sync_mss() to
#   be unsigned.
#   
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/ipv4/tcp_output.c
#   2004/09/28 13:26:01-07:00 davem@nuts.davemloft.net +31 -1
#   [TCP]: Uninline tcp_current_mss().
# 
# include/net/tcp.h
#   2004/09/28 13:26:00-07:00 davem@nuts.davemloft.net +2 -32
#   [TCP]: Uninline tcp_current_mss().
# 
diff -Nru a/include/net/tcp.h b/include/net/tcp.h
--- a/include/net/tcp.h	2004-09-28 13:49:22 -07:00
+++ b/include/net/tcp.h	2004-09-28 13:49:22 -07:00
@@ -961,7 +961,8 @@
 
 extern void tcp_delete_keepalive_timer (struct sock *);
 extern void tcp_reset_keepalive_timer (struct sock *, unsigned long);
-extern int tcp_sync_mss(struct sock *sk, u32 pmtu);
+extern unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu);
+extern unsigned int tcp_current_mss(struct sock *sk, int large);
 
 extern const char timer_bug_msg[];
 
@@ -1033,37 +1034,6 @@
 	default:
 		printk(timer_bug_msg);
 	};
-}
-
-/* Compute the current effective MSS, taking SACKs and IP options,
- * and even PMTU discovery events into account.
- *
- * LARGESEND note: !urg_mode is overkill, only frames up to snd_up
- * cannot be large. However, taking into account rare use of URG, this
- * is not a big flaw.
- */
-
-static inline unsigned int tcp_current_mss(struct sock *sk, int large)
-{
-	struct tcp_opt *tp = tcp_sk(sk);
-	struct dst_entry *dst = __sk_dst_get(sk);
-	int do_large, mss_now;
-
-	do_large = (large &&
-		    (sk->sk_route_caps & NETIF_F_TSO) &&
-		    !tp->urg_mode);
-	mss_now = do_large ? tp->mss_cache : tp->mss_cache_std;
-
-	if (dst) {
-		u32 mtu = dst_pmtu(dst);
-		if (mtu != tp->pmtu_cookie ||
-		    tp->ext2_header_len != dst->header_len)
-			mss_now = tcp_sync_mss(sk, mtu);
-	}
-	if (tp->eff_sacks)
-		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
-			    (tp->eff_sacks * TCPOLEN_SACK_PERBLOCK));
-	return mss_now;
 }
 
 /* Initialize RCV_MSS value.
diff -Nru a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
--- a/net/ipv4/tcp_output.c	2004-09-28 13:49:22 -07:00
+++ b/net/ipv4/tcp_output.c	2004-09-28 13:49:22 -07:00
@@ -603,7 +603,7 @@
    this function.			--ANK (980731)
  */
 
-int tcp_sync_mss(struct sock *sk, u32 pmtu)
+unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu)
 {
 	struct tcp_opt *tp = tcp_sk(sk);
 	struct dst_entry *dst = __sk_dst_get(sk);
@@ -661,6 +661,36 @@
 	return mss_now;
 }
 
+/* Compute the current effective MSS, taking SACKs and IP options,
+ * and even PMTU discovery events into account.
+ *
+ * LARGESEND note: !urg_mode is overkill, only frames up to snd_up
+ * cannot be large. However, taking into account rare use of URG, this
+ * is not a big flaw.
+ */
+
+unsigned int tcp_current_mss(struct sock *sk, int large)
+{
+	struct tcp_opt *tp = tcp_sk(sk);
+	struct dst_entry *dst = __sk_dst_get(sk);
+	int do_large, mss_now;
+
+	do_large = (large &&
+		    (sk->sk_route_caps & NETIF_F_TSO) &&
+		    !tp->urg_mode);
+	mss_now = do_large ? tp->mss_cache : tp->mss_cache_std;
+
+	if (dst) {
+		u32 mtu = dst_pmtu(dst);
+		if (mtu != tp->pmtu_cookie ||
+		    tp->ext2_header_len != dst->header_len)
+			mss_now = tcp_sync_mss(sk, mtu);
+	}
+	if (tp->eff_sacks)
+		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
+			    (tp->eff_sacks * TCPOLEN_SACK_PERBLOCK));
+	return mss_now;
+}
 
 /* This routine writes packets to the network.  It advances the
  * send_head.  This happens as incoming acks open up the remote

[-- Attachment #3: diff2 --]
[-- Type: application/octet-stream, Size: 2712 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/09/28 13:46:58-07:00 davem@nuts.davemloft.net 
#   [TCP]: Move TSO mss calcs to tcp_current_mss()
#   
#   Based upon a bug fix patch and suggestions from
#   Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/ipv4/tcp_output.c
#   2004/09/28 13:46:28-07:00 davem@nuts.davemloft.net +29 -24
#   [TCP]: Move TSO mss calcs to tcp_current_mss()
#   
#   Based upon a bug fix patch and suggestions from
#   Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
diff -Nru a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
--- a/net/ipv4/tcp_output.c	2004-09-28 13:49:37 -07:00
+++ b/net/ipv4/tcp_output.c	2004-09-28 13:49:37 -07:00
@@ -639,25 +639,6 @@
 	tp->pmtu_cookie = pmtu;
 	tp->mss_cache = tp->mss_cache_std = mss_now;
 
-	if (sk->sk_route_caps & NETIF_F_TSO) {
-		int large_mss, factor;
-
-		large_mss = 65535 - tp->af_specific->net_header_len -
-			tp->ext_header_len - tp->ext2_header_len - tp->tcp_header_len;
-
-		if (tp->max_window && large_mss > (tp->max_window>>1))
-			large_mss = max((tp->max_window>>1), 68U - tp->tcp_header_len);
-
-		/* Always keep large mss multiple of real mss, but
-		 * do not exceed congestion window.
-		 */
-		factor = large_mss / mss_now;
-		if (factor > tp->snd_cwnd)
-			factor = tp->snd_cwnd;
-
-		tp->mss_cache = mss_now * factor;
-	}
-
 	return mss_now;
 }
 
@@ -675,17 +656,41 @@
 	struct dst_entry *dst = __sk_dst_get(sk);
 	int do_large, mss_now;
 
-	do_large = (large &&
-		    (sk->sk_route_caps & NETIF_F_TSO) &&
-		    !tp->urg_mode);
-	mss_now = do_large ? tp->mss_cache : tp->mss_cache_std;
-
+	mss_now = tp->mss_cache_std;
 	if (dst) {
 		u32 mtu = dst_pmtu(dst);
 		if (mtu != tp->pmtu_cookie ||
 		    tp->ext2_header_len != dst->header_len)
 			mss_now = tcp_sync_mss(sk, mtu);
 	}
+
+	do_large = (large &&
+		    (sk->sk_route_caps & NETIF_F_TSO) &&
+		    !tp->urg_mode);
+
+	if (do_large) {
+		int large_mss, factor;
+
+		large_mss = 65535 - tp->af_specific->net_header_len -
+			tp->ext_header_len - tp->ext2_header_len -
+			tp->tcp_header_len;
+
+		if (tp->max_window && large_mss > (tp->max_window>>1))
+			large_mss = max((tp->max_window>>1),
+					68U - tp->tcp_header_len);
+
+		/* Always keep large mss multiple of real mss, but
+		 * do not exceed congestion window.
+		 */
+		factor = large_mss / mss_now;
+		if (factor > tp->snd_cwnd)
+			factor = tp->snd_cwnd;
+
+		tp->mss_cache = mss_now * factor;
+
+		mss_now = tp->mss_cache;
+	}
+
 	if (tp->eff_sacks)
 		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
 			    (tp->eff_sacks * TCPOLEN_SACK_PERBLOCK));