Make netif_rx_ni preempt-safe
Subsystems (2)
PATCH 1/1 Make netif_rx_ni preempt-safe
This patch makes netif_rx_ni() preempt-safe. The problem was reported by Alain Schroeder. Here are the users:
drivers/s390/net/ctcmain.cdrivers/s390/net/netiucv.cdrivers/net/irda/vlsi_ir.cdrivers/net/tun.c
As David S. Miller explained, the do_softirq (and therefore the preempt dis/enable) is required because there is no softirq check on the return path when netif_rx is called from non-interrupt context.
linux/netdevice.h renamed from linux/netdevice.h~ +2 -0
@@ -696,9 +696,11 @@*/ static inline int netif_rx_ni (struct sk_buff *skb) { +preempt_disable ();int err =netif_rx (skb);if (softirq_pending (smp_processor_id ()))do_softirq (); +preempt_enable ();return err; }
linux/netdevice.h line 700
Quoted 9 lines · Lee Revell
linux/netdevice.h renamed from linux/netdevice.h~ +1
-0@@ -696,9 +696,11 @@*/ static inline int netif_rx_ni (struct sk_buff *skb) { +preempt_disable ();int err =netif_rx (skb);
This is broken on older compilers.
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.txtQuoted 12 lines · Herbert Xu
linux/netdevice.h line 700
Quoted 9 lines · Lee Revell
linux/netdevice.h renamed from linux/netdevice.h~ +1
-0@@ -696,9 +696,11 @@*/ static inline int netif_rx_ni (struct sk_buff *skb) { +preempt_disable ();int err =netif_rx (skb);This is broken on older compilers.
How about this:
linux/netdevice.h renamed from linux/netdevice.h~ +4 -1
@@ -696,9 +696,12 @@*/ static inline int netif_rx_ni (struct sk_buff *skb) { -int err =netif_rx (skb); +int err; +preempt_disable (); + err =netif_rx (skb);if (softirq_pending (smp_processor_id ()))do_softirq (); +preempt_enable ();return err; }
Quoted 9 lines
How about this:
Signed-off-by Lee Revell <[email protected]>linux/netdevice.h renamed from linux/netdevice.h~ +0
-0@@ -696,9 +696,12 @@*/ static inline int netif_rx_ni (struct sk_buff *skb)
linux/netdevice.h line 706
Quoted 10 lines
linux/netdevice.h renamed from linux/netdevice.h~ +4
-1{ -int err =netif_rx (skb); +int err; +preempt_disable (); + err =netif_rx (skb);if (softirq_pending (smp_processor_id ()))do_softirq (); +preempt_enable ();return err; }
#include <linux/netdevice.h>
int netif_rx_ni_(struct sk_buff *skb)
{
int err;
preempt_disable();
err = netif_rx(skb);
if (softirq_pending(smp_processor_id()))
do_softirq();
preempt_enable();
return err;
}objdump -d:
00000000 <netif_rx_ni_>:
0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 56 push %esi 4: 53 push %ebx 5: bb 00 f0 ff ff mov $0xfffff000,%ebx a: 21 e3 and %esp,%ebx c: ff 43 14 incl 0x14(%ebx) f: 8b 4d 08 mov 0x8(%ebp),%ecx 12: 51 push %ecx 13: e8 fc ff ff ff call 14 <netif_rx_ni_+0x14> 18: 89 c6 mov %eax,%esi 1a: 8b 43 10 mov 0x10(%ebx),%eax 1d: c1 e0 07 shl $0x7,%eax 20: 8b 80 00 00 00 00 mov 0x0(%eax),%eax 26: 85 c0 test %eax,%eax 28: 5a pop %edx 29: 75 25 jne 50 <netif_rx_ni_+0x50> 2b: 8b 43 08 mov 0x8(%ebx),%eax 2e: ff 4b 14 decl 0x14(%ebx) 31: a8 08 test $0x8,%al 33: 75 09 jne 3e <netif_rx_ni_+0x3e> 35: 8d 65 f8 lea 0xfffffff8(%ebp),%esp 38: 5b pop %ebx 39: 89 f0 mov %esi,%eax 3b: 5e pop %esi 3c: 5d pop %ebp 3d: c3 ret 3e: e8 fc ff ff ff call 3f <netif_rx_ni_+0x3f> 43: eb f0 jmp 35 <netif_rx_ni_+0x35> 45: 8d 74 26 00 lea 0x0(%esi,1),%esi 49: 8d bc 27 00 00 00 00 lea 0x0(%edi,1),%edi 50: e8 fc ff ff ff call 51 <netif_rx_ni_+0x51> 55: eb d4 jmp 2b <netif_rx_ni_+0x2b>
0x57 == 87 bytes is too big for inline.
--
vda
Quoted 1 line · Denis Vlasenko
0x57== 87 bytes is too big for inline.
Ugh. So the only fix is not to inline it?
Lee
Quoted 4 lines · Lee Revell
Quoted 1 line · Denis Vlasenko
0x57== 87 bytes is too big for inline.Ugh. So the only fix is not to inline it?
Yes.
You can make it conditionally inline/non-inline depending on SMP/preempt if you feel masochistic today :), but last time I asked davem thought that it is over the top.
Deinline it.
--
vda
Quoted 11 lines · Denis Vlasenko
Quoted 4 lines · Lee Revell
Quoted 1 line · Denis Vlasenko
0x57== 87 bytes is too big for inline.Ugh. So the only fix is not to inline it?
Yes.
You can make it conditionally inline/non-inline depending on SMP/preempt if you feel masochistic today :), but last time I asked davem thought that it is over the top.
I agree, not worth the trouble. This would actually depend only on PREEMPT and not SMP.
OK, third try.
linux/netdevice.h renamed from linux/netdevice.h~ +5 -2
@@ -694,11 +694,14 @@/* Post buffer to the network code from _non interrupt_ context. * see net/core/dev.c for netif_rx description. */ -static inline int netif_rx_ni (struct sk_buff *skb) +static int netif_rx_ni (struct sk_buff *skb) { -int err =netif_rx (skb); +int err; +preempt_disable (); + err =netif_rx (skb);if (softirq_pending (smp_processor_id ()))do_softirq (); +preempt_enable ();return err; }
linux/netdevice.h line 697
Quoted 12 lines
OK, third try.
Signed-off-by Lee Revell <[email protected]>linux/netdevice.h renamed from linux/netdevice.h~ +1
-1@@ -694,11 +694,14 @@/* Post buffer to the network code from _non interrupt_ context. * see net/core/dev.c for netif_rx description. */ -static inline int netif_rx_ni (struct sk_buff *skb) +static int netif_rx_ni (struct sk_buff *skb)
non-inline functions must not live in .h files
--
vda
Quoted 4 lines · Denis Vlasenko
Quoted 4 lines
OK, third try.
Signed-off-by Lee Revell <[email protected]>
Quoted 10 lines
linux/netdevice.h line 697
Quoted 8 lines
linux/netdevice.h renamed from linux/netdevice.h~ +1
-1@@ -694,11 +694,14 @@/* Post buffer to the network code from _non interrupt_ context. * see net/core/dev.c for netif_rx description. */ -static inline int netif_rx_ni (struct sk_buff *skb) +static int netif_rx_ni (struct sk_buff *skb)non-inline functions must not live in .h files
Where do you suggest we put it?
Lee
Quoted 11 lines · Lee Revell
Quoted 10 lines
linux/netdevice.h line 697
Quoted 8 lines
linux/netdevice.h renamed from linux/netdevice.h~ +1
-1@@ -694,11 +694,14 @@/* Post buffer to the network code from _non interrupt_ context. * see net/core/dev.c for netif_rx description. */ -static inline int netif_rx_ni (struct sk_buff *skb) +static int netif_rx_ni (struct sk_buff *skb)non-inline functions must not live in .h files
Quoted 1 line
Where do you suggest we put it?
Somewhere near this place:
http://lxr.linux.no/source/net/core/dev.c?v=2.6.8.1#L1555
--
vda
David S. Miller
maintainerQuoted 12 lines · On Wed, 20 Oct 2004 23:32:33 +0300 Denis Vlasenko…
Quoted 11 lines · Lee Revell
Quoted 10 lines
linux/netdevice.h line 697
Quoted 8 lines
linux/netdevice.h renamed from linux/netdevice.h~ +1
-1@@ -694,11 +694,14 @@/* Post buffer to the network code from _non interrupt_ context. * see net/core/dev.c for netif_rx description. */ -static inline int netif_rx_ni (struct sk_buff *skb) +static int netif_rx_ni (struct sk_buff *skb)non-inline functions must not live in .h files
Quoted 5 lines
Quoted 1 line
Where do you suggest we put it?
Somewhere near this place:
I've done this as follows, thanks.
# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/10/20 16:57:53-07:00 [email protected]
# [NET]: Uninline netif_rx_ni().
#
# It expands to a lot of code when SMP or PREEMPT is
# enabled.
#
# Signed-off-by: David S. Miller <[email protected]>
#
# net/core/dev.c # 2004/10/20 16:57:03-07:00 [email protected] +14 -0 # [NET]: Uninline netif_rx_ni(). # # include/linux/netdevice.h # 2004/10/20 16:57:03-07:00 [email protected] +1 -15 # [NET]: Uninline netif_rx_ni(). # diff -Nru a/include/linux/netdevice.h b/include/linux/netdevice.h --- a/include/linux/netdevice.h 2004-10-20 16:58:28 -07:00 +++ b/include/linux/netdevice.h 2004-10-20 16:58:28 -07:00 @@ -677,6 +677,7 @@
#define HAVE_NETIF_RX 1
extern int netif_rx(struct sk_buff *skb);
+extern int netif_rx_ni(struct sk_buff *skb);
#define HAVE_NETIF_RECEIVE_SKB 1
extern int netif_receive_skb(struct sk_buff *skb);
extern int dev_ioctl(unsigned int cmd, void __user *);
@@ -690,21 +691,6 @@
extern void dev_init(void);
extern int netdev_nit;
-
-/* Post buffer to the network code from _non interrupt_ context.
- * see net/core/dev.c for netif_rx description.
- */
-static inline int netif_rx_ni(struct sk_buff *skb)
-{
- int err = netif_rx(skb);
-
- preempt_disable();
- if (softirq_pending(smp_processor_id()))
- do_softirq();
- preempt_enable();
-
- return err;
-}
/* Called by rtnetlink.c:rtnl_unlock() */
extern void netdev_run_todo(void);
net/core/dev.c +14 -0
@@ -1587,6 +1587,20 @@return NET_RX_DROP ; } +int netif_rx_ni (struct sk_buff *skb) +{ +int err =netif_rx (skb); + +preempt_disable (); +if (softirq_pending (smp_processor_id ())) +do_softirq (); +preempt_enable (); + +return err; +} + +EXPORT_SYMBOL (netif_rx_ni); +static __inline__void skb_bond (struct sk_buff *skb) {struct net_device *dev = skb->dev;
net/core/dev.c line 1596
Quoted 8 lines · David S. Miller
net/core/dev.c +7
-0+int netif_rx_ni (struct sk_buff *skb) +{ +int err =netif_rx (skb); + +preempt_disable (); +if (softirq_pending (smp_processor_id ())) +do_softirq ();
You need to move the netif_rx call inside the disable as otherwise you might be checking the pending flag on the wrong CPU.
Cheers,
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.txtDavid S. Miller
maintainerQuoted 12 lines · On Thu, 21 Oct 2004 10:35:03 +1000 Herbert Xu <[email protected]>…
net/core/dev.c line 1596
Quoted 8 lines · David S. Miller
net/core/dev.c +7
-0+int netif_rx_ni (struct sk_buff *skb) +{ +int err =netif_rx (skb); + +preempt_disable (); +if (softirq_pending (smp_processor_id ())) +do_softirq ();You need to move the
netif_rxcall inside the disable as otherwise you might be checking the pending flag on the wrong CPU.
Good catch, I've made that fix in my tree.
Thanks.
Quoted 13 lines
#include <
linux/netdevice.h>int
netif_rx_ni_(struct sk_buff *skb){ int err; preempt_disable(); err = netif_rx(skb); if (softirq_pending(smp_processor_id())) do_softirq(); preempt_enable(); return err; }
Quoted 3 lines
objdump -d:
00000000 <netif_rx_ni_>:
0: 55 push %ebp
BTW, I once got an email which claimed that compilers optimize code better than humans. Aha... here it is:
Quoted 2 lines · J. Ryan Earl
I doubt you would be capable of generating assembly that would be any faster than gcc
Let's take this code as a "random example". Code is not bad per se, but it can be made smaller _without_ sacrificing speed.
(Before someone says it - I don't buy the arguments like "push(mem) is microcoded - it is slower, use mov+push(reg)". Yes, it is *maybe* slower. On your today's CPU. Will it be slower on tomorrow's one? I doubt it. Specifically, on Athlon it was a VectorPath insn. On Athlon64 it is a Double => equivalent to mov+push(reg). But it takes less icache on each and every CPU from 486 upwards. Doesn't this "optimization" looks silly on Athlon64?)
Quoted 11 lines
objdump -d:
00000000 <netif_rx_ni_>:0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 56 push %esi 4: 53 push %ebx 5: bb 00 f0 ff ff mov $0xfffff000,%ebx a: 21 e3 and %esp,%ebx c: ff 43 14 incl 0x14(%ebx) f: 8b 4d 08 mov 0x8(%ebp),%ecx 12: 51 push %ecx
replace: mov+push => push 0x8(%ebp)
Quoted 6 lines
13: e8 fc ff ff ff call 14 <netif_rx_ni_+0x14> 18: 89 c6 mov %eax,%esi 1a: 8b 43 10 mov 0x10(%ebx),%eax 1d: c1 e0 07 shl $0x7,%eax 20: 8b 80 00 00 00 00 mov 0x0(%eax),%eax 26: 85 c0 test %eax,%eax
mov+test => cmp 0x0(%eax),0 (side note: 0x0 is not a 0, it's a reloc...)
Quoted 5 lines
28: 5a pop %edx 29: 75 25 jne 50 <netif_rx_ni_+0x50> 2b: 8b 43 08 mov 0x8(%ebx),%eax 2e: ff 4b 14 decl 0x14(%ebx) 31: a8 08 test $0x8,%al
mov+test => test 0x8(%ebx),$0x8
Quoted 11 lines
33: 75 09 jne 3e <netif_rx_ni_+0x3e> 35: 8d 65 f8 lea 0xfffffff8(%ebp),%esp 38: 5b pop %ebx 39: 89 f0 mov %esi,%eax 3b: 5e pop %esi 3c: 5d pop %ebp 3d: c3 ret 3e: e8 fc ff ff ff call 3f <netif_rx_ni_+0x3f> 43: eb f0 jmp 35 <netif_rx_ni_+0x35> 45: 8d 74 26 00 lea 0x0(%esi,1),%esi 49: 8d bc 27 00 00 00 00 lea 0x0(%edi,1),%edi
padding is larger that code it aligns! Zero gain in speed, 11 bytes wasted. >8(
Quoted 2 lines
50: e8 fc ff ff ff call 51 <netif_rx_ni_+0x51> 55: eb d4 jmp 2b <netif_rx_ni_+0x2b>
--
vda
Quoted 2 lines · Denis Vlasenko
padding is larger that code it aligns! Zero gain in speed, 11 bytes wasted. >8(
Still too big to inline it? You could submit a patch that converts it to asm... :-)
Lee