* [PATCH net-2.6.25 0/8] [NET]: More uninlining & related
@ 2008-01-12 9:34 Ilpo Järvinen
2008-01-12 11:16 ` David Miller
[not found] ` <12001304691978-git-send-email-ilpo.jarvinen@helsinki.fi>
0 siblings, 2 replies; 17+ messages in thread
From: Ilpo Järvinen @ 2008-01-12 9:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Hi Dave,
First of all, I changed output encoding of git to utf-8, so I
guess the encoding should not cause the same 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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-2.6.25 0/8] [NET]: More uninlining & related 2008-01-12 9:34 [PATCH net-2.6.25 0/8] [NET]: More uninlining & related Ilpo Järvinen @ 2008-01-12 11:16 ` David Miller [not found] ` <12001304691978-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:16 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:21 +0200 > First of all, I changed output encoding of git to utf-8, so I > guess the encoding should not cause the same trouble for you. I worked on a rebase of net-2.6.25 today and was able to use emacs to fixup the encoding for all existing net-2.6.25 commits. I've also put some things in place to avoid this problem in the future. Thanks. > 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? Unless there is some pressing reason to keep the code around it should be removed. Anyways, I'll take a look. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304691978-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state [not found] ` <12001304691978-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:18 ` David Miller [not found] ` <1200130469783-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:18 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <1200130469783-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 2/8] [TCP]: Uninline tcp_is_cwnd_limited [not found] ` <1200130469783-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:19 ` David Miller [not found] ` <12001304693917-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:19 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:23 +0200 > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> Applied. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304693917-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 3/8] [XFRM] xfrm_policy: kill some bloat [not found] ` <12001304693917-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:20 ` David Miller [not found] ` <12001304703362-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:20 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:24 +0200 > 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> Applied. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304703362-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 4/8] [IPV6] route: kill some bloat [not found] ` <12001304703362-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:21 ` David Miller [not found] ` <12001304703302-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:21 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:25 +0200 > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> Applied. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304703302-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 5/8] [NETLINK] af_netlink: kill some bloat [not found] ` <12001304703302-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:22 ` David Miller [not found] ` <12001304703789-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:22 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:26 +0200 > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> Applied. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304703789-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 6/8] [NETFILTER] xt_policy.c: kill some bloat [not found] ` <12001304703789-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:23 ` David Miller 2008-01-12 19:03 ` Patrick McHardy [not found] ` <12001304701053-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 1 reply; 17+ messages in thread From: David Miller @ 2008-01-12 11:23 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev, kaber, netfilter-devel From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:27 +0200 Ilpo, please post netfilter patches to netfilter-devel, CC:'ing Patrick McHardy. Patrick, please review, thanks. > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 6/8] [NETFILTER] xt_policy.c: kill some bloat 2008-01-12 11:23 ` [PATCH 6/8] [NETFILTER] xt_policy.c: " David Miller @ 2008-01-12 19:03 ` Patrick McHardy 2008-01-13 5:26 ` David Miller 0 siblings, 1 reply; 17+ messages in thread From: Patrick McHardy @ 2008-01-12 19:03 UTC (permalink / raw) To: David Miller; +Cc: ilpo.jarvinen, netdev, netfilter-devel David Miller wrote: > From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> > Date: Sat, 12 Jan 2008 11:34:27 +0200 > > Ilpo, please post netfilter patches to netfilter-devel, > CC:'ing Patrick McHardy. > > Patrick, please review, thanks. This looks fine to me, thanks Ilpo. >> 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() That would probably end up rather ugly because they walk over different structures and have some minor differences. - To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 6/8] [NETFILTER] xt_policy.c: kill some bloat 2008-01-12 19:03 ` Patrick McHardy @ 2008-01-13 5:26 ` David Miller 0 siblings, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-13 5:26 UTC (permalink / raw) To: kaber; +Cc: ilpo.jarvinen, netdev, netfilter-devel From: Patrick McHardy <kaber@trash.net> Date: Sat, 12 Jan 2008 20:03:31 +0100 > David Miller wrote: > > From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> > > Date: Sat, 12 Jan 2008 11:34:27 +0200 > > > > Ilpo, please post netfilter patches to netfilter-devel, > > CC:'ing Patrick McHardy. > > > > Patrick, please review, thanks. > > This looks fine to me, thanks Ilpo. Applied, thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304701053-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [PATCH 7/8] [PKTGEN]: Kill dead static inlines [not found] ` <12001304701053-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:24 ` David Miller [not found] ` <12001304701901-git-send-email-ilpo.jarvinen@helsinki.fi> 1 sibling, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:24 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:28 +0200 > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> These were leftovers from the ktime_t conversion of pktgen. Applied, th anks! ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <12001304701901-git-send-email-ilpo.jarvinen@helsinki.fi>]
* Re: [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs [not found] ` <12001304701901-git-send-email-ilpo.jarvinen@helsinki.fi> @ 2008-01-12 11:25 ` David Miller 0 siblings, 0 replies; 17+ messages in thread From: David Miller @ 2008-01-12 11:25 UTC (permalink / raw) To: ilpo.jarvinen; +Cc: netdev From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 12 Jan 2008 11:34:29 +0200 > 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> I think this is the right thing to do, applied. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-2.6.25 0/8] [NET]: More uninlining & related @ 2008-01-12 9:40 Ilpo Järvinen 2008-01-12 9:40 ` [PATCH 1/8] [TCP]: Uninline tcp_set_state Ilpo Järvinen 0 siblings, 1 reply; 17+ messages in thread From: Ilpo Järvinen @ 2008-01-12 9:40 UTC (permalink / raw) To: David Miller; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/8] [TCP]: Uninline tcp_set_state 2008-01-12 9:40 [PATCH net-2.6.25 0/8] [NET]: More uninlining & related Ilpo Järvinen @ 2008-01-12 9:40 ` Ilpo Järvinen 2008-01-12 21:03 ` Stephen Hemminger 0 siblings, 1 reply; 17+ messages in thread From: Ilpo Järvinen @ 2008-01-12 9:40 UTC (permalink / raw) To: David Miller; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state 2008-01-12 9:40 ` [PATCH 1/8] [TCP]: Uninline tcp_set_state Ilpo Järvinen @ 2008-01-12 21:03 ` Stephen Hemminger 2008-01-12 21:27 ` Arnaldo Carvalho de Melo 2008-01-14 7:20 ` Ilpo Järvinen 0 siblings, 2 replies; 17+ messages in thread From: Stephen Hemminger @ 2008-01-12 21:03 UTC (permalink / raw) To: netdev 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state 2008-01-12 21:03 ` Stephen Hemminger @ 2008-01-12 21:27 ` Arnaldo Carvalho de Melo 2008-01-14 7:20 ` Ilpo Järvinen 1 sibling, 0 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2008-01-12 21:27 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state 2008-01-12 21:03 ` Stephen Hemminger 2008-01-12 21:27 ` Arnaldo Carvalho de Melo @ 2008-01-14 7:20 ` Ilpo Järvinen 2008-01-17 12:41 ` Andi Kleen 1 sibling, 1 reply; 17+ messages in thread From: Ilpo Järvinen @ 2008-01-14 7:20 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Netdev [-- 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/8] [TCP]: Uninline tcp_set_state 2008-01-14 7:20 ` Ilpo Järvinen @ 2008-01-17 12:41 ` Andi Kleen 0 siblings, 0 replies; 17+ messages in thread From: Andi Kleen @ 2008-01-17 12:41 UTC (permalink / raw) To: Ilpo Järvinen; +Cc: Stephen Hemminger, Netdev "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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2008-01-17 12:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-12 9:34 [PATCH net-2.6.25 0/8] [NET]: More uninlining & related Ilpo Järvinen
2008-01-12 11:16 ` David Miller
[not found] ` <12001304691978-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:18 ` [PATCH 1/8] [TCP]: Uninline tcp_set_state David Miller
[not found] ` <1200130469783-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:19 ` [PATCH 2/8] [TCP]: Uninline tcp_is_cwnd_limited David Miller
[not found] ` <12001304693917-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:20 ` [PATCH 3/8] [XFRM] xfrm_policy: kill some bloat David Miller
[not found] ` <12001304703362-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:21 ` [PATCH 4/8] [IPV6] route: " David Miller
[not found] ` <12001304703302-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:22 ` [PATCH 5/8] [NETLINK] af_netlink: " David Miller
[not found] ` <12001304703789-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:23 ` [PATCH 6/8] [NETFILTER] xt_policy.c: " David Miller
2008-01-12 19:03 ` Patrick McHardy
2008-01-13 5:26 ` David Miller
[not found] ` <12001304701053-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:24 ` [PATCH 7/8] [PKTGEN]: Kill dead static inlines David Miller
[not found] ` <12001304701901-git-send-email-ilpo.jarvinen@helsinki.fi>
2008-01-12 11:25 ` [RFC PATCH 8/8] [PKTGEN]: uninline getCurUs David Miller
-- strict thread matches above, loose matches on Subject: below --
2008-01-12 9:40 [PATCH net-2.6.25 0/8] [NET]: More uninlining & related Ilpo Järvinen
2008-01-12 9:40 ` [PATCH 1/8] [TCP]: Uninline tcp_set_state Ilpo Järvinen
2008-01-12 21:03 ` Stephen Hemminger
2008-01-12 21:27 ` Arnaldo Carvalho de Melo
2008-01-14 7:20 ` Ilpo Järvinen
2008-01-17 12:41 ` Andi Kleen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).