[PATCH net-2.6.25 0/4] [NET]: Bloat, bloat and more bloat

[PATCH net-2.6.25 0/4] [NET]: Bloat, bloat and more bloat

[Ilpo Järvinen]

Hi Dave,

After Arnaldo got codiff's inline instrumentation bugs fixed
(thanks! :-)), I got my .c-inline-bloat-o-meter to power up
reliably after some tweaking and bug fixing on my behalf...
It shows some very high readings every now and then in the
code under net/.

...Aand... we've a sovereign winner, though it was only fifth
on a kernel wide list (arch/ excluded due to number of
reasons) :-/.

Hall of (unquestionable) fame (measured per inline, top 10 under
net/):
  -4496 ctnetlink_parse_tuple        netfilter/nf_conntrack_netlink.c
  -2165 ctnetlink_dump_tuples        netfilter/nf_conntrack_netlink.c
  -2115 __ip_vs_get_out_rt           ipv4/ipvs/ip_vs_xmit.c
  -1924 xfrm_audit_helper_pktinfo    xfrm/xfrm_state.c
  -1799 ctnetlink_parse_tuple_proto  netfilter/nf_conntrack_netlink.c
  -1268 ctnetlink_parse_tuple_ip     netfilter/nf_conntrack_netlink.c
  -1093 ctnetlink_exp_dump_expect    netfilter/nf_conntrack_netlink.c
  -1060 ccid3_update_send_interval   dccp/ccids/ccid3.c
  -983  ctnetlink_dump_tuples_proto  netfilter/nf_conntrack_netlink.c
  -827  ctnetlink_exp_dump_tuple     netfilter/nf_conntrack_netlink.c

Removing inlines is done iteratively because e.g., uninlining
ctnetlink_parse_tuple affected ..._{proto,ip} prices as well.

i386/gcc (GCC) 4.1.2 20070626 (Red Hat 4.1.2-13)/allyesconfig
except CONFIG_FORCED_INLINING. Tried without some CONFIG.*DEBUG
as well and got slightly better numbers for some functions, yet
the number don't differ enough to be that meaningful, ie., if  
there's bloat somewhere, removing DEBUGs won't make it go away.

...After this first aid we're at least below 1k :-).

--
 i.

[PATCH 1/4] [NETFILTER]: Kill some supper dupper bloatry

[Ilpo Järvinen]

From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>

/me awards the bloatiest-of-all-net/-.c-code award to
nf_conntrack_netlink.c, congratulations to all the authors :-/!

Hall of (unquestionable) fame (measured per inline, top 10 under
net/):
  -4496 ctnetlink_parse_tuple        netfilter/nf_conntrack_netlink.c
  -2165 ctnetlink_dump_tuples        netfilter/nf_conntrack_netlink.c
  -2115 __ip_vs_get_out_rt           ipv4/ipvs/ip_vs_xmit.c
  -1924 xfrm_audit_helper_pktinfo    xfrm/xfrm_state.c
  -1799 ctnetlink_parse_tuple_proto  netfilter/nf_conntrack_netlink.c
  -1268 ctnetlink_parse_tuple_ip     netfilter/nf_conntrack_netlink.c
  -1093 ctnetlink_exp_dump_expect    netfilter/nf_conntrack_netlink.c
  -1060 void ccid3_update_send_interval  dccp/ccids/ccid3.c
  -983  ctnetlink_dump_tuples_proto  netfilter/nf_conntrack_netlink.c
  -827  ctnetlink_exp_dump_tuple     netfilter/nf_conntrack_netlink.c

  (i386 / gcc (GCC) 4.1.2 20070626 (Red Hat 4.1.2-13) /
   allyesconfig except CONFIG_FORCED_INLINING)

...and I left < 200 byte gains as future work item.

After iterative inline removal, I finally have this:

net/netfilter/nf_conntrack_netlink.c:
  ctnetlink_exp_fill_info   | -1104
  ctnetlink_new_expect      | -1572
  ctnetlink_fill_info       | -1303
  ctnetlink_new_conntrack   | -2230
  ctnetlink_get_expect      | -341
  ctnetlink_del_expect      | -352
  ctnetlink_expect_event    | -1110
  ctnetlink_conntrack_event | -1548
  ctnetlink_del_conntrack   | -729
  ctnetlink_get_conntrack   | -728
 10 functions changed, 11017 bytes removed, diff: -11017

net/netfilter/nf_conntrack_netlink.c:
  ctnetlink_parse_tuple     | +419
  dump_nat_seq_adj          | +183
  ctnetlink_dump_counters   | +166
  ctnetlink_dump_tuples     | +261
  ctnetlink_exp_dump_expect | +633
  ctnetlink_change_status   | +460
 6 functions changed, 2122 bytes added, diff: +2122

net/netfilter/nf_conntrack_netlink.o:
 16 functions changed, 2122 bytes added, 11017 bytes removed, diff: -8895

Without a number of CONFIG.*DEBUGs, I got this:
net/netfilter/nf_conntrack_netlink.o:
 16 functions changed, 2122 bytes added, 11029 bytes removed, diff: -8907

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index d93d58d..38141f1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -95,7 +95,7 @@ nla_put_failure:
 	return -1;
 }
 
-static inline int
+static int
 ctnetlink_dump_tuples(struct sk_buff *skb,
 		      const struct nf_conntrack_tuple *tuple)
 {
@@ -205,7 +205,7 @@ nla_put_failure:
 }
 
 #ifdef CONFIG_NF_CT_ACCT
-static inline int
+static int
 ctnetlink_dump_counters(struct sk_buff *skb, const struct nf_conn *ct,
 			enum ip_conntrack_dir dir)
 {
@@ -284,7 +284,7 @@ nla_put_failure:
 }
 
 #ifdef CONFIG_NF_NAT_NEEDED
-static inline int
+static int
 dump_nat_seq_adj(struct sk_buff *skb, const struct nf_nat_seq *natseq, int type)
 {
 	struct nlattr *nest_parms;
@@ -648,7 +648,7 @@ ctnetlink_parse_tuple_proto(struct nlattr *attr,
 	return ret;
 }
 
-static inline int
+static int
 ctnetlink_parse_tuple(struct nlattr *cda[], struct nf_conntrack_tuple *tuple,
 		      enum ctattr_tuple type, u_int8_t l3num)
 {
@@ -888,7 +888,7 @@ out:
 	return err;
 }
 
-static inline int
+static int
 ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[])
 {
 	unsigned long d;
@@ -1349,7 +1349,7 @@ nla_put_failure:
 	return -1;
 }
 
-static inline int
+static int
 ctnetlink_exp_dump_expect(struct sk_buff *skb,
 			  const struct nf_conntrack_expect *exp)
 {
-- 
1.5.0.6

[PATCH 2/4] [IPVS]: Kill some bloat

[Ilpo Järvinen]

From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>

net/ipv4/ipvs/ip_vs_xmit.c:
  ip_vs_icmp_xmit   | -638
  ip_vs_tunnel_xmit | -674
  ip_vs_nat_xmit    | -716
  ip_vs_dr_xmit     | -682
 4 functions changed, 2710 bytes removed, diff: -2710

net/ipv4/ipvs/ip_vs_xmit.c:
  __ip_vs_get_out_rt | +595
 1 function changed, 595 bytes added, diff: +595

net/ipv4/ipvs/ip_vs_xmit.o:
 5 functions changed, 595 bytes added, 2710 bytes removed, diff: -2115

Without some CONFIG.*DEBUGs:

net/ipv4/ipvs/ip_vs_xmit.o:
 5 functions changed, 383 bytes added, 1513 bytes removed, diff: -1130

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

diff --git a/net/ipv4/ipvs/ip_vs_xmit.c b/net/ipv4/ipvs/ip_vs_xmit.c
index 1e96bf8..8436bf8 100644
--- a/net/ipv4/ipvs/ip_vs_xmit.c
+++ b/net/ipv4/ipvs/ip_vs_xmit.c
@@ -59,7 +59,7 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos, u32 cookie)
 	return dst;
 }
 
-static inline struct rtable *
+static struct rtable *
 __ip_vs_get_out_rt(struct ip_vs_conn *cp, u32 rtos)
 {
 	struct rtable *rt;			/* Route to the other host */
-- 
1.5.0.6

[PATCH 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>

net/xfrm/xfrm_state.c:
  xfrm_audit_state_delete          | -589
  xfrm_replay_check                | -542
  xfrm_audit_state_icvfail         | -520
  xfrm_audit_state_add             | -589
  xfrm_audit_state_replay_overflow | -523
  xfrm_audit_state_notfound_simple | -509
  xfrm_audit_state_notfound        | -521
 7 functions changed, 3793 bytes removed, diff: -3793

net/xfrm/xfrm_state.c:
  xfrm_audit_helper_pktinfo | +522
  xfrm_audit_helper_sainfo  | +598
 2 functions changed, 1120 bytes added, diff: +1120

net/xfrm/xfrm_state.o:
 9 functions changed, 1120 bytes added, 3793 bytes removed, diff: -2673

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

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 4dec655..81b0fce 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2008,8 +2008,8 @@ void __init xfrm_state_init(void)
 }
 
 #ifdef CONFIG_AUDITSYSCALL
-static inline void xfrm_audit_helper_sainfo(struct xfrm_state *x,
-					    struct audit_buffer *audit_buf)
+static void xfrm_audit_helper_sainfo(struct xfrm_state *x,
+				     struct audit_buffer *audit_buf)
 {
 	struct xfrm_sec_ctx *ctx = x->security;
 	u32 spi = ntohl(x->id.spi);
@@ -2036,8 +2036,8 @@ static inline void xfrm_audit_helper_sainfo(struct xfrm_state *x,
 	audit_log_format(audit_buf, " spi=%u(0x%x)", spi, spi);
 }
 
-static inline void xfrm_audit_helper_pktinfo(struct sk_buff *skb, u16 family,
-					     struct audit_buffer *audit_buf)
+static void xfrm_audit_helper_pktinfo(struct sk_buff *skb, u16 family,
+				      struct audit_buffer *audit_buf)
 {
 	struct iphdr *iph4;
 	struct ipv6hdr *iph6;
-- 
1.5.0.6

[PATCH 4/4] [CCID3]: Kill some bloat

[Ilpo Järvinen]

From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>

Without a number of CONFIG.*DEBUG:

net/dccp/ccids/ccid3.c:
  ccid3_hc_tx_update_x          | -170
  ccid3_hc_tx_packet_sent       | -175
  ccid3_hc_tx_packet_recv       | -169
  ccid3_hc_tx_no_feedback_timer | -192
  ccid3_hc_tx_send_packet       | -144
 5 functions changed, 850 bytes removed, diff: -850

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

net/dccp/ccids/ccid3.o:
 6 functions changed, 191 bytes added, 850 bytes removed, diff: -659

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

diff --git a/net/dccp/ccids/ccid3.c b/net/dccp/ccids/ccid3.c
index 00b5f11..8a817dd 100644
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -97,7 +97,7 @@ static inline u64 rfc3390_initial_rate(struct sock *sk)
 /*
  * Recalculate t_ipi and delta (should be called whenever X changes)
  */
-static inline void ccid3_update_send_interval(struct ccid3_hc_tx_sock *hctx)
+static void ccid3_update_send_interval(struct ccid3_hc_tx_sock *hctx)
 {
 	/* Calculate new t_ipi = s / X_inst (X_inst is in 64 * bytes/second) */
 	hctx->ccid3hctx_t_ipi = scaled_div32(((u64)hctx->ccid3hctx_s) << 6,
-- 
1.5.0.6

Re: [PATCH 4/4] [CCID3]: Kill some bloat

[Arnaldo Carvalho de Melo]

Em Sat, Jan 05, 2008 at 03:39:08PM +0200, Ilpo Järvinen escreveu:
> From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>
> 
> Without a number of CONFIG.*DEBUG:
> 
> net/dccp/ccids/ccid3.c:
>   ccid3_hc_tx_update_x          | -170
>   ccid3_hc_tx_packet_sent       | -175
>   ccid3_hc_tx_packet_recv       | -169
>   ccid3_hc_tx_no_feedback_timer | -192
>   ccid3_hc_tx_send_packet       | -144
>  5 functions changed, 850 bytes removed, diff: -850
> 
> net/dccp/ccids/ccid3.c:
>   ccid3_update_send_interval | +191
>  1 function changed, 191 bytes added, diff: +191
> 
> net/dccp/ccids/ccid3.o:
>  6 functions changed, 191 bytes added, 850 bytes removed, diff: -659
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Re: [PATCH net-2.6.25 0/4] [NET]: Bloat, bloat and more bloat

[Arnaldo Carvalho de Melo]

Em Sat, Jan 05, 2008 at 03:39:04PM +0200, Ilpo Järvinen escreveu:
> Hi Dave,
> 
> After Arnaldo got codiff's inline instrumentation bugs fixed
> (thanks! :-)), I got my .c-inline-bloat-o-meter to power up
> reliably after some tweaking and bug fixing on my behalf...
> It shows some very high readings every now and then in the
> code under net/.

Thank you for the reports and for showing how these tools can be put to
good use! 

If you have any further suggestions on how to make codiff and the
dwarves to be of more help or find any other bug, please let me know.

- Arnaldo

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Herbert Xu]

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

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

> #ifdef CONFIG_AUDITSYSCALL
> -static inline void xfrm_audit_helper_sainfo(struct xfrm_state *x,
> -                                           struct audit_buffer *audit_buf)
> +static void xfrm_audit_helper_sainfo(struct xfrm_state *x,
> +                                    struct audit_buffer *audit_buf)

We should never use inline except when it's on the fast path and this
is definitely not a fast path.  If a function ends up being called
just once the compiler will most likely inline it anyway, making the
use of the keyword inline redundant.

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 3/4] [XFRM]: Kill some bloat

[Paul Moore]

On Saturday 05 January 2008 7:29:35 pm Herbert Xu wrote:
> Ilpo J??rvinen <ilpo.jarvinen@helsinki.fi> wrote:
> > Signed-off-by: Ilpo J??rvinen <ilpo.jarvinen@helsinki.fi>
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> > #ifdef CONFIG_AUDITSYSCALL
> > -static inline void xfrm_audit_helper_sainfo(struct xfrm_state *x,
> > -                                           struct audit_buffer
> > *audit_buf) +static void xfrm_audit_helper_sainfo(struct xfrm_state *x,
> > +                                    struct audit_buffer *audit_buf)
>
> We should never use inline except when it's on the fast path and this
> is definitely not a fast path.  If a function ends up being called
> just once the compiler will most likely inline it anyway, making the
> use of the keyword inline redundant.

For the record the inline was there before the audit patch I submitted ... 
then again, I suppose I could have removed the inline while I was at it, I 
just didn't notice it.  Sorry about that.

Thanks for fixing this guys.

-- 
paul moore
linux security @ hp

Re: [PATCH 1/4] [NETFILTER]: Kill some supper dupper bloatry

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat,  5 Jan 2008 15:39:05 +0200

>  10 functions changed, 11017 bytes removed, diff: -11017
 ...
>  6 functions changed, 2122 bytes added, diff: +2122

Yikes, applied :-)

Thanks Ilpo.

Re: [PATCH 2/4] [IPVS]: Kill some bloat

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat,  5 Jan 2008 15:39:06 +0200

> net/ipv4/ipvs/ip_vs_xmit.c:
>   ip_vs_icmp_xmit   | -638
>   ip_vs_tunnel_xmit | -674
>   ip_vs_nat_xmit    | -716
>   ip_vs_dr_xmit     | -682
>  4 functions changed, 2710 bytes removed, diff: -2710
> 
> net/ipv4/ipvs/ip_vs_xmit.c:
>   __ip_vs_get_out_rt | +595
>  1 function changed, 595 bytes added, diff: +595
> 
> net/ipv4/ipvs/ip_vs_xmit.o:
>  5 functions changed, 595 bytes added, 2710 bytes removed, diff: -2115
> 
> Without some CONFIG.*DEBUGs:
> 
> net/ipv4/ipvs/ip_vs_xmit.o:
>  5 functions changed, 383 bytes added, 1513 bytes removed, diff: -1130
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied, thanks.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat,  5 Jan 2008 15:39:07 +0200

> From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>
> 
> net/xfrm/xfrm_state.c:
>   xfrm_audit_state_delete          | -589
>   xfrm_replay_check                | -542
>   xfrm_audit_state_icvfail         | -520
>   xfrm_audit_state_add             | -589
>   xfrm_audit_state_replay_overflow | -523
>   xfrm_audit_state_notfound_simple | -509
>   xfrm_audit_state_notfound        | -521
>  7 functions changed, 3793 bytes removed, diff: -3793
> 
> net/xfrm/xfrm_state.c:
>   xfrm_audit_helper_pktinfo | +522
>   xfrm_audit_helper_sainfo  | +598
>  2 functions changed, 1120 bytes added, diff: +1120
> 
> net/xfrm/xfrm_state.o:
>  9 functions changed, 1120 bytes added, 3793 bytes removed, diff: -2673
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 4/4] [CCID3]: Kill some bloat

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat,  5 Jan 2008 15:39:08 +0200

> From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@helsinki.fi>
> 
> Without a number of CONFIG.*DEBUG:
> 
> net/dccp/ccids/ccid3.c:
>   ccid3_hc_tx_update_x          | -170
>   ccid3_hc_tx_packet_sent       | -175
>   ccid3_hc_tx_packet_recv       | -169
>   ccid3_hc_tx_no_feedback_timer | -192
>   ccid3_hc_tx_send_packet       | -144
>  5 functions changed, 850 bytes removed, diff: -850
> 
> net/dccp/ccids/ccid3.c:
>   ccid3_update_send_interval | +191
>  1 function changed, 191 bytes added, diff: +191
> 
> net/dccp/ccids/ccid3.o:
>  6 functions changed, 191 bytes added, 850 bytes removed, diff: -659
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Also applied, thanks.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 06 Jan 2008 11:29:35 +1100

> We should never use inline except when it's on the fast path and this
> is definitely not a fast path.  If a function ends up being called
> just once the compiler will most likely inline it anyway, making the
> use of the keyword inline redundant.

Similarly I question just about any inline usage at all in *.c files
these days.

I even would discourage it's use for fast-path cases as well.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

On Sat, 5 Jan 2008, David Miller wrote:

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sun, 06 Jan 2008 11:29:35 +1100
> 
> > We should never use inline except when it's on the fast path and this
> > is definitely not a fast path.  If a function ends up being called
> > just once the compiler will most likely inline it anyway, making the
> > use of the keyword inline redundant.
> 
> Similarly I question just about any inline usage at all in *.c files
> these days.
> 
> I even would discourage it's use for fast-path cases as well.

There still seems to be good candidates for inline in *.c files, in worst 
case I had +172 due to inline removed and ~60 are on +10-+90 range with 
my gcc, later gccs might do better but I definately would just blindly 
remove them all. Here's the other end of the list:

+35 static inline hci_encrypt_change_evt        net/bluetooth/hci_event.c
+36 static __inline__ tcp_in_window     net/ipv4/tcp_minisocks.c
+38 static inline hci_conn_complete_evt net/bluetooth/hci_event.c
+38 static inline hci_conn_request_evt  net/bluetooth/hci_event.c
+42 static inline gred_wred_mode        net/sched/sch_gred.c
+45 static inline secpath_has_nontransport      net/xfrm/xfrm_policy.c
+52 static inline bool port_match       net/netfilter/xt_tcpudp.c
+67 static inline dn_check_idf  net/decnet/dn_nsp_in.c
+90 static inline __ieee80211_queue_stopped     net/mac80211/tx.c
+172 static inline sctp_chunk_length_valid      net/sctp/sm_statefuns.c


-- 
 i.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Herbert Xu]

On Mon, Jan 07, 2008 at 09:13:22AM +0200, Ilpo Järvinen wrote:
>
> There still seems to be good candidates for inline in *.c files, in worst 
> case I had +172 due to inline removed and ~60 are on +10-+90 range with 
> my gcc, later gccs might do better but I definately would just blindly 
> remove them all. Here's the other end of the list:

Right.  If the inline helps the compiler perform code size optimisations
that it otherwise couldn't then it might be worthwhile.

So when in doubt, check the assembly :)

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 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

On Sun, 6 Jan 2008, Herbert Xu wrote:

> is definitely not a fast path.  If a function ends up being called
> just once the compiler will most likely inline it anyway, making the
> use of the keyword inline redundant.

Unexpected enough, even this logic seems to fail in a way with my gcc, I'm 
yet to study it closer but it seems to me that e.g., uninlining only once 
called tcp_fastretrans_alert is worth of at least 100 bytes (note that 
it's not inlined by us, gcc did it all by itself)! Otherwise I'd fail to 
understand why I got -270 bytes from uninlining tcp_moderate_cwnd which is 
only 57 bytes as unlined with three call sites.

    net/ipv4/tcp_input.c:
      tcp_undo_cwr          |  -48
      tcp_try_undo_recovery |  -55
      tcp_ack               | -2941
     3 functions changed, 3044 bytes removed, diff: -3044
    
    net/ipv4/tcp_input.c:
      tcp_moderate_cwnd     |  +57
      tcp_fastretrans_alert | +2717
     2 functions changed, 2774 bytes added, diff: +2774
    
    net/ipv4/tcp_input.o:
     5 functions changed, 2774 bytes added, 3044 bytes removed, diff: -270

I'll probably force uninlining of it without tcp_moderate_cwnd noise and 
try a number of gcc versions.


-- 
 i.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Herbert Xu]

On Mon, Jan 07, 2008 at 10:21:47AM +0200, Ilpo Järvinen wrote:
>
> Unexpected enough, even this logic seems to fail in a way with my gcc, I'm 
> yet to study it closer but it seems to me that e.g., uninlining only once 
> called tcp_fastretrans_alert is worth of at least 100 bytes (note that 
> it's not inlined by us, gcc did it all by itself)! Otherwise I'd fail to 
> understand why I got -270 bytes from uninlining tcp_moderate_cwnd which is 
> only 57 bytes as unlined with three call sites.

Yeah I've studied this effect over the years in my maintainence of
the dash shell where size is paramount :) 

On x86-32 due to the scarcity of registers local variables often end
up on the stack.  In that case gcc will usually refer to them via %ebp
or %esp.  It just so happens that if the offset is within 128 bytes
of the base register then the instruction can encode the offset with
just a single byte.  However, if it exceeds 128 bytes the instruction
will take 4 bytes to encode the offset.

Since bigger functions are more likely to go over that threshold,
this explains some instances where uninlining ends up being bigger.

There are other similar issues, such as the branch offset which also
goes from 1 byte to 4 when you go over 128 bytes.

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 3/4] [XFRM]: Kill some bloat

[Andi Kleen]

David Miller <davem@davemloft.net> writes:

> Similarly I question just about any inline usage at all in *.c files

Don't forget the .h files. Especially a lot of stuff in tcp.h should
be probably in some .c file and not be inline.

-Andi

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 08 Jan 2008 01:23:11 +0100

> David Miller <davem@davemloft.net> writes:
> 
> > Similarly I question just about any inline usage at all in *.c files
> 
> Don't forget the .h files. Especially a lot of stuff in tcp.h should
> be probably in some .c file and not be inline.

I explicitly left them out.

Most of them are abstractions of common 2 or 3 instruction
calculations, and thus should stay inline.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Andi Kleen]

On Mon, Jan 07, 2008 at 05:54:58PM -0800, David Miller wrote:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Tue, 08 Jan 2008 01:23:11 +0100
> 
> > David Miller <davem@davemloft.net> writes:
> > 
> > > Similarly I question just about any inline usage at all in *.c files
> > 
> > Don't forget the .h files. Especially a lot of stuff in tcp.h should
> > be probably in some .c file and not be inline.
> 
> I explicitly left them out.
> 
> Most of them are abstractions of common 2 or 3 instruction
> calculations, and thus should stay inline.

Definitely not in tcp.h. It has quite a lot of very long functions, of
which very few really need to be inline: (AFAIK the only one where 
it makes really sense is tcp_set_state due to constant evaluation; 
although I never quite understood why the callers just didn't 
call explicit functions to do these actions) 

% awk '  { line++ } ; /^{/ { start = line } ; /^}/ { n++; r += line-start-2; } ; END { print r/n }' < include/net/tcp.h 
9.48889

The average function length is 9 lines.

-Andi

Re: [PATCH 3/4] [XFRM]: Kill some bloat II

[Andi Kleen]

> % awk '  { line++ } ; /^{/ { start = line } ; /^}/ { n++; r += line-start-2; } ; END { print r/n }' < include/net/tcp.h 
> 9.48889
> 
> The average function length is 9 lines.

Actually 8 -- the awk hack had a off by one. Still too long.

-Andi

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 8 Jan 2008 03:05:29 +0100

> On Mon, Jan 07, 2008 at 05:54:58PM -0800, David Miller wrote:
> > I explicitly left them out.
> > 
> > Most of them are abstractions of common 2 or 3 instruction
> > calculations, and thus should stay inline.
> 
> Definitely not in tcp.h. It has quite a lot of very long functions, of
> which very few really need to be inline: (AFAIK the only one where 
> it makes really sense is tcp_set_state due to constant evaluation; 
> although I never quite understood why the callers just didn't 
> call explicit functions to do these actions) 
> 
> % awk '  { line++ } ; /^{/ { start = line } ; /^}/ { n++; r += line-start-2; } ; END { print r/n }' < include/net/tcp.h 
> 9.48889
> 
> The average function length is 9 lines.

The vast majority of them are one, two, and three liners.

There are about 4 or 5 inlines in there are in fact large and perhaps
should be removed, and these puff up your average.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Andi Kleen]

On Mon, Jan 07, 2008 at 07:37:00PM -0800, David Miller wrote:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Tue, 8 Jan 2008 03:05:29 +0100
> 
> > On Mon, Jan 07, 2008 at 05:54:58PM -0800, David Miller wrote:
> > > I explicitly left them out.
> > > 
> > > Most of them are abstractions of common 2 or 3 instruction
> > > calculations, and thus should stay inline.
> > 
> > Definitely not in tcp.h. It has quite a lot of very long functions, of
> > which very few really need to be inline: (AFAIK the only one where 
> > it makes really sense is tcp_set_state due to constant evaluation; 
> > although I never quite understood why the callers just didn't 
> > call explicit functions to do these actions) 
> > 
> > % awk '  { line++ } ; /^{/ { start = line } ; /^}/ { n++; r += line-start-2; } ; END { print r/n }' < include/net/tcp.h 
> > 9.48889
> > 
> > The average function length is 9 lines.
> 
> The vast majority of them are one, two, and three liners.

% awk '  { line++ } ; /^{/ { total++; start = line } ; /^}/ { len=line-start-3; if (len > 4) l++; if (len >= 10) k++; } ; END { print total, l, l/total, k, k/total }' < include/net/tcp.h
68 28 0.411765 20 0.294118

41% are over 4 lines, 29% are >= 10 lines.

-Andi

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 8 Jan 2008 06:00:07 +0100

> On Mon, Jan 07, 2008 at 07:37:00PM -0800, David Miller wrote:
> > The vast majority of them are one, two, and three liners.
> 
> % awk '  { line++ } ; /^{/ { total++; start = line } ; /^}/ { len=line-start-3; if (len > 4) l++; if (len >= 10) k++; } ; END { print total, l, l/total, k, k/total }' < include/net/tcp.h
> 68 28 0.411765 20 0.294118
> 
> 41% are over 4 lines, 29% are >= 10 lines.

Take out the comments and whitespace lines, your script is
too simplistic.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

On Mon, 7 Jan 2008, David Miller wrote:

> From: Andi Kleen <andi@firstfloor.org>
> Date: Tue, 8 Jan 2008 06:00:07 +0100
> 
> > On Mon, Jan 07, 2008 at 07:37:00PM -0800, David Miller wrote:
> > > The vast majority of them are one, two, and three liners.
> > 
> > % awk '  { line++ } ; /^{/ { total++; start = line } ; /^}/ { len=line-start-3; if (len > 4) l++; if (len >= 10) k++; } ; END { print total, l, l/total, k, k/total }' < include/net/tcp.h
> > 68 28 0.411765 20 0.294118
> > 
> > 41% are over 4 lines, 29% are >= 10 lines.
> 
> Take out the comments and whitespace lines, your script is
> too simplistic.

He should also remove these, which involve just syntactic casting to 
please the compiler:

	struct tcp_sock *tp = tcp_sk(sk);
	struct inet_connection_sock *icsk = inet_csk(sk);

...and maybe some other similar ones. I'm sure that's going to make his 
statistics even more questionable as is because we need tp all around.

But about his concerns, I spend couple of minutes in looking to it by 
hand. These are likely to be a win:
	tcp_set_state
	tcp_prequeue 
	tcp_prequeue_init
	tcp_openreq_init

...about rest I'm very unsure but there are some that probably are a 
minor win as well.

tcp_dec_quickack_mode has only a single caller anyway so I'd doubt that 
it's going to be significant, I thought moving it to .c earlier but 
haven't just done that yet :-).


-- 
 i.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

On Tue, 8 Jan 2008, Andi Kleen wrote:

> David Miller <davem@davemloft.net> writes:
> 
> > Similarly I question just about any inline usage at all in *.c files
> 
> Don't forget the .h files. Especially a lot of stuff in tcp.h should
> be probably in some .c file and not be inline.

I'm not forgetting them... :-)

My intention is to do the same for all headers as well but I'd really like 
to get some actual number of bytes by measuring rather than taking them of 
the hat.

...It just requires some work still to get the uninlining actually do the 
right thing and actually testing it involves orders of magnitude more 
exhaustive recompilation than .o only checking required so after finishing 
the script I either has to setup n distcc master with a number of slaves 
each or wait for some time for the results :-).

However, I suspect that anything in tcp.h won't match to something like 
~5-10k like I've seen in three .c files already + one with debugging that 
has even more than 10k (even if all in tcp.h summed up :-)).

-- 
 i.

Re: [PATCH net-2.6.25 0/4] [NET]: Bloat, bloat and more bloat

[Ilpo Järvinen]

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

On Sat, 5 Jan 2008, Arnaldo Carvalho de Melo wrote:

> Em Sat, Jan 05, 2008 at 03:39:04PM +0200, Ilpo Järvinen escreveu:
> > Hi Dave,
> > 
> > After Arnaldo got codiff's inline instrumentation bugs fixed
> > (thanks! :-)), I got my .c-inline-bloat-o-meter to power up
> > reliably after some tweaking and bug fixing on my behalf...
> > It shows some very high readings every now and then in the
> > code under net/.
> 
> Thank you for the reports and for showing how these tools can be put to
> good use! 
> 
> If you have any further suggestions on how to make codiff and the
> dwarves to be of more help or find any other bug, please let me know.

It could use a bit less memory because my header inline checking attempt 
on a machine with 2G+2G ends up like this:

$ codiff vmlinux.o.base vmlinux.o
libclasses: out of memory(inline_expansion__new)
$ ls -al vmlinux.o{,.base}
-rw-r----- 1 ijjarvin tkol 633132586 Jan  9 13:11 vmlinux.o
-rw-r----- 1 ijjarvin tkol 633132572 Jan  9 00:58 vmlinux.o.base
$

Considering that's only 0.6G+0.6G I've a problem in understanding why 
codiff's number crunching eats up so much memory.

I just hope there isn't any O(n^2) or worse algos in it either once
the memory consumption gets resolved. :-)


-- 
 i.

Re: [PATCH 3/4] [XFRM]: Kill some bloat

[Ilpo Järvinen]

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

On Tue, 8 Jan 2008, Ilpo Järvinen wrote:

> On Mon, 7 Jan 2008, David Miller wrote:
> 
> > From: Andi Kleen <andi@firstfloor.org>
> > Date: Tue, 8 Jan 2008 06:00:07 +0100
> > 
> > > On Mon, Jan 07, 2008 at 07:37:00PM -0800, David Miller wrote:
> > > > The vast majority of them are one, two, and three liners.
> > > 
> > > % awk '  { line++ } ; /^{/ { total++; start = line } ; /^}/ { len=line-start-3; if (len > 4) l++; if (len >= 10) k++; } ; END { print total, l, l/total, k, k/total }' < include/net/tcp.h
> > > 68 28 0.411765 20 0.294118
> > > 
> > > 41% are over 4 lines, 29% are >= 10 lines.
> > 
> > Take out the comments and whitespace lines, your script is
> > too simplistic.

In addition it triggered spuriously per struct/enum end brace :-) and
was using the last known function starting brace in there so no wonder
the numbers were that high... Counting with the corrected lines 
(len=line-start-1) & spurious matches removed:

74 19 0.256757 7 0.0945946


Here are (finally) the measured bytes (couple of the functions are 
missing because I had couple of bugs in the regexps and the #if trickery 
at the inline resulted failed compiles):

 12 funcs, 242+, 1697-, diff: -1455 	 tcp_set_state 
 13 funcs, 92+, 632-, diff: -540 	 tcp_is_cwnd_limited 
 12 funcs, 2836+, 3225-, diff: -389 	 tcp_current_ssthresh 
 5 funcs, 261+, 556-, diff: -295 	 tcp_prequeue 
 7 funcs, 2777+, 3049-, diff: -272 	 tcp_clear_retrans_hints_partial 
 11 funcs, 64+, 275-, diff: -211 	 tcp_win_from_space 
 6 funcs, 128+, 320-, diff: -192 	 tcp_prequeue_init 
 12 funcs, 45+, 209-, diff: -164 	 tcp_set_ca_state 
 7 funcs, 106+, 237-, diff: -131 	 tcp_fast_path_check 
 5 funcs, 167+, 291-, diff: -124 	 tcp_write_queue_purge 
 6 funcs, 43+, 160-, diff: -117 	 tcp_push_pending_frames 
 9 funcs, 55+, 159-, diff: -104 	 tcp_v4_check 
 6 funcs, 4+, 97-, diff: -93 	 tcp_packets_in_flight 
 7 funcs, 58+, 150-, diff: -92 	 tcp_fast_path_on 
 4 funcs, 4+, 91-, diff: -87 	 tcp_clear_options 
 6 funcs, 141+, 217-, diff: -76 	 tcp_openreq_init 
 8 funcs, 38+, 111-, diff: -73 	 tcp_unlink_write_queue 
 7 funcs, 32+, 103-, diff: -71 	 tcp_checksum_complete 
 7 funcs, 35+, 101-, diff: -66 	 __tcp_fast_path_on 
 5 funcs, 4+, 66-, diff: -62 	 tcp_receive_window 
 6 funcs, 67+, 128-, diff: -61 	 tcp_add_write_queue_tail 
 7 funcs, 30+, 86-, diff: -56 	 tcp_ca_event 
 6 funcs, 73+, 106-, diff: -33 	 tcp_paws_check 
 4 funcs, 4+, 36-, diff: -32 	 tcp_highest_sack_seq 
 6 funcs, 46+, 78-, diff: -32 	 tcp_fin_time 
 3 funcs, 4+, 35-, diff: -31 	 tcp_clear_all_retrans_hints 
 7 funcs, 30+, 51-, diff: -21 	 __tcp_add_write_queue_tail 
 3 funcs, 4+, 14-, diff: -10 	 tcp_enable_fack 
 4 funcs, 4+, 14-, diff: -10 	 keepalive_time_when 
 8 funcs, 66+, 73-, diff: -7 	 tcp_full_space 
 3 funcs, 4+, 5-, diff: -1 	 tcp_wnd_end 
 4 funcs, 97+, 97-, diff: +0 	 tcp_mib_init 
 3 funcs, 4+, 3-, diff: +1 	 tcp_skb_is_last 
 2 funcs, 4+, 2-, diff: +2 	 keepalive_intvl_when 
 2 funcs, 4+, 2-, diff: +2 	 tcp_is_fack 
 2 funcs, 4+, 2-, diff: +2 	 tcp_skb_mss 
 2 funcs, 4+, 2-, diff: +2 	 tcp_write_queue_empty 
 2 funcs, 4+, 2-, diff: +2 	 tcp_advance_highest_sack 
 2 funcs, 4+, 2-, diff: +2 	 tcp_advance_send_head 
 2 funcs, 4+, 2-, diff: +2 	 tcp_check_send_head 
 2 funcs, 4+, 2-, diff: +2 	 tcp_highest_sack_reset 
 2 funcs, 4+, 2-, diff: +2 	 tcp_init_send_head 
 2 funcs, 4+, 2-, diff: +2 	 tcp_sack_reset 
 6 funcs, 47+, 44-, diff: +3 	 tcp_space 
 5 funcs, 55+, 50-, diff: +5 	 tcp_too_many_orphans 
 3 funcs, 8+, 2-, diff: +6 	 tcp_minshall_update 
 3 funcs, 8+, 2-, diff: +6 	 tcp_update_wl 
 8 funcs, 25+, 14-, diff: +11 	 between 
 3 funcs, 14+, 2-, diff: +12 	 tcp_put_md5sig_pool 
 3 funcs, 14+, 2-, diff: +12 	 tcp_clear_xmit_timers 
 5 funcs, 30+, 17-, diff: +13 	 tcp_dec_pcount_approx_int 
 6 funcs, 33+, 20-, diff: +13 	 tcp_insert_write_queue_after 
 3 funcs, 17+, 2-, diff: +15 	 __tcp_checksum_complete 
 5 funcs, 17+, 2-, diff: +15 	 tcp_init_wl 
 4 funcs, 57+, 41-, diff: +16 	 tcp_dec_quickack_mode 
 4 funcs, 40+, 22-, diff: +18 	 __tcp_add_write_queue_head 
 5 funcs, 36+, 16-, diff: +20 	 tcp_highest_sack_combine 
 4 funcs, 40+, 18-, diff: +22 	 tcp_dec_pcount_approx 
 6 funcs, 29+, 5-, diff: +24 	 tcp_is_sack 
 4 funcs, 28+, 2-, diff: +26 	 tcp_is_reno 
 5 funcs, 50+, 24-, diff: +26 	 tcp_insert_write_queue_before 
 4 funcs, 83+, 56-, diff: +27 	 tcp_check_probe_timer 
 8 funcs, 69+, 14-, diff: +55 	 tcp_left_out 
 11 funcs, 2995+, 2893-, diff: +102 	 tcp_skb_pcount 
 30 funcs, 930+, 2-, diff: +928 	 before 

-- 
 i.