This patch makes netif_rx_ni() preempt-safe. The problem was reported
by Alain Schroeder. Here are the users:
drivers/s390/net/ctcmain.c
drivers/s390/net/netiucv.c
drivers/net/irda/vlsi_ir.c
drivers/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.
Signed-Off-By: Lee Revell <[email protected]>
--- include/linux/netdevice.h~ 2004-10-19 18:50:18.000000000 -0400
+++ include/linux/netdevice.h 2004-10-19 18:51:01.000000000 -0400
@@ -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;
}
On Tue, Oct 19, 2004 at 07:55:33PM -0400, Lee Revell wrote:
>
> --- include/linux/netdevice.h~ 2004-10-19 18:50:18.000000000 -0400
> +++ include/linux/netdevice.h 2004-10-19 18:51:01.000000000 -0400
> @@ -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.
--
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
On Tue, 2004-10-19 at 20:00, Herbert Xu wrote:
> On Tue, Oct 19, 2004 at 07:55:33PM -0400, Lee Revell wrote:
> >
> > --- include/linux/netdevice.h~ 2004-10-19 18:50:18.000000000 -0400
> > +++ include/linux/netdevice.h 2004-10-19 18:51:01.000000000 -0400
> > @@ -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:
Signed-Off-By: Lee Revell <[email protected]>
--- include/linux/netdevice.h~ 2004-10-19 20:16:48.000000000 -0400
+++ include/linux/netdevice.h 2004-10-19 20:21:01.000000000 -0400
@@ -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;
}
> How about this:
>
> Signed-Off-By: Lee Revell <[email protected]>
>
> --- include/linux/netdevice.h~ 2004-10-19 20:16:48.000000000 -0400
> +++ include/linux/netdevice.h 2004-10-19 20:21:01.000000000 -0400
> @@ -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;
> }
#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
On Wed, 2004-10-20 at 11:11, Denis Vlasenko wrote:
> 0x57 == 87 bytes is too big for inline.
Ugh. So the only fix is not to inline it?
Lee
> #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
BTW, I once got an email which claimed that compilers
optimize code better than humans. Aha... here it is:
On Sunday 11 April 2004 23:58, J. Ryan Earl wrote:
>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?)
> 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)
> 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...)
> 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
> 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(
> 50: e8 fc ff ff ff call 51 <netif_rx_ni_+0x51>
> 55: eb d4 jmp 2b <netif_rx_ni_+0x2b>
--
vda
On Wed, 2004-10-20 at 15:14, Denis Vlasenko wrote:
> On Wednesday 20 October 2004 19:47, Lee Revell wrote:
> > On Wed, 2004-10-20 at 11:11, Denis Vlasenko wrote:
> > > 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.
Signed-Off-By: Lee Revell <[email protected]>
--- include/linux/netdevice.h~ 2004-10-20 15:51:00.000000000 -0400
+++ include/linux/netdevice.h 2004-10-20 15:51:54.000000000 -0400
@@ -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;
}
> OK, third try.
>
> Signed-Off-By: Lee Revell <[email protected]>
>
> --- include/linux/netdevice.h~ 2004-10-20 15:51:00.000000000 -0400
> +++ include/linux/netdevice.h 2004-10-20 15:51:54.000000000 -0400
> @@ -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
On Wed, 2004-10-20 at 15:55, Denis Vlasenko wrote:
> 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
On Wednesday 20 October 2004 23:25, Lee Revell wrote:
> > > --- include/linux/netdevice.h~ 2004-10-20 15:51:00.000000000 -0400
> > > +++ include/linux/netdevice.h 2004-10-20 15:51:54.000000000 -0400
> > > @@ -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?
Somewhere near this place:
http://lxr.linux.no/source/net/core/dev.c?v=2.6.8.1#L1555
--
vda
On Wed, 2004-10-20 at 15:56, Denis Vlasenko wrote:
> > OK, third try.
> >
> > Signed-Off-By: Lee Revell <[email protected]>
> >
> > --- include/linux/netdevice.h~ 2004-10-20 15:51:00.000000000 -0400
> > +++ include/linux/netdevice.h 2004-10-20 15:51:54.000000000 -0400
> > @@ -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
On Wed, 20 Oct 2004 23:32:33 +0300
Denis Vlasenko <[email protected]> wrote:
> On Wednesday 20 October 2004 23:25, Lee Revell wrote:
> > > > --- include/linux/netdevice.h~ 2004-10-20 15:51:00.000000000 -0400
> > > > +++ include/linux/netdevice.h 2004-10-20 15:51:54.000000000 -0400
> > > > @@ -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?
>
> Somewhere near this place:
>
> http://lxr.linux.no/source/net/core/dev.c?v=2.6.8.1#L1555
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);
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c 2004-10-20 16:58:28 -07:00
+++ b/net/core/dev.c 2004-10-20 16:58:28 -07:00
@@ -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;
On Wed, Oct 20, 2004 at 05:15:08PM -0700, David S. Miller wrote:
>
> +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,
--
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
On Thu, 21 Oct 2004 10:35:03 +1000
Herbert Xu <[email protected]> wrote:
> On Wed, Oct 20, 2004 at 05:15:08PM -0700, David S. Miller wrote:
> >
> > +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.
Good catch, I've made that fix in my tree.
Thanks.
On Wednesday 20 October 2004 19:47, Lee Revell wrote:
> On Wed, 2004-10-20 at 11:11, Denis Vlasenko wrote:
> > 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