Re: [PATCH] netpoll: use non-BH variant of RCU

Re: [PATCH] netpoll: use non-BH variant of RCU

[Herbert Xu]

On Fri, Aug 13, 2010 at 09:29:12AM -0700, Paul E. McKenney wrote:
>
> > But this doesn't really solve the problem for netif_rx.  The reason
> > is that netif_rx can either be called with IRQs on OR off.  So we
> > need to take the right precautions in the case where IRQs are
> > enabled along with BH.
> 
> Interesting...
> 
> Is it possible that IRQs are off at rcu_read_lock_bh_irqsoff() time, but
> enabled by the time we get to rcu_read_unlock_bh_irqsoff()?  I hope not,
> but have to ask.  If I am guaranteed of the same state in both cases,
> I can do something like the following:

Yes in our case it's certainly guaranteed that IRQs will remain
off.
 
> But all in all, mightn't it be easier to remove the checks from
> _local_bh_enable(), and then just use rcu_read_lock_bh()?  Have those
> checks really been that helpful in finding bugs?  ;-)

You are right.  It would be much simpler to simply have it not
warn.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] netpoll: use non-BH variant of RCU

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 13 Aug 2010 13:51:57 -0400

> On Fri, Aug 13, 2010 at 09:29:12AM -0700, Paul E. McKenney wrote:
>> But all in all, mightn't it be easier to remove the checks from
>> _local_bh_enable(), and then just use rcu_read_lock_bh()?  Have those
>> checks really been that helpful in finding bugs?  ;-)
> 
> You are right.  It would be much simpler to simply have it not
> warn.

For now I'm going to assume that this is how the issue will be
addressed.

Re: [PATCH] netpoll: use non-BH variant of RCU

[Paul E. McKenney]

On Sat, Aug 14, 2010 at 11:50:33PM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 13 Aug 2010 13:51:57 -0400
> 
> > On Fri, Aug 13, 2010 at 09:29:12AM -0700, Paul E. McKenney wrote:
> >> But all in all, mightn't it be easier to remove the checks from
> >> _local_bh_enable(), and then just use rcu_read_lock_bh()?  Have those
> >> checks really been that helpful in finding bugs?  ;-)
> > 
> > You are right.  It would be much simpler to simply have it not
> > warn.
> 
> For now I'm going to assume that this is how the issue will be
> addressed.

And here is the patch, at long last.  Adding Ingo and Peter Z to CC
in case there is some problem with this approach that I cannot see.

							Thanx, Paul

commit 095d050d5d04344d3cdd4ff8558ea8709e3c4beb
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Thu Sep 2 10:07:21 2010 -0700

    softirq: adjust error check
    
    The error check for _local_bh_enable_ip() warns on both in_irq() and
    irqs_disabled().  The check for in_irq() is necessary, because a hardirq
    might well have interrupted bh execution, in which case it is simply
    not possible for the hardirq to exclude the bh that got interrupted.
    
    However, it is perfectly reasonable to disable bh while hardirqs are
    disabled, and this in fact promotes common code.  This commit therefore
    removes the irqs_disabled() check.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 07b4f1b..3cff1bb 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -140,7 +140,7 @@ EXPORT_SYMBOL(_local_bh_enable);
 
 static inline void _local_bh_enable_ip(unsigned long ip)
 {
-	WARN_ON_ONCE(in_irq() || irqs_disabled());
+	WARN_ON_ONCE(in_irq());
 #ifdef CONFIG_TRACE_IRQFLAGS
 	local_irq_disable();
 #endif

Re: [PATCH] netpoll: use non-BH variant of RCU

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Thu, 2 Sep 2010 10:26:25 -0700

>     softirq: adjust error check
>     
>     The error check for _local_bh_enable_ip() warns on both in_irq() and
>     irqs_disabled().  The check for in_irq() is necessary, because a hardirq
>     might well have interrupted bh execution, in which case it is simply
>     not possible for the hardirq to exclude the bh that got interrupted.
>     
>     However, it is perfectly reasonable to disable bh while hardirqs are
>     disabled, and this in fact promotes common code.  This commit therefore
>     removes the irqs_disabled() check.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] netpoll: use non-BH variant of RCU

[Paul E. McKenney]

On Fri, Sep 03, 2010 at 08:34:09AM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Thu, 2 Sep 2010 10:26:25 -0700
> 
> >     softirq: adjust error check
> >     
> >     The error check for _local_bh_enable_ip() warns on both in_irq() and
> >     irqs_disabled().  The check for in_irq() is necessary, because a hardirq
> >     might well have interrupted bh execution, in which case it is simply
> >     not possible for the hardirq to exclude the bh that got interrupted.
> >     
> >     However, it is perfectly reasonable to disable bh while hardirqs are
> >     disabled, and this in fact promotes common code.  This commit therefore
> >     removes the irqs_disabled() check.
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Thank you, David!

							Thanx, Paul