[PATCH net-2.6.25 0/8] [NET]: More uninlining & related

[PATCH net-2.6.25 0/8] [NET]: More uninlining & related

[Ilpo Järvinen]

Hi Dave,

First of all, I changed output encoding to utf-8, so I guess
the encoding should not cause trouble for you.

Here are couple of more to uninline things. Pretty
straightforward except the EXPORT_SYMBOLs, I've no idea which
is the right variant (feel free to fix them while applying :-)).
Also pktgen uninlining is somewhat questionable because it's
just a testing tool so feel free to drop it if it feels
unnecessary (I could have asked first but it's just as easy to
do it this way if not easier)...

There were more dead static inlines found after inlines removed
(gcc didn't report them otherwise) than in pktgen now included,
but I'm not sure if I should send "by default" patches removing
or #if 0'ing them?


--
 i.

ps. I apologize that I must resend to get them to netdev as well
because git-send-email of this system (not sure if later could)
still seems to be lacking proper encoding of my name when it
decides to add it to Cc list all by itself and those 8-bit chars
in address got rejected.

[PATCH 1/8] [TCP]: Uninline tcp_set_state

[Ilpo Järvinen]

net/ipv4/tcp.c:
  tcp_close_state | -226
  tcp_done        | -145
  tcp_close       | -564
  tcp_disconnect  | -141
 4 functions changed, 1076 bytes removed, diff: -1076

net/ipv4/tcp_input.c:
  tcp_fin               |  -86
  tcp_rcv_state_process | -164
 2 functions changed, 250 bytes removed, diff: -250

net/ipv4/tcp_ipv4.c:
  tcp_v4_connect | -209
 1 function changed, 209 bytes removed, diff: -209

net/ipv4/arp.c:
  arp_ignore |   +5
 1 function changed, 5 bytes added, diff: +5

net/ipv6/tcp_ipv6.c:
  tcp_v6_connect | -158
 1 function changed, 158 bytes removed, diff: -158

net/sunrpc/xprtsock.c:
  xs_sendpages |   -2
 1 function changed, 2 bytes removed, diff: -2

net/dccp/ccids/ccid3.c:
  ccid3_update_send_interval |   +7
 1 function changed, 7 bytes added, diff: +7

net/ipv4/tcp.c:
  tcp_set_state | +238
 1 function changed, 238 bytes added, diff: +238

built-in.o:
 12 functions changed, 250 bytes added, 1695 bytes removed, diff: -1445

I've no explanation why some unrelated changes seem to occur
consistently as well (arp_ignore, ccid3_update_send_interval;
I checked the arp_ignore asm and it seems to be due to some
reordered of operation order causing some extra opcodes to be
generated). Still, the benefits are pretty obvious from the
codiff's results.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Cc: Andi Kleen <andi@firstfloor.org>
---
 include/net/tcp.h |   35 +----------------------------------
 net/ipv4/tcp.c    |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 48081ad..306580c 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -926,40 +926,7 @@ static const char *statename[]={
 	"Close Wait","Last ACK","Listen","Closing"
 };
 #endif
-
-static inline void tcp_set_state(struct sock *sk, int state)
-{
-	int oldstate = sk->sk_state;
-
-	switch (state) {
-	case TCP_ESTABLISHED:
-		if (oldstate != TCP_ESTABLISHED)
-			TCP_INC_STATS(TCP_MIB_CURRESTAB);
-		break;
-
-	case TCP_CLOSE:
-		if (oldstate == TCP_CLOSE_WAIT || oldstate == TCP_ESTABLISHED)
-			TCP_INC_STATS(TCP_MIB_ESTABRESETS);
-
-		sk->sk_prot->unhash(sk);
-		if (inet_csk(sk)->icsk_bind_hash &&
-		    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
-			inet_put_port(&tcp_hashinfo, sk);
-		/* fall through */
-	default:
-		if (oldstate==TCP_ESTABLISHED)
-			TCP_DEC_STATS(TCP_MIB_CURRESTAB);
-	}
-
-	/* Change state AFTER socket is unhashed to avoid closed
-	 * socket sitting in hash tables.
-	 */
-	sk->sk_state = state;
-
-#ifdef STATE_TRACE
-	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n",sk, statename[oldstate],statename[state]);
-#endif	
-}
+extern void tcp_set_state(struct sock *sk, int state);
 
 extern void tcp_done(struct sock *sk);
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 34085e3..7d7b958 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1652,6 +1652,41 @@ recv_urg:
 	goto out;
 }
 
+void tcp_set_state(struct sock *sk, int state)
+{
+	int oldstate = sk->sk_state;
+
+	switch (state) {
+	case TCP_ESTABLISHED:
+		if (oldstate != TCP_ESTABLISHED)
+			TCP_INC_STATS(TCP_MIB_CURRESTAB);
+		break;
+
+	case TCP_CLOSE:
+		if (oldstate == TCP_CLOSE_WAIT || oldstate == TCP_ESTABLISHED)
+			TCP_INC_STATS(TCP_MIB_ESTABRESETS);
+
+		sk->sk_prot->unhash(sk);
+		if (inet_csk(sk)->icsk_bind_hash &&
+		    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
+			inet_put_port(&tcp_hashinfo, sk);
+		/* fall through */
+	default:
+		if (oldstate==TCP_ESTABLISHED)
+			TCP_DEC_STATS(TCP_MIB_CURRESTAB);
+	}
+
+	/* Change state AFTER socket is unhashed to avoid closed
+	 * socket sitting in hash tables.
+	 */
+	sk->sk_state = state;
+
+#ifdef STATE_TRACE
+	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n",sk, statename[oldstate],statename[state]);
+#endif	
+}
+EXPORT_SYMBOL_GPL(tcp_set_state);
+
 /*
  *	State processing on a close. This implements the state shift for
  *	sending our FIN frame. Note that we only send a FIN for some
-- 
1.5.0.6

[PATCH 2/8] [TCP]: Uninline tcp_is_cwnd_limited

[Ilpo Järvinen]

net/ipv4/tcp_cong.c:
  tcp_reno_cong_avoid |  -65
 1 function changed, 65 bytes removed, diff: -65

net/ipv4/arp.c:
  arp_ignore |   -5
 1 function changed, 5 bytes removed, diff: -5

net/ipv4/tcp_bic.c:
  bictcp_cong_avoid |  -57
 1 function changed, 57 bytes removed, diff: -57

net/ipv4/tcp_cubic.c:
  bictcp_cong_avoid |  -61
 1 function changed, 61 bytes removed, diff: -61

net/ipv4/tcp_highspeed.c:
  hstcp_cong_avoid |  -63
 1 function changed, 63 bytes removed, diff: -63

net/ipv4/tcp_hybla.c:
  hybla_cong_avoid |  -85
 1 function changed, 85 bytes removed, diff: -85

net/ipv4/tcp_htcp.c:
  htcp_cong_avoid |  -57
 1 function changed, 57 bytes removed, diff: -57

net/ipv4/tcp_veno.c:
  tcp_veno_cong_avoid |  -52
 1 function changed, 52 bytes removed, diff: -52

net/ipv4/tcp_scalable.c:
  tcp_scalable_cong_avoid |  -61
 1 function changed, 61 bytes removed, diff: -61

net/ipv4/tcp_yeah.c:
  tcp_yeah_cong_avoid |  -75
 1 function changed, 75 bytes removed, diff: -75

net/ipv4/tcp_illinois.c:
  tcp_illinois_cong_avoid |  -54
 1 function changed, 54 bytes removed, diff: -54

net/dccp/ccids/ccid3.c:
  ccid3_update_send_interval |   -7
  ccid3_hc_tx_packet_recv    |   +7
 2 functions changed, 7 bytes added, 7 bytes removed, diff: +0

net/ipv4/tcp_cong.c:
  tcp_is_cwnd_limited |  +88
 1 function changed, 88 bytes added, diff: +88

built-in.o:
 14 functions changed, 95 bytes added, 642 bytes removed, diff: -547

...Again some gcc artifacts visible as well.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 include/net/tcp.h   |   22 +---------------------
 net/ipv4/tcp_cong.c |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 306580c..7de4ea3 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -786,27 +786,7 @@ static inline u32 tcp_wnd_end(const struct tcp_sock *tp)
 {
 	return tp->snd_una + tp->snd_wnd;
 }
-
-/* RFC2861 Check whether we are limited by application or congestion window
- * This is the inverse of cwnd check in tcp_tso_should_defer
- */
-static inline int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight)
-{
-	const struct tcp_sock *tp = tcp_sk(sk);
-	u32 left;
-
-	if (in_flight >= tp->snd_cwnd)
-		return 1;
-
-	if (!sk_can_gso(sk))
-		return 0;
-
-	left = tp->snd_cwnd - in_flight;
-	if (sysctl_tcp_tso_win_divisor)
-		return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd;
-	else
-		return left <= tcp_max_burst(tp);
-}
+extern int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight);
 
 static inline void tcp_minshall_update(struct tcp_sock *tp, unsigned int mss,
 				       const struct sk_buff *skb)
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 4451750..3a6be23 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -274,6 +274,27 @@ int tcp_set_congestion_control(struct sock *sk, const char *name)
 	return err;
 }
 
+/* RFC2861 Check whether we are limited by application or congestion window
+ * This is the inverse of cwnd check in tcp_tso_should_defer
+ */
+int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight)
+{
+	const struct tcp_sock *tp = tcp_sk(sk);
+	u32 left;
+
+	if (in_flight >= tp->snd_cwnd)
+		return 1;
+
+	if (!sk_can_gso(sk))
+		return 0;
+
+	left = tp->snd_cwnd - in_flight;
+	if (sysctl_tcp_tso_win_divisor)
+		return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd;
+	else
+		return left <= tcp_max_burst(tp);
+}
+EXPORT_SYMBOL_GPL(tcp_is_cwnd_limited);
 
 /*
  * Slow start is used when congestion window is less than slow start
-- 
1.5.0.6

[PATCH 3/8] [XFRM] xfrm_policy: kill some bloat

[Ilpo Järvinen]

net/xfrm/xfrm_policy.c:
  xfrm_audit_policy_delete | -692
  xfrm_audit_policy_add    | -692
 2 functions changed, 1384 bytes removed, diff: -1384

net/xfrm/xfrm_policy.c:
  xfrm_audit_common_policyinfo | +704
 1 function changed, 704 bytes added, diff: +704

net/xfrm/xfrm_policy.o:
 3 functions changed, 704 bytes added, 1384 bytes removed, diff: -680

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/xfrm/xfrm_policy.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 534b29e..47219f9 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2367,8 +2367,8 @@ void __init xfrm_init(void)
 }
 
 #ifdef CONFIG_AUDITSYSCALL
-static inline void xfrm_audit_common_policyinfo(struct xfrm_policy *xp,
-						struct audit_buffer *audit_buf)
+static void xfrm_audit_common_policyinfo(struct xfrm_policy *xp,
+					 struct audit_buffer *audit_buf)
 {
 	struct xfrm_sec_ctx *ctx = xp->security;
 	struct xfrm_selector *sel = &xp->selector;
-- 
1.5.0.6

[PATCH 4/8] [IPV6] route: kill some bloat

[Ilpo Järvinen]

net/ipv6/route.c:
  ip6_pkt_prohibit_out | -130
  ip6_pkt_discard      | -261
  ip6_pkt_discard_out  | -130
  ip6_pkt_prohibit     | -261
 4 functions changed, 782 bytes removed, diff: -782

net/ipv6/route.c:
  ip6_pkt_drop | +300
 1 function changed, 300 bytes added, diff: +300

net/ipv6/route.o:
 5 functions changed, 300 bytes added, 782 bytes removed, diff: -482

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv6/route.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 02354a7..d3b5811 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1762,8 +1762,7 @@ int ipv6_route_ioctl(unsigned int cmd, void __user *arg)
  *	Drop the packet on the floor
  */
 
-static inline int ip6_pkt_drop(struct sk_buff *skb, int code,
-			       int ipstats_mib_noroutes)
+static int ip6_pkt_drop(struct sk_buff *skb, int code, int ipstats_mib_noroutes)
 {
 	int type;
 	switch (ipstats_mib_noroutes) {
-- 
1.5.0.6

[PATCH 5/8] [NETLINK] af_netlink: kill some bloat

[Ilpo Järvinen]

net/netlink/af_netlink.c:
  netlink_realloc_groups        |  -46
  netlink_insert                |  -49
  netlink_autobind              |  -94
  netlink_clear_multicast_users |  -48
  netlink_bind                  |  -55
  netlink_setsockopt            |  -54
  netlink_release               |  -86
  netlink_kernel_create         |  -47
  netlink_change_ngroups        |  -56
 9 functions changed, 535 bytes removed, diff: -535

net/netlink/af_netlink.c:
  netlink_table_ungrab |  +53
 1 function changed, 53 bytes added, diff: +53

net/netlink/af_netlink.o:
 10 functions changed, 53 bytes added, 535 bytes removed, diff: -482

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/netlink/af_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index be07f1b..21f9e30 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -193,7 +193,7 @@ static void netlink_table_grab(void)
 	}
 }
 
-static inline void netlink_table_ungrab(void)
+static void netlink_table_ungrab(void)
 	__releases(nl_table_lock)
 {
 	write_unlock_irq(&nl_table_lock);
-- 
1.5.0.6

[PATCH 6/8] [NETFILTER] xt_policy.c: kill some bloat

[Ilpo Järvinen]

net/netfilter/xt_policy.c:
  policy_mt | -906
 1 function changed, 906 bytes removed, diff: -906

net/netfilter/xt_policy.c:
  match_xfrm_state | +427
 1 function changed, 427 bytes added, diff: +427

net/netfilter/xt_policy.o:
 2 functions changed, 427 bytes added, 906 bytes removed, diff: -479

Alternatively, this could be done by combining identical
parts of the match_policy_in/out()

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/netfilter/xt_policy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_policy.c b/net/netfilter/xt_policy.c
index 46ee7e8..45731ca 100644
--- a/net/netfilter/xt_policy.c
+++ b/net/netfilter/xt_policy.c
@@ -33,7 +33,7 @@ xt_addr_cmp(const union xt_policy_addr *a1, const union xt_policy_addr *m,
 	return false;
 }
 
-static inline bool
+static bool
 match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
 		 unsigned short family)
 {
-- 
1.5.0.6

[PATCH 7/8] [PKTGEN]: Kill dead static inlines

[Ilpo Järvinen]

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/core/pktgen.c |   94 -----------------------------------------------------
 1 files changed, 0 insertions(+), 94 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index ede1fea..ebfb126 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -397,62 +397,6 @@ struct pktgen_thread {
 #define REMOVE 1
 #define FIND   0
 
-/*  This code works around the fact that do_div cannot handle two 64-bit
-    numbers, and regular 64-bit division doesn't work on x86 kernels.
-    --Ben
-*/
-
-#define PG_DIV 0
-
-/* This was emailed to LMKL by: Chris Caputo <ccaputo@alt.net>
- * Function copied/adapted/optimized from:
- *
- *  nemesis.sourceforge.net/browse/lib/static/intmath/ix86/intmath.c.html
- *
- * Copyright 1994, University of Cambridge Computer Laboratory
- * All Rights Reserved.
- *
- */
-static inline s64 divremdi3(s64 x, s64 y, int type)
-{
-	u64 a = (x < 0) ? -x : x;
-	u64 b = (y < 0) ? -y : y;
-	u64 res = 0, d = 1;
-
-	if (b > 0) {
-		while (b < a) {
-			b <<= 1;
-			d <<= 1;
-		}
-	}
-
-	do {
-		if (a >= b) {
-			a -= b;
-			res += d;
-		}
-		b >>= 1;
-		d >>= 1;
-	}
-	while (d);
-
-	if (PG_DIV == type) {
-		return (((x ^ y) & (1ll << 63)) == 0) ? res : -(s64) res;
-	} else {
-		return ((x & (1ll << 63)) == 0) ? a : -(s64) a;
-	}
-}
-
-/* End of hacks to deal with 64-bit math on x86 */
-
-/** Convert to milliseconds */
-static inline __u64 tv_to_ms(const struct timeval *tv)
-{
-	__u64 ms = tv->tv_usec / 1000;
-	ms += (__u64) tv->tv_sec * (__u64) 1000;
-	return ms;
-}
-
 /** Convert to micro-seconds */
 static inline __u64 tv_to_us(const struct timeval *tv)
 {
@@ -461,39 +405,6 @@ static inline __u64 tv_to_us(const struct timeval *tv)
 	return us;
 }
 
-static inline __u64 pg_div(__u64 n, __u32 base)
-{
-	__u64 tmp = n;
-	do_div(tmp, base);
-	/* printk("pktgen: pg_div, n: %llu  base: %d  rv: %llu\n",
-	   n, base, tmp); */
-	return tmp;
-}
-
-static inline __u64 pg_div64(__u64 n, __u64 base)
-{
-	__u64 tmp = n;
-/*
- * How do we know if the architecture we are running on
- * supports division with 64 bit base?
- *
- */
-#if defined(__sparc_v9__) || defined(__powerpc64__) || defined(__alpha__) || defined(__x86_64__) || defined(__ia64__)
-
-	do_div(tmp, base);
-#else
-	tmp = divremdi3(n, base, PG_DIV);
-#endif
-	return tmp;
-}
-
-static inline __u64 getCurMs(void)
-{
-	struct timeval tv;
-	do_gettimeofday(&tv);
-	return tv_to_ms(&tv);
-}
-
 static inline __u64 getCurUs(void)
 {
 	struct timeval tv;
@@ -501,11 +412,6 @@ static inline __u64 getCurUs(void)
 	return tv_to_us(&tv);
 }
 
-static inline __u64 tv_diff(const struct timeval *a, const struct timeval *b)
-{
-	return tv_to_us(a) - tv_to_us(b);
-}
-
 /* old include end */
 
 static char version[] __initdata = VERSION;
-- 
1.5.0.6

[RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Ilpo Järvinen]

net/core/pktgen.c:
  pktgen_stop_device   |  -50
  pktgen_run           | -105
  pktgen_if_show       |  -37
  pktgen_thread_worker | -702
 4 functions changed, 894 bytes removed, diff: -894

net/core/pktgen.c:
  getCurUs |  +36
 1 function changed, 36 bytes added, diff: +36

net/core/pktgen.o:
 5 functions changed, 36 bytes added, 894 bytes removed, diff: -858

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/core/pktgen.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index ebfb126..d18fdb1 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -405,7 +405,7 @@ static inline __u64 tv_to_us(const struct timeval *tv)
 	return us;
 }
 
-static inline __u64 getCurUs(void)
+static __u64 getCurUs(void)
 {
 	struct timeval tv;
 	do_gettimeofday(&tv);
-- 
1.5.0.6

Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 12 Jan 2008 11:34:22 +0200

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied, thanks.

> +#endif	
> +}
> +EXPORT_SYMBOL_GPL(tcp_set_state);

I fixed up the trailing whitespace on the "#endif" line.

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Herbert Xu]

Hi Ilpo:

On Sat, Jan 12, 2008 at 09:40:17AM +0000, Ilpo Järvinen wrote:

Your emails are now using UTF-8 encoding but it's still declaring
ISO-8859-1 as the charset.  So you probably want to fix that up or
your name may show up as Jävinen on the reader's screen.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1134 bytes --]

On Sat, 12 Jan 2008, Herbert Xu wrote:

> On Sat, Jan 12, 2008 at 09:40:17AM +0000, Ilpo Järvinen wrote:
> 
> Your emails are now using UTF-8 encoding but it's still declaring
> ISO-8859-1 as the charset. 

Thanks for trying to help but my situation is such that I think it got 
also you confused (this kind of mixed encoding is beoynd my skills 
really)... :-) Besides, I wouldn't mind of having incorrect characters
in my name, I'm just used to that but somebody else wasn't that happy 
about it.

Here's one example...

From: "=?utf-8?q?Ilpo_J=C3=A4rvinen?=" <ilpo.jarvinen@helsinki.fi>
...
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Something still needed besides these to declare email utf-8?

> So you probably want to fix that up or your name may show up as Jävinen 
> on the reader's screen.

Did you actually see this? I'd expect to see that as well but no, From is 
correctly decoded by my ISO-8859-1'ish MUA, aha, seems that I still have 
something to do to deal with the Signed-off line.

...Maybe I just fall-back to changing my last name, it's the only 
full-proof solution... ;-)

-- 
 i.

Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state

[Stephen Hemminger]

On Sat, 12 Jan 2008 11:40:10 +0200
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:

> net/ipv4/tcp.c:
>   tcp_close_state | -226
>   tcp_done        | -145
>   tcp_close       | -564
>   tcp_disconnect  | -141
>  4 functions changed, 1076 bytes removed, diff: -1076
> 
> net/ipv4/tcp_input.c:
>   tcp_fin               |  -86
>   tcp_rcv_state_process | -164
>  2 functions changed, 250 bytes removed, diff: -250
> 
> net/ipv4/tcp_ipv4.c:
>   tcp_v4_connect | -209
>  1 function changed, 209 bytes removed, diff: -209
> 
> net/ipv4/arp.c:
>   arp_ignore |   +5
>  1 function changed, 5 bytes added, diff: +5
> 
> net/ipv6/tcp_ipv6.c:
>   tcp_v6_connect | -158
>  1 function changed, 158 bytes removed, diff: -158
> 
> net/sunrpc/xprtsock.c:
>   xs_sendpages |   -2
>  1 function changed, 2 bytes removed, diff: -2
> 
> net/dccp/ccids/ccid3.c:
>   ccid3_update_send_interval |   +7
>  1 function changed, 7 bytes added, diff: +7
> 
> net/ipv4/tcp.c:
>   tcp_set_state | +238
>  1 function changed, 238 bytes added, diff: +238
> 
> built-in.o:
>  12 functions changed, 250 bytes added, 1695 bytes removed, diff: -1445
> 
> I've no explanation why some unrelated changes seem to occur
> consistently as well (arp_ignore, ccid3_update_send_interval;
> I checked the arp_ignore asm and it seems to be due to some
> reordered of operation order causing some extra opcodes to be
> generated). Still, the benefits are pretty obvious from the
> codiff's results.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> Cc: Andi Kleen <andi@firstfloor.org>
> ---
>  include/net/tcp.h |   35 +----------------------------------
>  net/ipv4/tcp.c    |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 48081ad..306580c 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -926,40 +926,7 @@ static const char *statename[]={
>  	"Close Wait","Last ACK","Listen","Closing"
>  };
>  #endif
> -
> -static inline void tcp_set_state(struct sock *sk, int state)
> -{
> -	int oldstate = sk->sk_state;
> -
> -	switch (state) {
> -	case TCP_ESTABLISHED:
> -		if (oldstate != TCP_ESTABLISHED)
> -			TCP_INC_STATS(TCP_MIB_CURRESTAB);
> -		break;
> -
> -	case TCP_CLOSE:
> -		if (oldstate == TCP_CLOSE_WAIT || oldstate == TCP_ESTABLISHED)
> -			TCP_INC_STATS(TCP_MIB_ESTABRESETS);
> -
> -		sk->sk_prot->unhash(sk);
> -		if (inet_csk(sk)->icsk_bind_hash &&
> -		    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
> -			inet_put_port(&tcp_hashinfo, sk);
> -		/* fall through */
> -	default:
> -		if (oldstate==TCP_ESTABLISHED)
> -			TCP_DEC_STATS(TCP_MIB_CURRESTAB);
> -	}
> -
> -	/* Change state AFTER socket is unhashed to avoid closed
> -	 * socket sitting in hash tables.
> -	 */
> -	sk->sk_state = state;
> -
> -#ifdef STATE_TRACE
> -	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n",sk, statename[oldstate],statename[state]);
> -#endif	
> -}
>


Since the function is called with a constant state, I guess the assumption
was that gcc would be smart enough to only include the code needed, it looks like
either code was bigger or the compiler was dumber than expected

-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Herbert Xu]

On Sat, Jan 12, 2008 at 02:59:50PM +0200, Ilpo Järvinen wrote:
>
> Here's one example...
> 
> From: "=?utf-8?q?Ilpo_J=C3=A4rvinen?=" <ilpo.jarvinen@helsinki.fi>
> ...
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: 8bit
> 
> Something still needed besides these to declare email utf-8?

If that's what it said then it'd be perfect.  However, quoting from this
very email that I'm replying to:

Content-Type: text/plain; charset=iso-8859-1

> > So you probably want to fix that up or your name may show up as Jävinen
> > on the reader's screen.
> 
> Did you actually see this? I'd expect to see that as well but no, From is
> correctly decoded by my ISO-8859-1'ish MUA, aha, seems that I still have
> something to do to deal with the Signed-off line.

Yeah I did see that.  Otherwise I wouldn't have noticed :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state

[Arnaldo Carvalho de Melo]

Em Sat, Jan 12, 2008 at 01:03:55PM -0800, Stephen Hemminger escreveu:
> On Sat, 12 Jan 2008 11:40:10 +0200
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:
> > -
> > -#ifdef STATE_TRACE
> > -	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n",sk, statename[oldstate],statename[state]);
> > -#endif	
> > -}
> >
> 
> 
> Since the function is called with a constant state, I guess the assumption
> was that gcc would be smart enough to only include the code needed, it looks like
> either code was bigger or the compiler was dumber than expected

Yup, that is one more instance where having proper tools to check if our
assumptions are right proves fruitful. I'm very happy for Ilpo to be
doing this work and making the case for us to work even harder on having
such tools to check if our assumptions are right.

- Arnaldo

P.S. subscribe to dwarves@vger.kernel.org and help us produce even more
such compiler watchdogs, right now changes to the dwarves are being made
such that we can check if the layout we think is optimal really holds to
that promise :-)

Soon I'll be looking at Ulrich's promising new disasm stuff :-)

Archives: http://news.gmane.org/gmane.comp.debugging.dwarves

- Arnaldo

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 12 Jan 2008 23:17:40 +1100

> Your emails are now using UTF-8 encoding but it's still declaring
> ISO-8859-1 as the charset.  So you probably want to fix that up or
> your name may show up as Jävinen on the reader's screen.

Yes, for people using the GIT email patch applying tools, it might do
that.  However I believe there are options to git-am and friends to
force it to put things into UTF-8.

I ran into some problems with encoding because I apply stuff by hand
using scripts I wrote.

But those problem won't happen any more for me.

I'm now saving the commit message and patch into files explicitly in
emacs, and verifying the coding (with "C-h C RET") and if it's wrong I
fix it before saving with "C-x RET f mule-utf-8"

Then I feed those into my patch applying scripts.

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 12 Jan 2008 14:59:50 +0200 (EET)

> ...Maybe I just fall-back to changing my last name, it's the only 
> full-proof solution... ;-)

Don't do this!  Otherwise I won't have a frequent test case to make
sure my patch applying scripts are working properly. :-))))

Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2871 bytes --]

On Sat, 12 Jan 2008, Stephen Hemminger wrote:

> On Sat, 12 Jan 2008 11:40:10 +0200
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:

...snip...

> > built-in.o:
> >  12 functions changed, 250 bytes added, 1695 bytes removed, diff: -1445

...snip...

> >  include/net/tcp.h |   35 +----------------------------------
> >  net/ipv4/tcp.c    |   35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+), 34 deletions(-)
> > 
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 48081ad..306580c 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -926,40 +926,7 @@ static const char *statename[]={
> >  	"Close Wait","Last ACK","Listen","Closing"
> >  };
> >  #endif
> > -
> > -static inline void tcp_set_state(struct sock *sk, int state)
> > -{
> > -	int oldstate = sk->sk_state;
> > -
> > -	switch (state) {
> > -	case TCP_ESTABLISHED:
> > -		if (oldstate != TCP_ESTABLISHED)
> > -			TCP_INC_STATS(TCP_MIB_CURRESTAB);
> > -		break;
> > -
> > -	case TCP_CLOSE:
> > -		if (oldstate == TCP_CLOSE_WAIT || oldstate == TCP_ESTABLISHED)
> > -			TCP_INC_STATS(TCP_MIB_ESTABRESETS);
> > -
> > -		sk->sk_prot->unhash(sk);
> > -		if (inet_csk(sk)->icsk_bind_hash &&
> > -		    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
> > -			inet_put_port(&tcp_hashinfo, sk);
> > -		/* fall through */
> > -	default:
> > -		if (oldstate==TCP_ESTABLISHED)
> > -			TCP_DEC_STATS(TCP_MIB_CURRESTAB);
> > -	}
> > -
> > -	/* Change state AFTER socket is unhashed to avoid closed
> > -	 * socket sitting in hash tables.
> > -	 */
> > -	sk->sk_state = state;
> > -
> > -#ifdef STATE_TRACE
> > -	SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n",sk, statename[oldstate],statename[state]);
> > -#endif	
> > -}
> >
> 
> 
> Since the function is called with a constant state, I guess the assumption
> was that gcc would be smart enough to only include the code needed, it looks like
> either code was bigger or the compiler was dumber than expected

I'd guess that compiler was just dumber... :-) It might be an interesting 
experiment to convert it to if's and see if it would make a difference, 
maybe it just gets confused by the switch or something.

Besides, it not always that obvious that gcc is able to determine "the
constant state", considering e.g., the complexity in the cases with
tcp_rcv_synsent_state_process, tcp_close_state, tcp_done. In such cases
uninlining should be done and gcc is probably not able to mix both cases
nicely for a single function?

However, after looking a bit, I'm partially leaning towards the other 
option too:

> >   tcp_done        | -145
> >   tcp_disconnect  | -141

...These called for tcp_set_state just _once_, while this calls for it 
twice:

> >   tcp_fin               |  -86

...Obviously the compiler was able to perform some reductions but 43 bytes 
per inlining seems still a bit high number.


-- 
 i.

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1113 bytes --]

On Sat, 12 Jan 2008, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Sat, 12 Jan 2008 14:59:50 +0200 (EET)
> 
> > ...Maybe I just fall-back to changing my last name, it's the only 
> > full-proof solution... ;-)
> 
> Don't do this!  Otherwise I won't have a frequent test case to make
> sure my patch applying scripts are working properly. :-))))

So which test case you prefer? :-) Is iso-8859-1 from+content ok? Or 
should I keep trying to live with mixed utf-8 which I didn't got even 
fully working last time because git-send-email is probably either too dumb 
or too intelligent (I'm not even sure which), but you were able correct it 
by your tools so the flawed signed-off never entered to the git logs as 
incorrectly formatted :-).

I'd prefer sending them as iso-8859-1 compliant (and I guess you are able 
to test your fix-to-utf-8 machinery with it as well :-)), as it would also 
make my mails compatible with other people's git apply tools you're not 
using (otherwise I'd probably forget to change it occassionally when 
interacting with others than you).


-- 
 i.

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 14 Jan 2008 09:43:08 +0200 (EET)

> So which test case you prefer? :-) Is iso-8859-1 from+content ok? Or 
> should I keep trying to live with mixed utf-8 which I didn't got even 
> fully working last time because git-send-email is probably either too dumb 
> or too intelligent (I'm not even sure which), but you were able correct it 
> by your tools so the flawed signed-off never entered to the git logs as 
> incorrectly formatted :-).
> 
> I'd prefer sending them as iso-8859-1 compliant (and I guess you are able 
> to test your fix-to-utf-8 machinery with it as well :-)), as it would also 
> make my mails compatible with other people's git apply tools you're not 
> using (otherwise I'd probably forget to change it occassionally when 
> interacting with others than you).

For now either way is fine with me.  If the situation changes I'll
let you know.

I'm surprised git-send-email can't get it purely utf8 correctly.

I wonder if there is some issue with how it gets your name
string for the commit author etc.

I wonder if getting it into your global GIT config file in proper
UTF8 encoding would fix things.

Put something like this into ~/.gitconfig

--------------------
[user]
	name = Ilpo Järvinen
	email = ilpo.jarvinen@helsinki.fi
--------------------

The GIT maintainer is Finnish which makes this situation even
more perplexing to me, you might want to discuss it with him :-)

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3687 bytes --]

On Sun, 13 Jan 2008, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 14 Jan 2008 09:43:08 +0200 (EET)
> 
> > I'd prefer sending them as iso-8859-1 compliant (and I guess you are able 
> > to test your fix-to-utf-8 machinery with it as well :-)), as it would also 
> > make my mails compatible with other people's git apply tools you're not 
> > using (otherwise I'd probably forget to change it occassionally when 
> > interacting with others than you).
> 
> For now either way is fine with me.  If the situation changes I'll
> let you know.

Ok, I'll remain in iso-8859-1, it's something that is known to work from 
my end. Thanks anyway for fixing it, wasn't any big deal for me at any 
point of time but people started asking me privately to correct it which 
I of course couldn't... :-)

> I'm surprised git-send-email can't get it purely utf8 correctly.

The problem is that my system is ISO natively, so git-send-email might 
encode a ISO native .patch file's content while sending, which in this 
case was intentionally already utf-8.

It might surprise you but it wasn't a long time ago when git-send-email 
wouldn't care less e.g. about header encoding and I got rejects from 
netdev due to my name which wasn't encoded properly, I've 1.5.0.6 
currently and it seemed still fail to encode Cc addresses it adds from 
signed-offs unless I explicitly ask for it to not do that (I explicitly 
ask for especific, encoded, from header anyway because it was broken at 
some point of time and my sending template is copy-paste originating
from that time). There was some recent fixes in the git's logs regarding 
that encoding, so I intend to check if a later g-s-e is more able and if 
it isn't I'll report it to git folks.

> I wonder if there is some issue with how it gets your name
> string for the commit author etc.

I've had it working well since the encoding header got relatively recently 
added (wasn't available at early dawn of git era), before that it was just 
a mess locally. Funny enough, you were able to magically mangle my emails 
to utf-8'ed commits nicely back then so I got a "fixed" commit back after 
an RTT :-).

> I wonder if getting it into your global GIT config file in proper
> UTF8 encoding would fix things.
>
> Put something like this into ~/.gitconfig
> 
> --------------------
> [user]
> 	name = Ilpo Järvinen
> 	email = ilpo.jarvinen@helsinki.fi
> --------------------

I have this. In addition I have this (required to make my local system 
consistent):

[i18n]
        commitencoding = ISO-8859-1

The problem was just that the API (or better, ABI) between us 
wasn't properly working :-)). While Herbert was working as the
replacement-Dave in November, correct commit entries were created,
so git has been working fine (I guess he used git applying tools
instead of handmade scripts and they handle email correclt based
on it's encoding).

I tried logOutputEncoding = utf-8 in the last patch sets I sent (now 
could again remove it) but git-send-email problem appeared with it
because the system is ISO natively.

> The GIT maintainer is Finnish which makes this situation even
> more perplexing to me, you might want to discuss it with him :-)

Junio? Never heard that a Finnish name... ;-) Perhaps
git-send-email wasn't written by that Finnish guy... :-) 
...Besides, that Finnish git aintainer doesn't have any funny
characters in his name... ;-)

Thanks anyway for the tips & all, I think we have it now working
and I can return to inlines and rexmit_skb_hint things & other TCP
stuff rather than this hinderance. I've some interesting results
from net header inlines checks I ran overnight :-).

-- 
 i.

Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 14 Jan 2008 10:33:34 +0200 (EET)

> Thanks anyway for the tips & all, I think we have it now working
> and I can return to inlines and rexmit_skb_hint things & other TCP
> stuff rather than this hinderance. I've some interesting results
> from net header inlines checks I ran overnight :-).

Great, I look forward to seeing it :)

Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state

[Andi Kleen]

"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> writes:
>
> Besides, it not always that obvious that gcc is able to determine "the
> constant state", considering e.g., the complexity in the cases with
> tcp_rcv_synsent_state_process, tcp_close_state, tcp_done. In such cases
> uninlining should be done and gcc is probably not able to mix both cases
> nicely for a single function?

I think it would be cleanest to completely unswitch the function 
and split into tcp_set_closed()  / tcp_set_established() / tcp_other_state() 
called by the callers directly.

That would probably lose the state trace, but I never found 
that useful for anything.

-Andi