[PATCH] 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.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 <redacted>

--- 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:

This is broken on older compilers.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
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:

quoted

--- 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 <redacted>

--- 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

On Wed, 2004-10-20 at 15:14, Denis Vlasenko wrote:

On Wednesday 20 October 2004 19:47, Lee Revell wrote:

quoted

On Wed, 2004-10-20 at 11:11, Denis Vlasenko wrote:

quoted

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 <redacted>

--- 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;
 }
 

non-inline functions must not live in .h files
--
vda

On Wed, 2004-10-20 at 15:56, Denis Vlasenko wrote:

quoted

OK, third try.

Signed-Off-By: Lee Revell <redacted>

--- 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 Wednesday 20 October 2004 23:25, Lee Revell wrote:

quoted

quoted

--- 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, 20 Oct 2004 23:32:33 +0300
Denis Vlasenko [off-list ref] wrote:

On Wednesday 20 October 2004 23:25, Lee Revell wrote:

quoted

quoted

quoted

--- 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 davem@nuts.davemloft.net 
#   [NET]: Uninline netif_rx_ni().
#   
#   It expands to a lot of code when SMP or PREEMPT is
#   enabled.
#   
#   Signed-off-by: David S. Miller [off-list ref]
# 
# net/core/dev.c
#   2004/10/20 16:57:03-07:00 davem@nuts.davemloft.net +14 -0
#   [NET]: Uninline netif_rx_ni().
# 
# include/linux/netdevice.h
#   2004/10/20 16:57:03-07:00 davem@nuts.davemloft.net +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~} [off-list ref]
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 [off-list ref] wrote:

On Wed, Oct 20, 2004 at 05:15:08PM -0700, David S. Miller wrote:

quoted

 
+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:

quoted

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