* tun.c patch to fix "smp_processor_id() in preemptible code"
@ 2004-10-15 21:43 Alain Schroeder
2004-10-15 22:22 ` Herbert Xu
2004-10-15 22:35 ` Lee Revell
0 siblings, 2 replies; 13+ messages in thread
From: Alain Schroeder @ 2004-10-15 21:43 UTC (permalink / raw)
To: linux-kernel; +Cc: Jan-Benedict Glaw
[-- Attachment #1: Type: text/plain, Size: 781 bytes --]
I was getting these traces on a SMP host:
dsor-vm2 kernel: using smp_processor_id() in preemptible code: linux/480
dsor-vm2 kernel: [smp_processor_id+108/132] smp_processor_id+0x6c/0x84
kernel: [pg0+945156838/1070318592] tun_chr_writev+0x14e/0x174 [tun]
kernel: [pg0+945156917/1070318592] tun_chr_write+0x29/0x30 [tun]
kernel: [vfs_write+189/236] vfs_write+0xbd/0xec
kernel: [sys_write+64/108] sys_write+0x40/0x6c
kernel: [syscall_call+7/11] syscall_call+0x7/0xb
The (very) little attached patch fixes this.
Bye,
Alain
PS: I am not subscribed to the lkml.
--
"My grandfather once told me that there are two kinds of people: those
who work and those who take the credit. He told me to try to be in the
first group; there was less competition there." -- Indira Gandhi
[-- Attachment #2: tun-preempt.patch --]
[-- Type: text/x-patch, Size: 358 bytes --]
--- linux-2.6.8-rc2/drivers/net/tun.c Wed Jun 16 05:19:22 2004
+++ linux-2.6.9-rc4-mm1-skas/drivers/net/tun.c Fri Oct 15 21:16:18 2004
@@ -207,7 +207,9 @@
if (tun->flags & TUN_NOCHECKSUM)
skb->ip_summed = CHECKSUM_UNNECESSARY;
+ preempt_disable();
netif_rx_ni(skb);
+ preempt_enable();
tun->stats.rx_packets++;
tun->stats.rx_bytes += len;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-15 21:43 tun.c patch to fix "smp_processor_id() in preemptible code" Alain Schroeder
@ 2004-10-15 22:22 ` Herbert Xu
2004-10-15 22:35 ` Lee Revell
1 sibling, 0 replies; 13+ messages in thread
From: Herbert Xu @ 2004-10-15 22:22 UTC (permalink / raw)
To: Alain Schroeder; +Cc: linux-kernel, jbglaw
Alain Schroeder <alain@parkautomat.net> wrote:
>
> The (very) little attached patch fixes this.
Your change should be moved into netif_rx_ni itself.
--
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: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-15 21:43 tun.c patch to fix "smp_processor_id() in preemptible code" Alain Schroeder
2004-10-15 22:22 ` Herbert Xu
@ 2004-10-15 22:35 ` Lee Revell
[not found] ` <200410172314.38597.vda@port.imtp.ilyichevsk.odessa.ua>
1 sibling, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-15 22:35 UTC (permalink / raw)
To: Alain Schroeder; +Cc: linux-kernel, Jan-Benedict Glaw
On Fri, 2004-10-15 at 17:43, Alain Schroeder wrote:
> I was getting these traces on a SMP host:
Your patch:
+ preempt_disable();
netif_rx_ni(skb);
+ preempt_enable();
just wraps this code in preempt_disable/enable:
static inline int netif_rx_ni(struct sk_buff *skb)
{
int err = netif_rx(skb);
if (softirq_pending(smp_processor_id()))
do_softirq();
return err;
}
Isn't this considered an incorrect use of preempt_disable/enable? My
reasoning is that if this was correct we would see preempt_dis/enable
sprinkled all over the code which it isn't.
Why do you have to call do_softirq like that? I was under the
impression that you raise a softirq and it gets run later.
Lee
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
[not found] ` <200410172314.38597.vda@port.imtp.ilyichevsk.odessa.ua>
@ 2004-10-19 18:31 ` Lee Revell
2004-10-19 21:35 ` Herbert Xu
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 18:31 UTC (permalink / raw)
To: Denis Vlasenko; +Cc: Linux Network Development, linux-kernel
On Sun, 2004-10-17 at 16:14, Denis Vlasenko wrote:
> > Your patch:
> >
> > + preempt_disable();
> > netif_rx_ni(skb);
> > + preempt_enable();
> >
> > just wraps this code in preempt_disable/enable:
> >
> > static inline int netif_rx_ni(struct sk_buff *skb)
> > {
> > int err = netif_rx(skb);
> > if (softirq_pending(smp_processor_id()))
> > do_softirq();
> > return err;
> > }
> >
> > Isn't this considered an incorrect use of preempt_disable/enable? My
> > reasoning is that if this was correct we would see preempt_dis/enable
> > sprinkled all over the code which it isn't.
> >
> > Why do you have to call do_softirq like that? I was under the
> > impression that you raise a softirq and it gets run later.
>
> There is a possibility that this guy just wanted to fix his
> small problem.
>
Yes, that is what I thought. The question was more directed at the
list. I added netdev to the cc:.
I looked at Robert Love's book and I am still unclear on the use of
do_softirq above. To reiterate the question: why does netif_rx_ni have
to manually flush any pending softirqs on the current proccessor after
doing the rx? Is this just a performance hack?
Lee
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 18:31 ` Lee Revell
@ 2004-10-19 21:35 ` Herbert Xu
2004-10-19 21:51 ` Lee Revell
0 siblings, 1 reply; 13+ messages in thread
From: Herbert Xu @ 2004-10-19 21:35 UTC (permalink / raw)
To: Lee Revell; +Cc: vda, netdev, linux-kernel
Lee Revell <rlrevell@joe-job.com> wrote:
>
> I looked at Robert Love's book and I am still unclear on the use of
> do_softirq above. To reiterate the question: why does netif_rx_ni have
> to manually flush any pending softirqs on the current proccessor after
> doing the rx? Is this just a performance hack?
Yes it allows the packet to be processed immediately.
--
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: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 21:35 ` Herbert Xu
@ 2004-10-19 21:51 ` Lee Revell
2004-10-19 21:54 ` Herbert Xu
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 21:51 UTC (permalink / raw)
To: Herbert Xu; +Cc: vda, Linux Network Development, linux-kernel
On Tue, 2004-10-19 at 17:35, Herbert Xu wrote:
> Lee Revell <rlrevell@joe-job.com> wrote:
> >
> > I looked at Robert Love's book and I am still unclear on the use of
> > do_softirq above. To reiterate the question: why does netif_rx_ni have
> > to manually flush any pending softirqs on the current proccessor after
> > doing the rx? Is this just a performance hack?
>
> Yes it allows the packet to be processed immediately.
Ok, here is the correct patch. If this is really just a matter of
performance, and not required for correctness, disabling preemption is
broken, right?
Lee
--- include/linux/netdevice.h~ 2004-10-15 20:19:33.000000000 -0400
+++ include/linux/netdevice.h 2004-10-19 17:47:03.000000000 -0400
@@ -697,8 +697,6 @@
static inline int netif_rx_ni(struct sk_buff *skb)
{
int err = netif_rx(skb);
- if (softirq_pending(smp_processor_id()))
- do_softirq();
return err;
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 21:51 ` Lee Revell
@ 2004-10-19 21:54 ` Herbert Xu
2004-10-19 22:10 ` Lee Revell
0 siblings, 1 reply; 13+ messages in thread
From: Herbert Xu @ 2004-10-19 21:54 UTC (permalink / raw)
To: Lee Revell; +Cc: vda, Linux Network Development, linux-kernel
On Tue, Oct 19, 2004 at 05:51:17PM -0400, Lee Revell wrote:
>
> Ok, here is the correct patch. If this is really just a matter of
> performance, and not required for correctness, disabling preemption is
> broken, right?
No if you're doing this then you should get rid of netif_rx_ni()
altogether. But before you do that please ask all the people who
call it.
--
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: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 21:54 ` Herbert Xu
@ 2004-10-19 22:10 ` Lee Revell
2004-10-19 22:33 ` David S. Miller
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 22:10 UTC (permalink / raw)
To: Herbert Xu; +Cc: vda, Linux Network Development, linux-kernel, maxk, irda-users
On Tue, 2004-10-19 at 17:54, Herbert Xu wrote:
> On Tue, Oct 19, 2004 at 05:51:17PM -0400, Lee Revell wrote:
> >
> > Ok, here is the correct patch. If this is really just a matter of
> > performance, and not required for correctness, disabling preemption is
> > broken, right?
>
> No if you're doing this then you should get rid of netif_rx_ni()
> altogether. But before you do that please ask all the people who
> call it.
There are not a lot of them:
drivers/s390/net/ctcmain.c
drivers/s390/net/netiucv.c
drivers/net/irda/vlsi_ir.c
drivers/net/tun.c
>From netiuvc.c:
/*
* Since receiving is always initiated from a tasklet (in iucv.c),
* we must use netif_rx_ni() instead of netif_rx()
*/
This implies that the author thought it was a matter of correctness to
use netif_rx_ni vs. netif_rx. But it looks like the only difference is
that the former sacrifices preempt-safety for performance.
I could not find maintainers for the two s390 drivers, or a specific
maintainer for vlsi_ir.
Lee
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 22:10 ` Lee Revell
@ 2004-10-19 22:33 ` David S. Miller
2004-10-19 22:42 ` Lee Revell
0 siblings, 1 reply; 13+ messages in thread
From: David S. Miller @ 2004-10-19 22:33 UTC (permalink / raw)
To: Lee Revell; +Cc: herbert, vda, netdev, linux-kernel, maxk, irda-users
On Tue, 19 Oct 2004 18:10:58 -0400
Lee Revell <rlrevell@joe-job.com> wrote:
> /*
> * Since receiving is always initiated from a tasklet (in iucv.c),
> * we must use netif_rx_ni() instead of netif_rx()
> */
>
> This implies that the author thought it was a matter of correctness to
> use netif_rx_ni vs. netif_rx. But it looks like the only difference is
> that the former sacrifices preempt-safety for performance.
You can't really delete netif_rx_ni(), so if there is a preemptability
issue, just add the necessary preemption protection around the softirq
checks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 22:33 ` David S. Miller
@ 2004-10-19 22:42 ` Lee Revell
2004-10-19 22:42 ` David S. Miller
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 22:42 UTC (permalink / raw)
To: David S. Miller
Cc: herbert, vda, Linux Network Development, linux-kernel, maxk, irda-users
On Tue, 2004-10-19 at 18:33, David S. Miller wrote:
> On Tue, 19 Oct 2004 18:10:58 -0400
> Lee Revell <rlrevell@joe-job.com> wrote:
>
> > /*
> > * Since receiving is always initiated from a tasklet (in iucv.c),
> > * we must use netif_rx_ni() instead of netif_rx()
> > */
> >
> > This implies that the author thought it was a matter of correctness to
> > use netif_rx_ni vs. netif_rx. But it looks like the only difference is
> > that the former sacrifices preempt-safety for performance.
>
> You can't really delete netif_rx_ni(), so if there is a preemptability
> issue, just add the necessary preemption protection around the softirq
> checks.
>
Why not? AIUI the only valid reason to use preempt_disable/enable is in
the case of per-CPU data. This is not "real" per-CPU data, it's a
performance hack. Therefore it would be incorrect to add the preemption
protection, the fix is not to manually call do_softirq but to let the
softirq run by the normal mechanism.
Am I missing something?
Lee
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 22:42 ` Lee Revell
@ 2004-10-19 22:42 ` David S. Miller
2004-10-19 22:51 ` Lee Revell
0 siblings, 1 reply; 13+ messages in thread
From: David S. Miller @ 2004-10-19 22:42 UTC (permalink / raw)
To: Lee Revell; +Cc: herbert, vda, netdev, linux-kernel, maxk, irda-users
On Tue, 19 Oct 2004 18:42:11 -0400
Lee Revell <rlrevell@joe-job.com> wrote:
> On Tue, 2004-10-19 at 18:33, David S. Miller wrote:
> > On Tue, 19 Oct 2004 18:10:58 -0400
> > Lee Revell <rlrevell@joe-job.com> wrote:
> >
> > > /*
> > > * Since receiving is always initiated from a tasklet (in iucv.c),
> > > * we must use netif_rx_ni() instead of netif_rx()
> > > */
> > >
> > > This implies that the author thought it was a matter of correctness to
> > > use netif_rx_ni vs. netif_rx. But it looks like the only difference is
> > > that the former sacrifices preempt-safety for performance.
> >
> > You can't really delete netif_rx_ni(), so if there is a preemptability
> > issue, just add the necessary preemption protection around the softirq
> > checks.
> >
>
> Why not? AIUI the only valid reason to use preempt_disable/enable is in
> the case of per-CPU data. This is not "real" per-CPU data, it's a
> performance hack. Therefore it would be incorrect to add the preemption
> protection, the fix is not to manually call do_softirq but to let the
> softirq run by the normal mechanism.
>
> Am I missing something?
In code paths where netif_rx_ni() is called, there is not a softirq return
path check, which is why it is checked here.
Theoretically, if you remove the check, softirq processing can be deferred
indefinitely.
What I'm saying, therefore, is that netif_rx_ni() it not just a performance
hack, it is necessary for correctness as well.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 22:42 ` David S. Miller
@ 2004-10-19 22:51 ` Lee Revell
2004-10-20 0:44 ` David S. Miller
0 siblings, 1 reply; 13+ messages in thread
From: Lee Revell @ 2004-10-19 22:51 UTC (permalink / raw)
To: David S. Miller
Cc: herbert, vda, Linux Network Development, linux-kernel, maxk, irda-users
On Tue, 2004-10-19 at 18:42, David S. Miller wrote:
> AIUI the only valid reason to use preempt_disable/enable is in
> > the case of per-CPU data. This is not "real" per-CPU data, it's a
> > performance hack. Therefore it would be incorrect to add the preemption
> > protection, the fix is not to manually call do_softirq but to let the
> > softirq run by the normal mechanism.
> >
> > Am I missing something?
>
> In code paths where netif_rx_ni() is called, there is not a softirq return
> path check, which is why it is checked here.
>
> Theoretically, if you remove the check, softirq processing can be deferred
> indefinitely.
>
> What I'm saying, therefore, is that netif_rx_ni() it not just a performance
> hack, it is necessary for correctness as well.
>
OK, thanks for clarifying. The correct patch is therefore:
--- 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;
}
Lee
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: tun.c patch to fix "smp_processor_id() in preemptible code"
2004-10-19 22:51 ` Lee Revell
@ 2004-10-20 0:44 ` David S. Miller
0 siblings, 0 replies; 13+ messages in thread
From: David S. Miller @ 2004-10-20 0:44 UTC (permalink / raw)
To: Lee Revell; +Cc: herbert, vda, netdev, linux-kernel, maxk, irda-users
On Tue, 19 Oct 2004 18:51:28 -0400
Lee Revell <rlrevell@joe-job.com> wrote:
> OK, thanks for clarifying. The correct patch is therefore:
...
> static inline int netif_rx_ni(struct sk_buff *skb)
> {
> + preempt_disable();
> int err = netif_rx(skb);
You need to put statements after local function variable
declarations.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2004-10-20 10:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-10-15 21:43 tun.c patch to fix "smp_processor_id() in preemptible code" Alain Schroeder
2004-10-15 22:22 ` Herbert Xu
2004-10-15 22:35 ` Lee Revell
[not found] ` <200410172314.38597.vda@port.imtp.ilyichevsk.odessa.ua>
2004-10-19 18:31 ` Lee Revell
2004-10-19 21:35 ` Herbert Xu
2004-10-19 21:51 ` Lee Revell
2004-10-19 21:54 ` Herbert Xu
2004-10-19 22:10 ` Lee Revell
2004-10-19 22:33 ` David S. Miller
2004-10-19 22:42 ` Lee Revell
2004-10-19 22:42 ` David S. Miller
2004-10-19 22:51 ` Lee Revell
2004-10-20 0:44 ` 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).