tun.c patch to fix "smp_processor_id() in preemptible code"

tun.c patch to fix "smp_processor_id() in preemptible code"

[Alain Schroeder]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Herbert Xu]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Herbert Xu]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Herbert Xu]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[David S. Miller]

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.

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[David S. Miller]

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.

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[Lee Revell]

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

Re: tun.c patch to fix "smp_processor_id() in preemptible code"

[David S. Miller]

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.