* [PATCH] Make netif_rx_ni preempt-safe
@ 2004-10-19 23:55 Lee Revell
2004-10-20 0:00 ` Herbert Xu
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 23:55 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, David S. Miller, herbert, vda, linux-kernel, maxk,
irda-users, Linux Network Development, Alain Schroeder
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 <rlrevell@joe-job.com>
--- 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;
}
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-19 23:55 [PATCH] Make netif_rx_ni preempt-safe Lee Revell @ 2004-10-20 0:00 ` Herbert Xu 2004-10-20 0:22 ` Lee Revell 0 siblings, 1 reply; 13+ messages in thread From: Herbert Xu @ 2004-10-20 0:00 UTC (permalink / raw) To: Lee Revell Cc: Andrew Morton, linux-kernel, David S. Miller, vda, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 0:00 ` Herbert Xu @ 2004-10-20 0:22 ` Lee Revell 2004-10-20 15:11 ` Denis Vlasenko 0 siblings, 1 reply; 13+ messages in thread From: Lee Revell @ 2004-10-20 0:22 UTC (permalink / raw) To: Herbert Xu Cc: Andrew Morton, linux-kernel, David S. Miller, vda, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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 <rlrevell@joe-job.com> --- 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 0:22 ` Lee Revell @ 2004-10-20 15:11 ` Denis Vlasenko 2004-10-20 16:47 ` Lee Revell 0 siblings, 1 reply; 13+ messages in thread From: Denis Vlasenko @ 2004-10-20 15:11 UTC (permalink / raw) To: Lee Revell, Herbert Xu Cc: Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder > How about this: > > Signed-Off-By: Lee Revell <rlrevell@joe-job.com> > > --- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 15:11 ` Denis Vlasenko @ 2004-10-20 16:47 ` Lee Revell 2004-10-20 19:14 ` Denis Vlasenko 0 siblings, 1 reply; 13+ messages in thread From: Lee Revell @ 2004-10-20 16:47 UTC (permalink / raw) To: Denis Vlasenko Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 16:47 ` Lee Revell @ 2004-10-20 19:14 ` Denis Vlasenko 2004-10-20 19:53 ` Lee Revell 0 siblings, 1 reply; 13+ messages in thread From: Denis Vlasenko @ 2004-10-20 19:14 UTC (permalink / raw) To: Lee Revell Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 19:14 ` Denis Vlasenko @ 2004-10-20 19:53 ` Lee Revell 2004-10-20 19:56 ` Denis Vlasenko 0 siblings, 1 reply; 13+ messages in thread From: Lee Revell @ 2004-10-20 19:53 UTC (permalink / raw) To: Denis Vlasenko Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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 <rlrevell@joe-job.com> --- 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 19:53 ` Lee Revell @ 2004-10-20 19:56 ` Denis Vlasenko 2004-10-20 20:25 ` Lee Revell 0 siblings, 1 reply; 13+ messages in thread From: Denis Vlasenko @ 2004-10-20 19:56 UTC (permalink / raw) To: Lee Revell Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder > OK, third try. > > Signed-Off-By: Lee Revell <rlrevell@joe-job.com> > > --- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 19:56 ` Denis Vlasenko @ 2004-10-20 20:25 ` Lee Revell 2004-10-20 20:32 ` Denis Vlasenko 0 siblings, 1 reply; 13+ messages in thread From: Lee Revell @ 2004-10-20 20:25 UTC (permalink / raw) To: Denis Vlasenko Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder On Wed, 2004-10-20 at 15:56, Denis Vlasenko wrote: > > OK, third try. > > > > Signed-Off-By: Lee Revell <rlrevell@joe-job.com> > > > > --- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 20:25 ` Lee Revell @ 2004-10-20 20:32 ` Denis Vlasenko 2004-10-21 0:15 ` David S. Miller 0 siblings, 1 reply; 13+ messages in thread From: Denis Vlasenko @ 2004-10-20 20:32 UTC (permalink / raw) To: Lee Revell Cc: Herbert Xu, Andrew Morton, linux-kernel, David S. Miller, linux-kernel, maxk, irda-users, Linux Network Development, Alain Schroeder 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-20 20:32 ` Denis Vlasenko @ 2004-10-21 0:15 ` David S. Miller 2004-10-21 0:35 ` Herbert Xu 0 siblings, 1 reply; 13+ messages in thread From: David S. Miller @ 2004-10-21 0:15 UTC (permalink / raw) To: Denis Vlasenko Cc: rlrevell, herbert, akpm, linux-kernel, linux-kernel, maxk, irda-users, netdev, alain On Wed, 20 Oct 2004 23:32:33 +0300 Denis Vlasenko <vda@port.imtp.ilyichevsk.odessa.ua> 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 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 <davem@davemloft.net> # # 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-21 0:15 ` David S. Miller @ 2004-10-21 0:35 ` Herbert Xu 2004-10-21 4:58 ` David S. Miller 0 siblings, 1 reply; 13+ messages in thread From: Herbert Xu @ 2004-10-21 0:35 UTC (permalink / raw) To: David S. Miller Cc: Denis Vlasenko, rlrevell, akpm, linux-kernel, linux-kernel, maxk, irda-users, netdev, alain 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~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Make netif_rx_ni preempt-safe 2004-10-21 0:35 ` Herbert Xu @ 2004-10-21 4:58 ` David S. Miller 0 siblings, 0 replies; 13+ messages in thread From: David S. Miller @ 2004-10-21 4:58 UTC (permalink / raw) To: Herbert Xu Cc: vda, rlrevell, akpm, linux-kernel, linux-kernel, maxk, irda-users, netdev, alain On Thu, 21 Oct 2004 10:35:03 +1000 Herbert Xu <herbert@gondor.apana.org.au> 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2004-10-21 4:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-10-19 23:55 [PATCH] Make netif_rx_ni preempt-safe Lee Revell 2004-10-20 0:00 ` Herbert Xu 2004-10-20 0:22 ` Lee Revell 2004-10-20 15:11 ` Denis Vlasenko 2004-10-20 16:47 ` Lee Revell 2004-10-20 19:14 ` Denis Vlasenko 2004-10-20 19:53 ` Lee Revell 2004-10-20 19:56 ` Denis Vlasenko 2004-10-20 20:25 ` Lee Revell 2004-10-20 20:32 ` Denis Vlasenko 2004-10-21 0:15 ` David S. Miller 2004-10-21 0:35 ` Herbert Xu 2004-10-21 4:58 ` David S. Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).