Turn part of SNMP accounting macros into functions

RFC PATCH In review Pavel Emelyanov 6 participants 12 messages Aug 27, 2008 – Aug 29, 2008

Subsystems (4)

Networking Network drivers IPv4/IPv6 TCP

lore

Share & tools

lore

Pavel Emelyanov

author

2008-08-27 16:31 UTC #0 lore raw

RFC PATCH 1/1 Turn part of SNMP accounting macros into functions

After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:

add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)

Turning the CONFIG_NET_NS option on makes this shrink even larger:

add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)

So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).

Signed-off-by Pavel Emelyanov <[email protected]>

include/net/ip.h +10 -8

index 250e6ef..ad8ef7b 100644
@@ -156,14 +156,16 @@ struct ipv4_config
 };
 
 extern struct ipv4_config ipv4_config;
-#define IP_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.ip_statistics, field)
-#define IP_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.ip_statistics, field)
-#define IP_ADD_STATS_BH(net, field, val) SNMP_ADD_STATS_BH((net)->mib.ip_statistics, field, val)
-#define NET_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.net_statistics, field)
-#define NET_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.net_statistics, field)
-#define NET_INC_STATS_USER(net, field) 	SNMP_INC_STATS_USER((net)->mib.net_statistics, field)
-#define NET_ADD_STATS_BH(net, field, adnd) SNMP_ADD_STATS_BH((net)->mib.net_statistics, field, adnd)
-#define NET_ADD_STATS_USER(net, field, adnd) SNMP_ADD_STATS_USER((net)->mib.net_statistics, field, adnd)
+
+void IP_INC_STATS(struct net *net, int field);
+void IP_INC_STATS_BH(struct net *net, int field);
+void IP_ADD_STATS_BH(struct net *net, int field, int val);
+
+void NET_INC_STATS(struct net *net, int field);
+void NET_INC_STATS_BH(struct net *net, int field);
+void NET_INC_STATS_USER(struct net *net, int field);
+void NET_ADD_STATS_BH(struct net *net, int field, int val);
+void NET_ADD_STATS_USER(struct net *net, int field, int val);
 
 extern unsigned long snmp_fold_field(void *mib[], int offt);
 extern int snmp_mib_init(void *ptr[2], size_t mibsize);

include/net/tcp.h +4 -4

index 8983386..b973e68 100644
@@ -267,10 +267,10 @@ static inline int tcp_too_many_orphans(struct sock *sk, int num)
 
 extern struct proto tcp_prot;
 
-#define TCP_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.tcp_statistics, field)
-#define TCP_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.tcp_statistics, field)
-#define TCP_DEC_STATS(net, field)	SNMP_DEC_STATS((net)->mib.tcp_statistics, field)
-#define TCP_ADD_STATS_USER(net, field, val) SNMP_ADD_STATS_USER((net)->mib.tcp_statistics, field, val)
+void TCP_INC_STATS(struct net *net, int field);
+void TCP_INC_STATS_BH(struct net *net, int field);
+void TCP_DEC_STATS(struct net *net, int field);
+void TCP_ADD_STATS_USER(struct net *net, int field, int val);
 
 extern void			tcp_v4_err(struct sk_buff *skb, u32);

net/ipv4/af_inet.c +60 -0

63 lines not on this page · raw message

Stephen Hemminger

2008-08-27 16:41 UTC #1 lore raw

Quoted 124 lines · On Wed, 27 Aug 2008 20:31:25 +0400 Pavel Emelyanov <[email protected]> wrote:

After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:

add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)

Turning the CONFIG_NET_NS option on makes this shrink even larger:

add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)

So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).

Signed-off-by Pavel Emelyanov <[email protected]>

include/net/ip.h +10 -8

index 250e6ef..ad8ef7b 100644
@@ -156,14 +156,16 @@ struct ipv4_config
 };
 
 extern struct ipv4_config ipv4_config;
-#define IP_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.ip_statistics, field)
-#define IP_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.ip_statistics, field)
-#define IP_ADD_STATS_BH(net, field, val) SNMP_ADD_STATS_BH((net)->mib.ip_statistics, field, val)
-#define NET_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.net_statistics, field)
-#define NET_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.net_statistics, field)
-#define NET_INC_STATS_USER(net, field) 	SNMP_INC_STATS_USER((net)->mib.net_statistics, field)
-#define NET_ADD_STATS_BH(net, field, adnd) SNMP_ADD_STATS_BH((net)->mib.net_statistics, field, adnd)
-#define NET_ADD_STATS_USER(net, field, adnd) SNMP_ADD_STATS_USER((net)->mib.net_statistics, field, adnd)
+
+void IP_INC_STATS(struct net *net, int field);
+void IP_INC_STATS_BH(struct net *net, int field);
+void IP_ADD_STATS_BH(struct net *net, int field, int val);
+
+void NET_INC_STATS(struct net *net, int field);
+void NET_INC_STATS_BH(struct net *net, int field);
+void NET_INC_STATS_USER(struct net *net, int field);
+void NET_ADD_STATS_BH(struct net *net, int field, int val);
+void NET_ADD_STATS_USER(struct net *net, int field, int val);
 
 extern unsigned long snmp_fold_field(void *mib[], int offt);
 extern int snmp_mib_init(void *ptr[2], size_t mibsize);

include/net/tcp.h +4 -4

index 8983386..b973e68 100644
@@ -267,10 +267,10 @@ static inline int tcp_too_many_orphans(struct sock *sk, int num)
 
 extern struct proto tcp_prot;
 
-#define TCP_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.tcp_statistics, field)
-#define TCP_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.tcp_statistics, field)
-#define TCP_DEC_STATS(net, field)	SNMP_DEC_STATS((net)->mib.tcp_statistics, field)
-#define TCP_ADD_STATS_USER(net, field, val) SNMP_ADD_STATS_USER((net)->mib.tcp_statistics, field, val)
+void TCP_INC_STATS(struct net *net, int field);
+void TCP_INC_STATS_BH(struct net *net, int field);
+void TCP_DEC_STATS(struct net *net, int field);
+void TCP_ADD_STATS_USER(struct net *net, int field, int val);
 
 extern void			tcp_v4_err(struct sk_buff *skb, u32);

net/ipv4/af_inet.c +49 -0

index 8a3ac1f..413adda 100644
@@ -1580,3 +1580,63 @@ EXPORT_SYMBOL(inet_stream_connect);
 EXPORT_SYMBOL(inet_stream_ops);
 EXPORT_SYMBOL(inet_unregister_protosw);
 EXPORT_SYMBOL(sysctl_ip_nonlocal_bind);
+
+void NET_INC_STATS(struct net *net, int field)
+{
+	SNMP_INC_STATS((net)->mib.net_statistics, field);
+}
+
+void NET_INC_STATS_BH(struct net *net, int field)
+{
+	SNMP_INC_STATS_BH((net)->mib.net_statistics, field);
+}
+
+void NET_INC_STATS_USER(struct net *net, int field)
+{
+	SNMP_INC_STATS_USER((net)->mib.net_statistics, field);
+}
+
+void NET_ADD_STATS_BH(struct net *net, int field, int adnd)
+{
+	SNMP_ADD_STATS_BH((net)->mib.net_statistics, field, adnd);
+}
+
+void NET_ADD_STATS_USER(struct net *net, int field, int adnd)
+{
+	SNMP_ADD_STATS_USER((net)->mib.net_statistics, field, adnd);
+}
+
+void IP_INC_STATS(struct net *net, int field)
+{
+	SNMP_INC_STATS((net)->mib.ip_statistics, field);
+}
+
+void IP_INC_STATS_BH(struct net *net, int field)
+{
+	SNMP_INC_STATS_BH((net)->mib.ip_statistics, field);
+}
+
+void IP_ADD_STATS_BH(struct net *net, int field, int val)
+{
+	SNMP_ADD_STATS_BH((net)->mib.ip_statistics, field, val);
+}
+
+void TCP_INC_STATS(struct net *net, int field)
+{
+	SNMP_INC_STATS((net)->mib.tcp_statistics, field);
+}
+
+void TCP_INC_STATS_BH(struct net *net, int field)
+{
+	SNMP_INC_STATS_BH((net)->mib.tcp_statistics, field);

net/ipv4/af_inet.c EXPORT_SYMBOL(inet_stream_connect); · line 1641

Quoted 10 lines

net/ipv4/af_inet.c +10 -0

+}
+
+void TCP_DEC_STATS(struct net *net, int field)
+{
+	SNMP_DEC_STATS((net)->mib.tcp_statistics, field);
+}
+
+void TCP_ADD_STATS_USER(struct net *net, int field, int val)
+{
+	SNMP_ADD_STATS_USER((net)->mib.tcp_statistics, field, val);

Change function names to lower case please.

Ilpo Järvinen

2008-08-27 16:56 UTC #2 lore raw

In reply to Pavel Emelyanov

Quoted 8 lines · Pavel Emelyanov

After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)

Is there perhaps some DEBUG related config that could bloat these numbers? ...I didn't come up with anything obvious when I quickly looked the definations but I might have missed something.

Quoted 3 lines

So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

IMHO, even if there would be some performance regression, only the most hottest paths would need to have it inlined, the rest would be quite fine having it as a function.

--
i.

Pavel Emelyanov

author

2008-08-27 17:10 UTC #3 lore raw

Quoted 5 lines · Ilpo Järvinen

Quoted 3 lines · Pavel Emelyanov
After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:

Quoted 7 lines

Quoted 5 lines
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)
Is there perhaps some DEBUG related config that could bloat these numbers?

Nope. I only had a DEBUG_BUG_VERBOSE turned on in my experiments.

Quoted 2 lines

...I didn't come up with anything obvious when I quickly looked the definations but I might have missed something.

Well, looking at NET_INC_STATS_BH as a function

000000a9 <NET_INC_STATS_BH>:

      a9:       8b 40 74                mov    0x74(%eax),%eax
      ac:       64 8b 0d 00 00 00 00    mov    %fs:0x0,%ecx
                        af: R_386_32    per_cpu__cpu_number
      b3:       f7 d0                   not    %eax
      b5:       8b 04 88                mov    (%eax,%ecx,4),%eax
      b8:       ff 04 90                incl   (%eax,%edx,4)
      bb:       c3                      ret

I guess that the heaviest thing there is smp_processor_id, which is 7 bytes out of 19 total.

Actually the complete bloat-o-meter.sh output showed that there were ~10 functions, that became 8 bytes liter, ~10 that became 12-16 bytes liter and so on. This makes me think, that the code in the macro itself is pretty small, but is used widely in the networking code, and this spread makes such a big bloat.

Quoted 7 lines

Quoted 3 lines
So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).
IMHO, even if there would be some performance regression, only the most hottest paths would need to have it inlined, the rest would be quite fine having it as a function.

OK, thanks Ilpo. I'll then look at how to make this more beautiful and send to Dave.

Quoted 3 lines

--
i.

Ilpo Järvinen

2008-08-27 17:58 UTC #4 lore raw

Quoted 5 lines · Pavel Emelyanov

Actually the complete bloat-o-meter.sh output showed that there were ~10 functions, that became 8 bytes liter, ~10 that became 12-16 bytes liter and so on. This makes me think, that the code in the macro itself is pretty small, but is used widely in the networking code, and this spread makes such a big bloat.

I once measured all static inlines in the headers and the largest bloaters where of that type where the function itself is quite small but it's called from so many places that it add up.

Signature · 1 line

i.

Eric Dumazet

2008-08-27 17:07 UTC #5 lore raw

In reply to Pavel Emelyanov

Quoted 13 lines · Pavel Emelyanov

After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)
So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

Quoted 6 lines

The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).
Signed-off-by Pavel Emelyanov <[email protected]>

I dont know, but passing all those "struct net *net" to every network function in the kernel sounds overkill, especially for !CONFIG_NET_NS users. This is pure bloat.

We could define two macros so that function prototypes dont include useless pointers, especially on arches where only first and second parameter is passed in eax and edx register ;)

#ifdef CONFIG_NET_NS

# define VNETPTR ,struct net *net
# define NETPTR  net
#else
# define VNETPTR
# defint NETPTR  &init_net
#endif

...
void TCP_DEC_STATS(int field VNETPTR);
...
void TCP_DEC_STATS(int field VNETPTR)

{
	SNMP_DEC_STATS((NETPTR)->mib.tcp_statistics, field);
}

...

Pavel Emelyanov

author

2008-08-28 11:12 UTC #6 lore raw

Quoted 38 lines · Eric Dumazet

Quoted 13 lines · Pavel Emelyanov
After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)
So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).
Quoted 6 lines
The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).
Signed-off-by Pavel Emelyanov <[email protected]>
I dont know, but passing all those "struct net *net" to every network function in the kernel sounds overkill, especially for !CONFIG_NET_NS users. This is pure bloat.
We could define two macros so that function prototypes dont include useless pointers, especially on arches where only first and second parameter is passed in eax and edx register ;)
#ifdef CONFIG_NET_NS
# define VNETPTR ,struct net *net
# define NETPTR  net
#else
# define VNETPTR
# defint NETPTR  &init_net
#endif

Quoted 8 lines

...
void TCP_DEC_STATS(int field VNETPTR);
...
void TCP_DEC_STATS(int field VNETPTR)
{
	SNMP_DEC_STATS((NETPTR)->mib.tcp_statistics, field);
}
...

I agree with this, but I've checked whether compiling out the first argument of XXX_STATS would help and found out that there's almost no difference (-9 bytes) in the net/ipv4/built-in.o for NET_NS=n case.

After turning the macros into functions, compiling the first argument out gives us 500 more bytes out. Good catch, but I have a question - the way you proposed to implement the function itself is rather nice, but how would you implement the *call* to that function in order to make it variable-arguments depending on the config option?

I see the similar way:

+#ifdef CONFIG_NET_NS
+#define NET_ARG(net) net,
+#else
+#define NET_ARG(net)
+#endif
...
- TCP_INC_STATS(sock_net(sk), field);
+ TCP_INC_STATS(NET_ARG(sock_net(sk) field);

but do these 500 bytes compensate for this ugly style?

Eric Dumazet

2008-08-28 12:51 UTC #7 lore raw

Quoted 47 lines · Pavel Emelyanov

Quoted 16 lines · Eric Dumazet
I dont know, but passing all those "struct net *net" to every network function in the kernel sounds overkill, especially for !CONFIG_NET_NS users. This is pure bloat.
We could define two macros so that function prototypes dont include useless pointers, especially on arches where only first and second parameter is passed in eax and edx register ;)
#ifdef CONFIG_NET_NS
# define VNETPTR ,struct net *net
# define NETPTR  net
#else
# define VNETPTR
# defint NETPTR  &init_net
#endif
Quoted 8 lines
...
void TCP_DEC_STATS(int field VNETPTR);
...
void TCP_DEC_STATS(int field VNETPTR)
{
	SNMP_DEC_STATS((NETPTR)->mib.tcp_statistics, field);
}
...
I agree with this, but I've checked whether compiling out the first argument of XXX_STATS would help and found out that there's almost no difference (-9 bytes) in the net/ipv4/built-in.o for NET_NS=n case.
After turning the macros into functions, compiling the first argument out gives us 500 more bytes out. Good catch, but I have a question - the way you proposed to implement the function itself is rather nice, but how would you implement the *call* to that function in order to make it variable-arguments depending on the config option?
I see the similar way:
(quoted diff) +6 -1
+#ifdef CONFIG_NET_NS
+#define NET_ARG(net) net,
+#else
+#define NET_ARG(net)
+#endif
...
-	TCP_INC_STATS(sock_net(sk), field);
+	TCP_INC_STATS(NET_ARG(sock_net(sk) field);

Quoted 1 line

but do these 500 bytes compensate for this ugly style?

500 bytes here, but what about other uses in kernel ?

Sometime I wonder if a *global* pointer ('current' pointer for example is not passed to every kernel function that want access to current struture) would save many kbytes in vmlinux text. This way, !CONFIG_NET_NS overhead would completely vanish.

Oh well...

Pavel Emelyanov

author

2008-08-28 12:54 UTC #8 lore raw

Quoted 4 lines

Sometime I wonder if a *global* pointer ('current' pointer for example is not passed to every kernel function that want access to current struture) would save many kbytes in vmlinux text. This way, !CONFIG_NET_NS overhead would completely vanish.

:) I'm glad to hear, that we're not alone with such a view of how this have to work. We at OpenVZ proposed the current_net macro from the very beginning, but other netns developers decided to go the other way :(

Quoted 1 line

Oh well...

Oh well, indeed...

Herbert Xu

2008-08-29 07:01 UTC #9 lore raw

In reply to Pavel Emelyanov

Quoted 13 lines · Pavel Emelyanov

After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)
So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

Quoted 5 lines

The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).
Signed-off-by Pavel Emelyanov <[email protected]>

What happened to Christoph Lameter's new per-cpu stuff? That would allows us to turn these into a single inc/add instruction.

So I think we should put this patch on the back-burner until the status of the per-cpu stuff is settled.

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Pavel Emelyanov

author

2008-08-29 07:29 UTC #10 lore raw

Quoted 14 lines · Herbert Xu

Quoted 13 lines · Pavel Emelyanov
After turning IP_XXX_STATS, TCP_XXX_STATS and NET_XXX_STATS from macros into functions the net/ipv4/built-in.o shrank significantly:
add/remove: 14/0 grow/shrink: 0/67 up/down: 482/-2246 (-1764)
Turning the CONFIG_NET_NS option on makes this shrink even larger:
add/remove: 14/0 grow/shrink: 0/67 up/down: 478/-2646 (-2168)
So the question is - what was the reason to keep those as macros? I thought about the possible performance questions, but netperf didn't show any (I admit I just cannot cook it properly).

Quoted 8 lines

Quoted 5 lines
The sample patch is here, but it's not good (EXPORTs for ipv6 and a better place for functions rather than net/ipv4/af_inet.c are required).
Signed-off-by Pavel Emelyanov <[email protected]>
What happened to Christoph Lameter's new per-cpu stuff? That would allows us to turn these into a single inc/add instruction.

I've never heard about it, but if you tell, that this would make things *that* simple, then of course I will delay with this patch.

Quoted 2 lines

So I think we should put this patch on the back-burner until the status of the per-cpu stuff is settled.

OK, thanks.

Quoted 1 line

Thanks,

Christoph Lameter

2008-08-29 12:59 UTC #11 lore raw

In reply to Herbert Xu

Quoted 5 lines · Herbert Xu

What happened to Christoph Lameter's new per-cpu stuff? That would allows us to turn these into a single inc/add instruction.
So I think we should put this patch on the back-burner until the status of the per-cpu stuff is settled.

Well the zero based patchset is still not in for x86_64. The percpu stuff would only work for x86_32. 32 bit uses the available segment register for per cpu stuff. 64 bit wastes it on the pda.

Maybe its okay if itll just work for 32bit and it would put pressure on x86_64 to get the crap fixed up?