net/ipv4/netfilter/ip_tables.c: remove some inlines

PATCH In review Denys Vlasenko 2 participants 8 messages Dec 16, 2007 – Jan 8, 2008

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Denys Vlasenko

author

2007-12-16 20:47 UTC #0 lore raw

PATCH 1/1 net/ipv4/netfilter/ip_tables.c: remove some inlines

Hi Patrick, Harald,

I was working on unrelated problem and noticed that ip_tables.c seem to abuse inline. I prepared a patch which removes inlines except those which are used by packet matching code (and thus are really performance-critical).
I added comments explaining that remaining inlines are performance critical.

Result as reported by size:

text data bss dec hex filename

-  6451     380      88    6919    1b07 ip_tables.o
+  6339     348      72    6759    1a67 ip_tables.o

Please take this patch into netfilter queue.

Signed-off-by Denys Vlasenko <[email protected]>

--
vda

net/ipv4/netfilter/ip_tables.c +25 ~~-20~~

diff -urpN linux-2.6.org/net/ipv4/netfilter/ip_tables.c linux-2.6.ipt/net/ipv4/netfilter/ip_tables.c
@@ -74,6 +74,7 @@ do {								\
    Hence the start of any table is given by get_table() below.  */
 
 /* Returns whether matches rule or not. */
+/* Performance critical - called for every packet */
 static inline int
 ip_packet_match(const struct iphdr *ip,
 		const char *indev,

@@ -152,7 +153,7 @@ ip_packet_match(const struct iphdr *ip,
 	return 1;
 }
 
-static inline bool
+static bool
 ip_checkentry(const struct ipt_ip *ip)
 {
 	if (ip->flags & ~IPT_F_MASK) {

@@ -182,6 +183,7 @@ ipt_error(struct sk_buff *skb,
 	return NF_DROP;
 }
 
+/* Performance critical - called for every packet */
 static inline
 bool do_match(struct ipt_entry_match *m,
 	      const struct sk_buff *skb,

@@ -198,6 +200,7 @@ bool do_match(struct ipt_entry_match *m,
 		return false;
 }
 
+/* Performance critical */
 static inline struct ipt_entry *
 get_entry(void *base, unsigned int offset)
 {

@@ -205,6 +208,7 @@ get_entry(void *base, unsigned int offse
 }
 
 /* All zeroes == unconditional rule. */
+/* Mildly perf critical (only if packet tracing is on) */
 static inline int
 unconditional(const struct ipt_ip *ip)
 {

@@ -219,7 +223,7 @@ unconditional(const struct ipt_ip *ip)
 
 #if defined(CONFIG_NETFILTER_XT_TARGET_TRACE) || \
     defined(CONFIG_NETFILTER_XT_TARGET_TRACE_MODULE)
-static const char *hooknames[] = {
+static const char *const hooknames[] = {
 	[NF_IP_PRE_ROUTING]		= "PREROUTING",
 	[NF_IP_LOCAL_IN]		= "INPUT",
 	[NF_IP_FORWARD]			= "FORWARD",

@@ -233,7 +237,7 @@ enum nf_ip_trace_comments {
 	NF_IP_TRACE_COMMENT_POLICY,
 };
 
-static const char *comments[] = {
+static const char *const comments[] = {
 	[NF_IP_TRACE_COMMENT_RULE]	= "rule",
 	[NF_IP_TRACE_COMMENT_RETURN]	= "return",
 	[NF_IP_TRACE_COMMENT_POLICY]	= "policy",

@@ -249,6 +253,7 @@ static struct nf_loginfo trace_loginfo =
 	},
 };
 
+/* Mildly perf critical (only if packet tracing is on) */
 static inline int
 get_chainname_rulenum(struct ipt_entry *s, struct ipt_entry *e,
 		      char *hookname, char **chainname,

@@ -567,7 +572,7 @@ mark_source_chains(struct xt_table_info 
 	return 1;
 }
 
-static inline int
+static int
 cleanup_match(struct ipt_entry_match *m, unsigned int *i)
 {
 	if (i && (*i)-- == 0)

@@ -579,7 +584,7 @@ cleanup_match(struct ipt_entry_match *m,
 	return 0;
 }
 
-static inline int
+static int
 check_entry(struct ipt_entry *e, const char *name)
 {
 	struct ipt_entry_target *t;

@@ -599,7 +604,7 @@ check_entry(struct ipt_entry *e, const c
 	return 0;
 }
 
-static inline int check_match(struct ipt_entry_match *m, const char *name,
+static int check_match(struct ipt_entry_match *m, const char *name,
 				const struct ipt_ip *ip, unsigned int hookmask,
 				unsigned int *i)
 {

@@ -622,7 +627,7 @@ static inline int check_match(struct ipt
 	return ret;
 }
 
-static inline int
+static int
 find_check_match(struct ipt_entry_match *m,
 	    const char *name,
 	    const struct ipt_ip *ip,

@@ -651,7 +656,7 @@ err:
 	return ret;
 }
 
-static inline int check_target(struct ipt_entry *e, const char *name)
+static int check_target(struct ipt_entry *e, const char *name)
 {
 	struct ipt_entry_target *t;
 	struct xt_target *target;

@@ -672,7 +677,7 @@ static inline int check_target(struct ip
 	return ret;
 }
 
-static inline int
+static int
 find_check_entry(struct ipt_entry *e, const char *name, unsigned int size,
 	    unsigned int *i)
 {

@@ -716,7 +721,7 @@ find_check_entry(struct ipt_entry *e, co
 	return ret;
 }
 
-static inline int
+static int
 check_entry_size_and_hooks(struct ipt_entry *e,
 			   struct xt_table_info *newinfo,
 			   unsigned char *base,

@@ -759,7 +764,7 @@ check_entry_size_and_hooks(struct ipt_en
 	return 0;
 }
 
-static inline int
+static int
 cleanup_entry(struct ipt_entry *e, unsigned int *i)
 {
 	struct ipt_entry_target *t;

@@ -1293,7 +1298,7 @@ __do_replace(const char *name, unsigned 
 	get_counters(oldinfo, counters);
 	/* Decrease module usage counts and free resource */
 	loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id()];
-	IPT_ENTRY_ITERATE(loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL);
+	IPT_ENTRY_ITERATE(loc_cpu_old_entry, oldinfo->size, cleanup_entry, NULL);
 	xt_free_table_info(oldinfo);
 	if (copy_to_user(counters_ptr, counters,
 			 sizeof(struct xt_counters) * num_counters) != 0)

@@ -1361,7 +1366,7 @@ do_replace(void __user *user, unsigned i
 	return 0;
 
  free_newinfo_untrans:
-	IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry,NULL);
+	IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry, NULL);
  free_newinfo:
 	xt_free_table_info(newinfo);
 	return ret;

@@ -1369,7 +1374,7 @@ do_replace(void __user *user, unsigned i
 
 /* We're lazy, and add to the first CPU; overflow works its fey magic
  * and everything is OK. */
-static inline int
+static int
 add_counter_to_entry(struct ipt_entry *e,
 		     const struct xt_counters addme[],
 		     unsigned int *i)

@@ -1527,7 +1532,7 @@ out:
 	return ret;
 }
 
-static inline int
+static int
 compat_find_calc_match(struct ipt_entry_match *m,
 	    const char *name,
 	    const struct ipt_ip *ip,

@@ -1551,7 +1556,7 @@ compat_find_calc_match(struct ipt_entry_
 	return 0;
 }
 
-static inline int
+static int
 compat_release_match(struct ipt_entry_match *m, unsigned int *i)
 {
 	if (i && (*i)-- == 0)

@@ -1561,7 +1566,7 @@ compat_release_match(struct ipt_entry_ma
 	return 0;
 }
 
-static inline int
+static int
 compat_release_entry(struct ipt_entry *e, unsigned int *i)
 {
 	struct ipt_entry_target *t;

@@ -1576,7 +1581,7 @@ compat_release_entry(struct ipt_entry *e
 	return 0;
 }
 
-static inline int
+static int
 check_compat_entry_size_and_hooks(struct ipt_entry *e,
 			   struct xt_table_info *newinfo,
 			   unsigned int *size,

@@ -1702,7 +1707,7 @@ static int compat_copy_entry_from_user(s
 	return ret;
 }
 
-static inline int compat_check_entry(struct ipt_entry *e, const char *name,
+static int compat_check_entry(struct ipt_entry *e, const char *name,
 						unsigned int *i)
 {
 	int j, ret;

@@ -1895,7 +1900,7 @@ compat_do_replace(void __user *user, uns
 	return 0;
 
  free_newinfo_untrans:
-	IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry,NULL);
+	IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry, NULL);
  free_newinfo:
 	xt_free_table_info(newinfo);
 	return ret;

Patrick McHardy

2007-12-17 14:47 UTC #1 lore raw

Please CC netfilter-devel on netfilter patches.

Quoted 15 lines · Denys Vlasenko

Hi Patrick, Harald,
I was working on unrelated problem and noticed that ip_tables.c seem to abuse inline. I prepared a patch which removes inlines except those which are used by packet matching code (and thus are really performance-critical).
I added comments explaining that remaining inlines are performance critical.
Result as reported by size:
text data bss dec hex filename
(quoted diff) +1 -1
-  6451     380      88    6919    1b07 ip_tables.o
+  6339     348      72    6759    1a67 ip_tables.o

Quoted 1 line

Please take this patch into netfilter queue.

This clashes with my pending patches, which I'll push upstream today. I also spent some time resyncing ip_tables and ip6_tables so a diff of both (with some sed'ing) shows only the actual differences, so please update ip6_tables as well.

Denys Vlasenko

author

2007-12-27 04:01 UTC #2 lore raw

Quoted 18 lines · Patrick McHardy

Please CC netfilter-devel on netfilter patches.
Quoted 15 lines · Denys Vlasenko
Hi Patrick, Harald,
I was working on unrelated problem and noticed that ip_tables.c seem to abuse inline. I prepared a patch which removes inlines except those which are used by packet matching code (and thus are really performance-critical).
I added comments explaining that remaining inlines are performance critical.
Result as reported by size:
text data bss dec hex filename
(quoted diff) +1 -1
-  6451     380      88    6919    1b07 ip_tables.o
+  6339     348      72    6759    1a67 ip_tables.o

Quoted 6 lines

Quoted 1 line
Please take this patch into netfilter queue.
This clashes with my pending patches, which I'll push upstream today. I also spent some time resyncing ip_tables and ip6_tables so a diff of both (with some sed'ing) shows only the actual differences, so please update ip6_tables as well.

I would be happy to rediff the patch.

Which tree should I sync against in order to not collide with your changes?
--
vda

Patrick McHardy

2007-12-28 15:30 UTC #3 lore raw

Quoted 8 lines · Denys Vlasenko

Quoted 5 lines · Patrick McHardy
This clashes with my pending patches, which I'll push upstream today. I also spent some time resyncing ip_tables and ip6_tables so a diff of both (with some sed'ing) shows only the actual differences, so please update ip6_tables as well.

Quoted 5 lines

I would be happy to rediff the patch.
Which tree should I sync against in order to not collide with your changes?

The net-2.6.25.git tree contains all my changes.

Denys Vlasenko

author

2007-12-31 16:43 UTC #4 lore raw

PATCH 1/1 ip[6]_tables.c: remove some inlines

Quoted 11 lines · Patrick McHardy

Quoted 9 lines
Quoted 4 lines
This clashes with my pending patches, which I'll push upstream today. I also spent some time resyncing ip_tables and ip6_tables so a diff of both (with some sed'ing) shows only the actual differences, so please update ip6_tables as well.
I would be happy to rediff the patch.
Which tree should I sync against in order to not collide with your changes?
The net-2.6.25.git tree contains all my changes.

ip[6]_tables.c seem to abuse inline.

This patch removes inlines except those which are used by packet matching code and thus are performance-critical.

Please take this patch into netfilter queue.

Signed-off-by Denys Vlasenko <[email protected]>

--
vda

net/ipv4/netfilter/ip_tables.c +27 ~~-20~~

index f5b66ec..982b7f9 100644
@@ -75,6 +75,7 @@ do {								\
    Hence the start of any table is given by get_table() below.  */
 
 /* Returns whether matches rule or not. */
+/* Performance critical - called for every packet */
 static inline bool
 ip_packet_match(const struct iphdr *ip,
 		const char *indev,

@@ -153,7 +154,7 @@ ip_packet_match(const struct iphdr *ip,
 	return true;
 }
 
-static inline bool
+static bool
 ip_checkentry(const struct ipt_ip *ip)
 {
 	if (ip->flags & ~IPT_F_MASK) {

@@ -183,8 +184,9 @@ ipt_error(struct sk_buff *skb,
 	return NF_DROP;
 }
 
-static inline
-bool do_match(struct ipt_entry_match *m,
+/* Performance critical - called for every packet */
+static inline bool
+do_match(struct ipt_entry_match *m,
 	      const struct sk_buff *skb,
 	      const struct net_device *in,
 	      const struct net_device *out,

@@ -199,6 +201,7 @@ bool do_match(struct ipt_entry_match *m,
 		return false;
 }
 
+/* Performance critical */
 static inline struct ipt_entry *
 get_entry(void *base, unsigned int offset)
 {

@@ -206,6 +209,7 @@ get_entry(void *base, unsigned int offset)
 }
 
 /* All zeroes == unconditional rule. */
+/* Mildly perf critical (only if packet tracing is on) */
 static inline int
 unconditional(const struct ipt_ip *ip)
 {

@@ -221,7 +225,7 @@ unconditional(const struct ipt_ip *ip)
 
 #if defined(CONFIG_NETFILTER_XT_TARGET_TRACE) || \
     defined(CONFIG_NETFILTER_XT_TARGET_TRACE_MODULE)
-static const char *hooknames[] = {
+static const char *const hooknames[] = {
 	[NF_INET_PRE_ROUTING]		= "PREROUTING",
 	[NF_INET_LOCAL_IN]		= "INPUT",
 	[NF_INET_FORWARD]		= "FORWARD",

@@ -235,7 +239,7 @@ enum nf_ip_trace_comments {
 	NF_IP_TRACE_COMMENT_POLICY,
 };
 
-static const char *comments[] = {
+static const char *const comments[] = {
 	[NF_IP_TRACE_COMMENT_RULE]	= "rule",
 	[NF_IP_TRACE_COMMENT_RETURN]	= "return",
 	[NF_IP_TRACE_COMMENT_POLICY]	= "policy",

@@ -251,6 +255,7 @@ static struct nf_loginfo trace_loginfo = {
 	},
 };
 
+/* Mildly perf critical (only if packet tracing is on) */
 static inline int
 get_chainname_rulenum(struct ipt_entry *s, struct ipt_entry *e,
 		      char *hookname, char **chainname,

@@ -567,7 +572,7 @@ mark_source_chains(struct xt_table_info *newinfo,
 	return 1;
 }
 
-static inline int
+static int
 cleanup_match(struct ipt_entry_match *m, unsigned int *i)
 {
 	if (i && (*i)-- == 0)

@@ -579,7 +584,7 @@ cleanup_match(struct ipt_entry_match *m, unsigned int *i)
 	return 0;
 }
 
-static inline int
+static int
 check_entry(struct ipt_entry *e, const char *name)
 {
 	struct ipt_entry_target *t;

@@ -600,7 +605,8 @@ check_entry(struct ipt_entry *e, const char *name)
 	return 0;
 }
 
-static inline int check_match(struct ipt_entry_match *m, const char *name,
+static int
+check_match(struct ipt_entry_match *m, const char *name,
 			      const struct ipt_ip *ip,
 			      unsigned int hookmask, unsigned int *i)
 {

@@ -623,7 +629,7 @@ static inline int check_match(struct ipt_entry_match *m, const char *name,
 	return ret;
 }
 
-static inline int
+static int
 find_check_match(struct ipt_entry_match *m,
 		 const char *name,
 		 const struct ipt_ip *ip,

@@ -652,7 +658,7 @@ err:
 	return ret;
 }
 
-static inline int check_target(struct ipt_entry *e, const char *name)
+static int check_target(struct ipt_entry *e, const char *name)
 {
 	struct ipt_entry_target *t;
 	struct xt_target *target;

@@ -673,7 +679,7 @@ static inline int check_target(struct ipt_entry *e, const char *name)
 	return ret;
 }
 
-static inline int
+static int
 find_check_entry(struct ipt_entry *e, const char *name, unsigned int size,
 		 unsigned int *i)
 {

@@ -717,7 +723,7 @@ find_check_entry(struct ipt_entry *e, const char *name, unsigned int size,
 	return ret;
 }
 
-static inline int
+static int
 check_entry_size_and_hooks(struct ipt_entry *e,
 			   struct xt_table_info *newinfo,
 			   unsigned char *base,

@@ -760,7 +766,7 @@ check_entry_size_and_hooks(struct ipt_entry *e,
 	return 0;
 }
 
-static inline int
+static int
 cleanup_entry(struct ipt_entry *e, unsigned int *i)
 {
 	struct ipt_entry_target *t;

@@ -916,7 +922,7 @@ get_counters(const struct xt_table_info *t,
 	}
 }
 
-static inline struct xt_counters * alloc_counters(struct xt_table *table)
+static struct xt_counters * alloc_counters(struct xt_table *table)
 {
 	unsigned int countersize;
 	struct xt_counters *counters;

@@ -1304,7 +1310,7 @@ do_replace(void __user *user, unsigned int len)
 
 /* We're lazy, and add to the first CPU; overflow works its fey magic
  * and everything is OK. */
-static inline int
+static int
 add_counter_to_entry(struct ipt_entry *e,
 		     const struct xt_counters addme[],
 		     unsigned int *i)

@@ -1465,7 +1471,7 @@ out:
 	return ret;
 }
 
-static inline int
+static int
 compat_find_calc_match(struct ipt_entry_match *m,
 		       const char *name,
 		       const struct ipt_ip *ip,

@@ -1489,7 +1495,7 @@ compat_find_calc_match(struct ipt_entry_match *m,
 	return 0;
 }
 
-static inline int
+static int
 compat_release_match(struct ipt_entry_match *m, unsigned int *i)
 {
 	if (i && (*i)-- == 0)

@@ -1499,7 +1505,7 @@ compat_release_match(struct ipt_entry_match *m, unsigned int *i)
 	return 0;
 }
 
-static inline int
+static int
 compat_release_entry(struct compat_ipt_entry *e, unsigned int *i)
 {
 	struct ipt_entry_target *t;

@@ -1514,7 +1520,7 @@ compat_release_entry(struct compat_ipt_entry *e, unsigned int *i)
 	return 0;
 }
 
-static inline int
+static int
 check_compat_entry_size_and_hooks(struct compat_ipt_entry *e,
 				  struct xt_table_info *newinfo,
 				  unsigned int *size,

@@ -1637,7 +1643,8 @@ compat_copy_entry_from_user(struct compat_ipt_entry *e, void **dstptr,
 	return ret;
 }
 
-static inline int compat_check_entry(struct ipt_entry *e, const char *name,
+static int
+compat_check_entry(struct ipt_entry *e, const char *name,
 				     unsigned int *i)
 {
 	int j, ret;

net/ipv6/netfilter/ip6_tables.c +23 ~~-19~~

168 lines not on this page · raw message

Patrick McHardy

2007-12-31 17:00 UTC #5 lore raw

Re: [PATCH] ip[6]_tables.c: remove some inlines

Quoted 5 lines · Denys Vlasenko

ip[6]_tables.c seem to abuse inline.
This patch removes inlines except those which are used by packet matching code and thus are performance-critical.

Some people also consider the ruleset replacement path performance critical, but overall I agree with your patch. I'm travelling currently though so it will take a few days until I'll apply it.

Do you have some numbers that show the actual difference these changes make?

Denys Vlasenko

author

2007-12-31 17:55 UTC #6 lore raw

Re: [PATCH] ip[6]_tables.c: remove some inlines

Quoted 7 lines · Patrick McHardy

Quoted 5 lines · Denys Vlasenko
ip[6]_tables.c seem to abuse inline.
This patch removes inlines except those which are used by packet matching code and thus are performance-critical.

Quoted 6 lines

Some people also consider the ruleset replacement path performance critical, but overall I agree with your patch. I'm travelling currently though so it will take a few days until I'll apply it.
Do you have some numbers that show the actual difference these changes make?

Before:

$ size */*/*/ip*tables*.o
   text    data     bss     dec     hex filename
   6402     500      16    6918    1b06 net/ipv4/netfilter/ip_tables.o
   7130     500      16    7646    1dde net/ipv6/netfilter/ip6_tables.o

After:

$ size */*/*/ip*tables*.o
   text    data     bss     dec     hex filename
   6307     500      16    6823    1aa7 net/ipv4/netfilter/ip_tables.o
   7010     500      16    7526    1d66 net/ipv6/netfilter/ip6_tables.o

--
vda

Patrick McHardy

Applied

2008-01-08 16:36 UTC #7 lore raw

Re: [PATCH] ip[6]_tables.c: remove some inlines

Quoted 21 lines · Denys Vlasenko

Quoted 6 lines · Patrick McHardy
Quoted 5 lines · Denys Vlasenko
ip[6]_tables.c seem to abuse inline.
This patch removes inlines except those which are used by packet matching code and thus are performance-critical.
Quoted 6 lines
Some people also consider the ruleset replacement path performance critical, but overall I agree with your patch. I'm travelling currently though so it will take a few days until I'll apply it.
Do you have some numbers that show the actual difference these changes make?
Before:
$ size */*/*/ip*tables*.o
   text    data     bss     dec     hex filename
   6402     500      16    6918    1b06 net/ipv4/netfilter/ip_tables.o
   7130     500      16    7646    1dde net/ipv6/netfilter/ip6_tables.o

Quoted 6 lines

After:

$ size */*/*/ip*tables*.o
   text    data     bss     dec     hex filename
   6307     500      16    6823    1aa7 net/ipv4/netfilter/ip_tables.o
   7010     500      16    7526    1d66 net/ipv6/netfilter/ip6_tables.o

Applied, thanks.