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

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

[John W. Linville]

"netpoll: Fix RCU usage" switched netpoll_rx to use the BH variant
of RCU.  Unfortunately, calling netpoll_rx from netif_rx resulted in
the following backtrace:

WARNING: at kernel/softirq.c:143 _local_bh_enable_ip+0x3e/0xab()
Hardware name: 2373HU6
Modules linked in: arc4 ecb lib80211_crypt_wep fuse nfsd lockd nfs_acl auth_rpcgss exportfs sunrpc cpufreq_ondemand acpi_cpufreq mperf ip6t_REJECT nf_conntrack_ipv6 ip6table_filter ip6_tables ipv6 dm_multipath uinput snd_intel8x0m snd_intel8x0 snd_ac97_codec ac97_bus ppdev snd_seq ipw2200 nsc_ircc snd_seq_device video irda e1000 parport_pc snd_pcm libipw parport output crc_ccitt thinkpad_acpi cfg80211 i2c_i801 joydev pcspkr iTCO_wdt rfkill iTCO_vendor_support lib80211 snd_timer snd soundcore snd_page_alloc yenta_socket radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core [last unloaded: microcode]
Pid: 0, comm: swapper Not tainted 2.6.35-wl+ #5
Call Trace:
 [<c043aa42>] warn_slowpath_common+0x6a/0x7f
 [<c0440541>] ? _local_bh_enable_ip+0x3e/0xab
 [<c072e8dc>] ? rcu_read_unlock_bh+0x21/0x23
 [<c043aa6b>] warn_slowpath_null+0x14/0x18
 [<c0440541>] _local_bh_enable_ip+0x3e/0xab
 [<c04405cd>] local_bh_enable+0x10/0x12
 [<c072e8dc>] rcu_read_unlock_bh+0x21/0x23
 [<c072e978>] netpoll_rx+0x9a/0xa2
 [<c0730ab9>] netif_rx+0x13/0x85
 [<f7cc02db>] libipw_rx+0x78c/0x7b6 [libipw]
 [<c07c6902>] ? _raw_spin_lock_irqsave+0x60/0x6a
 [<f81aedc9>] ipw_irq_tasklet+0xec3/0x1285 [ipw2200]
 [<c07c6f01>] ? _raw_spin_unlock_irq+0x26/0x30
 [<c046189a>] ? print_lock_contention_bug+0x11/0xb2
 [<c046189a>] ? print_lock_contention_bug+0x11/0xb2
 [<c043fd87>] tasklet_action+0x78/0xcb
 [<c0440293>] __do_softirq+0xc4/0x183
 [<c044038d>] do_softirq+0x3b/0x5f
 [<c04404d0>] irq_exit+0x3a/0x6d
 [<c0404423>] do_IRQ+0x8b/0x9f
 [<c04038b5>] common_interrupt+0x35/0x3c
 [<c062ecfa>] ? acpi_idle_enter_simple+0xfe/0x13c
 [<c045007b>] ? exit_itimers+0x2d/0x73
 [<c062ecfc>] ? acpi_idle_enter_simple+0x100/0x13c
 [<c070bf10>] cpuidle_idle_call+0x78/0xdc
 [<c040251c>] cpu_idle+0x9b/0xb7
 [<c07b1dd2>] rest_init+0xa6/0xab
 [<c0a4b96d>] start_kernel+0x389/0x38e
 [<c0a4b0c9>] i386_start_kernel+0xc9/0xd0

Switching back to the non-BH variant of RCU resolves the issue.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 include/linux/netpoll.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 413742c..0bdd527 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -63,8 +63,8 @@ static inline bool netpoll_rx(struct sk_buff *skb)
 	unsigned long flags;
 	bool ret = false;
 
-	rcu_read_lock_bh();
-	npinfo = rcu_dereference_bh(skb->dev->npinfo);
+	rcu_read_lock();
+	npinfo = rcu_dereference(skb->dev->npinfo);
 
 	if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
 		goto out;
@@ -76,13 +76,13 @@ static inline bool netpoll_rx(struct sk_buff *skb)
 	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 
 out:
-	rcu_read_unlock_bh();
+	rcu_read_unlock();
 	return ret;
 }
 
 static inline int netpoll_rx_on(struct sk_buff *skb)
 {
-	struct netpoll_info *npinfo = rcu_dereference_bh(skb->dev->npinfo);
+	struct netpoll_info *npinfo = rcu_dereference(skb->dev->npinfo);
 
 	return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
 }
-- 
1.7.2.1

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

[Herbert Xu]

On Tue, Aug 10, 2010 at 04:25:24PM -0400, John W. Linville wrote:
> "netpoll: Fix RCU usage" switched netpoll_rx to use the BH variant
> of RCU.  Unfortunately, calling netpoll_rx from netif_rx resulted in
> the following backtrace:

Thanks for catching this John!

> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index 413742c..0bdd527 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -63,8 +63,8 @@ static inline bool netpoll_rx(struct sk_buff *skb)
>  	unsigned long flags;
>  	bool ret = false;
>  
> -	rcu_read_lock_bh();
> -	npinfo = rcu_dereference_bh(skb->dev->npinfo);
> +	rcu_read_lock();
> +	npinfo = rcu_dereference(skb->dev->npinfo);

I really wanted to avoid mixing the two different RCU primitives
because they require different synchronisations.

In this case, the problem is that we're being called in IRQ
context, where BH is diabled anyway, so we don't actually need
to do anything (assuming IRQ is off).

Paul, what could we do to resolve this (other than by switching
to the non-BH variant of RCU)? Perhaps an additional variant
of rcu_read_lock_bh that checks whether IRQ is off?

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

[Paul E. McKenney]

On Tue, Aug 10, 2010 at 04:43:58PM -0400, Herbert Xu wrote:
> On Tue, Aug 10, 2010 at 04:25:24PM -0400, John W. Linville wrote:
> > "netpoll: Fix RCU usage" switched netpoll_rx to use the BH variant
> > of RCU.  Unfortunately, calling netpoll_rx from netif_rx resulted in
> > the following backtrace:
> 
> Thanks for catching this John!
> 
> > diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> > index 413742c..0bdd527 100644
> > --- a/include/linux/netpoll.h
> > +++ b/include/linux/netpoll.h
> > @@ -63,8 +63,8 @@ static inline bool netpoll_rx(struct sk_buff *skb)
> >  	unsigned long flags;
> >  	bool ret = false;
> >  
> > -	rcu_read_lock_bh();
> > -	npinfo = rcu_dereference_bh(skb->dev->npinfo);
> > +	rcu_read_lock();
> > +	npinfo = rcu_dereference(skb->dev->npinfo);
> 
> I really wanted to avoid mixing the two different RCU primitives
> because they require different synchronisations.
> 
> In this case, the problem is that we're being called in IRQ
> context, where BH is diabled anyway, so we don't actually need
> to do anything (assuming IRQ is off).
> 
> Paul, what could we do to resolve this (other than by switching
> to the non-BH variant of RCU)? Perhaps an additional variant
> of rcu_read_lock_bh that checks whether IRQ is off?

Hello, Herbert,

Your suggestion of providing another API rcu_read_lock_irqsoff()
and rcu_read_unlock_irqsoff() is the best I can think of right offhand.

What tree/commit do you need the patch against?

							Thanx, Paul

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

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Tue, 10 Aug 2010 14:19:32 -0700

> Your suggestion of providing another API rcu_read_lock_irqsoff()
> and rcu_read_unlock_irqsoff() is the best I can think of right offhand.
> 
> What tree/commit do you need the patch against?

Linus's tree is fine for this.

But we need this fix for 2.6.35-stable too.  Herbert would
you object to putting John's more simple fix there?

John, when referencing other commits in commit messages,
please provide the SHA1 ID as well as the commit message
header line.

Thanks.

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

[Herbert Xu]

On Tue, Aug 10, 2010 at 04:31:17PM -0700, David Miller wrote:
>
> Linus's tree is fine for this.
> 
> But we need this fix for 2.6.35-stable too.  Herbert would
> you object to putting John's more simple fix there?

To use rcu_read_lock safely we'd also need to add do synchronize_rcu
in addition of synchronize_rcu_bh, right Paul?

Of course as we were doing this unsafely prior to my patch anyway
I'm also fine with just reverting it.

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

[Herbert Xu]

On Wed, Aug 11, 2010 at 07:03:30AM -0400, Herbert Xu wrote:
>
> To use rcu_read_lock safely we'd also need to add do synchronize_rcu
> in addition of synchronize_rcu_bh, right Paul?
> 
> Of course as we were doing this unsafely prior to my patch anyway
> I'm also fine with just reverting it.

Actually, we could just disable IRQs for stable.  How about this
patch (untested)?

netpoll: Disable IRQ around RCU dereference in netpoll_rx

We cannot use rcu_dereference_bh safely in netpoll_rx as we may
be called with IRQs disabled.  We could however simply disable
IRQs as that too causes BH to be disabled and is safe in either
case.

Thanks to John Linville for discovering this bug and providing
a patch.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 413742c..54f286b 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -63,20 +63,20 @@ static inline bool netpoll_rx(struct sk_buff *skb)
 	unsigned long flags;
 	bool ret = false;
 
-	rcu_read_lock_bh();
+	local_irq_save(flags);
 	npinfo = rcu_dereference_bh(skb->dev->npinfo);
 
 	if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
 		goto out;
 
-	spin_lock_irqsave(&npinfo->rx_lock, flags);
+	spin_lock(&npinfo->rx_lock);
 	/* check rx_flags again with the lock held */
 	if (npinfo->rx_flags && __netpoll_rx(skb))
 		ret = true;
-	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+	spin_unlock(&npinfo->rx_lock);
 
 out:
-	rcu_read_unlock_bh();
+	local_irq_restore(flags);
 	return ret;
 }
 
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

[Paul E. McKenney]

On Wed, Aug 11, 2010 at 07:03:30AM -0400, Herbert Xu wrote:
> On Tue, Aug 10, 2010 at 04:31:17PM -0700, David Miller wrote:
> >
> > Linus's tree is fine for this.
> > 
> > But we need this fix for 2.6.35-stable too.  Herbert would
> > you object to putting John's more simple fix there?
> 
> To use rcu_read_lock safely we'd also need to add do synchronize_rcu
> in addition of synchronize_rcu_bh, right Paul?

That is true.

> Of course as we were doing this unsafely prior to my patch anyway
> I'm also fine with just reverting it.

That is true as well.

							Thanx, Paul

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

[John W. Linville]

On Wed, Aug 11, 2010 at 07:27:06AM -0400, Herbert Xu wrote:
> On Wed, Aug 11, 2010 at 07:03:30AM -0400, Herbert Xu wrote:
> >
> > To use rcu_read_lock safely we'd also need to add do synchronize_rcu
> > in addition of synchronize_rcu_bh, right Paul?
> > 
> > Of course as we were doing this unsafely prior to my patch anyway
> > I'm also fine with just reverting it.
> 
> Actually, we could just disable IRQs for stable.  How about this
> patch (untested)?
> 
> netpoll: Disable IRQ around RCU dereference in netpoll_rx
> 
> We cannot use rcu_dereference_bh safely in netpoll_rx as we may
> be called with IRQs disabled.  We could however simply disable
> IRQs as that too causes BH to be disabled and is safe in either
> case.
> 
> Thanks to John Linville for discovering this bug and providing
> a patch.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This also seems to work for me.

John

P.S.  As pointed-out elsewhere, my patch neglected to change
synchronize_rcu_bh to synchronize_rcu in netpoll.c.  If some fault
is found w/ Herbert's patch then I can post a modified version of mine.
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

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

[Paul E. McKenney]

On Tue, Aug 10, 2010 at 04:31:17PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Tue, 10 Aug 2010 14:19:32 -0700
> 
> > Your suggestion of providing another API rcu_read_lock_irqsoff()
> > and rcu_read_unlock_irqsoff() is the best I can think of right offhand.
> > 
> > What tree/commit do you need the patch against?
> 
> Linus's tree is fine for this.
> 
> But we need this fix for 2.6.35-stable too.  Herbert would
> you object to putting John's more simple fix there?
> 
> John, when referencing other commits in commit messages,
> please provide the SHA1 ID as well as the commit message
> header line.

And here is an interim patch.  I have queued a slightly different
patch that takes advantage of the more sophisticated PROVE_RCU handling
currently in -tip, but the API is the same, so code written against
this patch will work against the upcoming patch.

Thoughts?

							Thanx, Paul

commit 8934f3fd1092b7ef6a1524e1d2356baeec9f330e
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Wed Aug 11 14:54:33 2010 -0700

    rcu: add rcu_read_lock_bh_irqsoff() and rcu_read_unlock_bh_irqsoff()
    
    The rcu_read_lock_bh() and rcu_read_unlock_bh() functions can no
    longer be used when interrupts are disabled due to new debug checks in
    the _local_bh_enable() function.  This commit therefore supplies new
    functions that may only be called with either interrupts disabled or
    from interrupt handlers, and this is checked for under CONFIG_PROVE_RCU.
    
    Requested-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 9fbc54a..08cdc58 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -270,10 +270,13 @@ extern int rcu_my_thread_group_empty(void);
 		(p); \
 	})
 
+void rcu_read_unlock_bh_irqsoff_check(void);
+
 #else /* #ifdef CONFIG_PROVE_RCU */
 
 #define rcu_dereference_check(p, c)	rcu_dereference_raw(p)
 #define rcu_dereference_protected(p, c) (p)
+#define rcu_read_unlock_bh_irqsoff_check() do { } while (0)
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
 
@@ -361,13 +364,13 @@ static inline void rcu_read_unlock(void)
  */
 static inline void rcu_read_lock_bh(void)
 {
-	__rcu_read_lock_bh();
+	local_bh_disable();
 	__acquire(RCU_BH);
 	rcu_read_acquire_bh();
 }
 
 /*
- * rcu_read_unlock_bh - marks the end of a softirq-only RCU critical section
+ * rcu_read_unlock_bh() - marks the end of a softirq-only RCU critical section
  *
  * See rcu_read_lock_bh() for more information.
  */
@@ -375,7 +378,34 @@ static inline void rcu_read_unlock_bh(void)
 {
 	rcu_read_release_bh();
 	__release(RCU_BH);
-	__rcu_read_unlock_bh();
+	local_bh_enable();
+}
+
+/**
+ * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
+ *
+ * This is equivalent of rcu_read_lock_bh(), but to be used where the
+ * caller either is in an irq handler or has irqs disabled.  Note that
+ * this function assumes that PREEMPT_RT kernels run irq handlers at
+ * higher priority than softirq handlers!
+ */
+static inline void rcu_read_lock_bh_irqsoff(void)
+{
+	rcu_read_unlock_bh_irqsoff_check();
+	__acquire(RCU_BH);
+	rcu_read_acquire_bh();
+}
+
+/*
+ * rcu_read_unlock_bh_irqsoff - marks the end of an RCU-bh critical section
+ *
+ * See rcu_read_lock_bh_irqsoff() for more information.
+ */
+static inline void rcu_read_unlock_bh_irqsoff(void)
+{
+	rcu_read_release_bh();
+	__release(RCU_BH);
+	rcu_read_unlock_bh_irqsoff_check();
 }
 
 /**
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index e2e8931..009c7f3 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -36,8 +36,6 @@ static inline void rcu_note_context_switch(int cpu)
 
 #define __rcu_read_lock()	preempt_disable()
 #define __rcu_read_unlock()	preempt_enable()
-#define __rcu_read_lock_bh()	local_bh_disable()
-#define __rcu_read_unlock_bh()	local_bh_enable()
 #define call_rcu_sched		call_rcu
 
 #define rcu_init_sched()	do { } while (0)
diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
index c0ed1c0..98b50d8 100644
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -75,15 +75,6 @@ static inline int rcu_preempt_depth(void)
 
 #endif /* #else #ifdef CONFIG_TREE_PREEMPT_RCU */
 
-static inline void __rcu_read_lock_bh(void)
-{
-	local_bh_disable();
-}
-static inline void __rcu_read_unlock_bh(void)
-{
-	local_bh_enable();
-}
-
 extern void call_rcu_sched(struct rcu_head *head,
 			   void (*func)(struct rcu_head *rcu));
 extern void synchronize_rcu_bh(void);
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 4d16983..d159835 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -113,6 +113,12 @@ int rcu_my_thread_group_empty(void)
 	return thread_group_empty(current);
 }
 EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
+
+void rcu_read_unlock_bh_irqsoff_check(void)
+{
+	WARN_ON_ONCE(in_irq() || irqs_disabled());
+}
+EXPORT_SYMBOL_GPL(rcu_read_unlock_bh_irqsoff_check);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
 #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD

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

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Wed, 11 Aug 2010 15:00:47 -0700

> @@ -113,6 +113,12 @@ int rcu_my_thread_group_empty(void)
>  	return thread_group_empty(current);
>  }
>  EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
> +
> +void rcu_read_unlock_bh_irqsoff_check(void)
> +{
> +	WARN_ON_ONCE(in_irq() || irqs_disabled());
> +}
> +EXPORT_SYMBOL_GPL(rcu_read_unlock_bh_irqsoff_check);
>  #endif /* #ifdef CONFIG_PROVE_RCU */

Is this WARN_ON_ONCE() test inverted?  It seems to be called where we
"should be" in an IRQ or have IRQs disabled.

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

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 11 Aug 2010 11:37:36 -0400

> On Wed, Aug 11, 2010 at 07:27:06AM -0400, Herbert Xu wrote:
>> On Wed, Aug 11, 2010 at 07:03:30AM -0400, Herbert Xu wrote:
>> >
>> > To use rcu_read_lock safely we'd also need to add do synchronize_rcu
>> > in addition of synchronize_rcu_bh, right Paul?
>> > 
>> > Of course as we were doing this unsafely prior to my patch anyway
>> > I'm also fine with just reverting it.
>> 
>> Actually, we could just disable IRQs for stable.  How about this
>> patch (untested)?
>> 
>> netpoll: Disable IRQ around RCU dereference in netpoll_rx
>> 
>> We cannot use rcu_dereference_bh safely in netpoll_rx as we may
>> be called with IRQs disabled.  We could however simply disable
>> IRQs as that too causes BH to be disabled and is safe in either
>> case.
>> 
>> Thanks to John Linville for discovering this bug and providing
>> a patch.
>> 
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> This also seems to work for me.

I'll queue up this variant for -stable.

And we'll go with making use of Paul's new RCU irqsoff interface
approach for Linus's tree.

Thanks.

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

[Paul E. McKenney]

On Wed, Aug 11, 2010 at 11:09:36PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Wed, 11 Aug 2010 15:00:47 -0700
> 
> > @@ -113,6 +113,12 @@ int rcu_my_thread_group_empty(void)
> >  	return thread_group_empty(current);
> >  }
> >  EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
> > +
> > +void rcu_read_unlock_bh_irqsoff_check(void)
> > +{
> > +	WARN_ON_ONCE(in_irq() || irqs_disabled());
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_read_unlock_bh_irqsoff_check);
> >  #endif /* #ifdef CONFIG_PROVE_RCU */
> 
> Is this WARN_ON_ONCE() test inverted?  It seems to be called where we
> "should be" in an IRQ or have IRQs disabled.

You are quite correct.  (Beat head against wall.)  :-/

------------------------------------------------------------------------

#!/bin/bash

for ((i=0;i<100;i++))
do
	echo "De Morgan when converting rcu_lockdep_assert() to WARN_ON_ONCE()"
done

------------------------------------------------------------------------

This does sort of defeat the purpose of writing something 100 times, but 
I am after all a software developer!!!

Thank you very much for catching this, and please see below for a
replacement patch.

							Thanx, Paul


commit 2c9ace45088a25b474167d04b416d279f4ea3401
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Wed Aug 11 14:54:33 2010 -0700

    rcu: add rcu_read_lock_bh_irqsoff() and rcu_read_unlock_bh_irqsoff()
    
    The rcu_read_lock_bh() and rcu_read_unlock_bh() functions can no
    longer be used when interrupts are disabled due to new debug checks in
    the _local_bh_enable() function.  This commit therefore supplies new
    functions that may only be called with either interrupts disabled or
    from interrupt handlers, and this is checked for under CONFIG_PROVE_RCU.
    
    Requested-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 9fbc54a..08cdc58 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -270,10 +270,13 @@ extern int rcu_my_thread_group_empty(void);
 		(p); \
 	})
 
+void rcu_read_unlock_bh_irqsoff_check(void);
+
 #else /* #ifdef CONFIG_PROVE_RCU */
 
 #define rcu_dereference_check(p, c)	rcu_dereference_raw(p)
 #define rcu_dereference_protected(p, c) (p)
+#define rcu_read_unlock_bh_irqsoff_check() do { } while (0)
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
 
@@ -361,13 +364,13 @@ static inline void rcu_read_unlock(void)
  */
 static inline void rcu_read_lock_bh(void)
 {
-	__rcu_read_lock_bh();
+	local_bh_disable();
 	__acquire(RCU_BH);
 	rcu_read_acquire_bh();
 }
 
 /*
- * rcu_read_unlock_bh - marks the end of a softirq-only RCU critical section
+ * rcu_read_unlock_bh() - marks the end of a softirq-only RCU critical section
  *
  * See rcu_read_lock_bh() for more information.
  */
@@ -375,7 +378,34 @@ static inline void rcu_read_unlock_bh(void)
 {
 	rcu_read_release_bh();
 	__release(RCU_BH);
-	__rcu_read_unlock_bh();
+	local_bh_enable();
+}
+
+/**
+ * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
+ *
+ * This is equivalent of rcu_read_lock_bh(), but to be used where the
+ * caller either is in an irq handler or has irqs disabled.  Note that
+ * this function assumes that PREEMPT_RT kernels run irq handlers at
+ * higher priority than softirq handlers!
+ */
+static inline void rcu_read_lock_bh_irqsoff(void)
+{
+	rcu_read_unlock_bh_irqsoff_check();
+	__acquire(RCU_BH);
+	rcu_read_acquire_bh();
+}
+
+/*
+ * rcu_read_unlock_bh_irqsoff - marks the end of an RCU-bh critical section
+ *
+ * See rcu_read_lock_bh_irqsoff() for more information.
+ */
+static inline void rcu_read_unlock_bh_irqsoff(void)
+{
+	rcu_read_release_bh();
+	__release(RCU_BH);
+	rcu_read_unlock_bh_irqsoff_check();
 }
 
 /**
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index e2e8931..009c7f3 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -36,8 +36,6 @@ static inline void rcu_note_context_switch(int cpu)
 
 #define __rcu_read_lock()	preempt_disable()
 #define __rcu_read_unlock()	preempt_enable()
-#define __rcu_read_lock_bh()	local_bh_disable()
-#define __rcu_read_unlock_bh()	local_bh_enable()
 #define call_rcu_sched		call_rcu
 
 #define rcu_init_sched()	do { } while (0)
diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
index c0ed1c0..98b50d8 100644
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -75,15 +75,6 @@ static inline int rcu_preempt_depth(void)
 
 #endif /* #else #ifdef CONFIG_TREE_PREEMPT_RCU */
 
-static inline void __rcu_read_lock_bh(void)
-{
-	local_bh_disable();
-}
-static inline void __rcu_read_unlock_bh(void)
-{
-	local_bh_enable();
-}
-
 extern void call_rcu_sched(struct rcu_head *head,
 			   void (*func)(struct rcu_head *rcu));
 extern void synchronize_rcu_bh(void);
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 4d16983..ae6ae40 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -113,6 +113,12 @@ int rcu_my_thread_group_empty(void)
 	return thread_group_empty(current);
 }
 EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
+
+void rcu_read_unlock_bh_irqsoff_check(void)
+{
+	WARN_ON_ONCE(!in_irq() && !irqs_disabled());
+}
+EXPORT_SYMBOL_GPL(rcu_read_unlock_bh_irqsoff_check);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
 #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD

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

[Herbert Xu]

On Thu, Aug 12, 2010 at 08:42:13AM -0700, Paul E. McKenney wrote:
>
> +/**
> + * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
> + *
> + * This is equivalent of rcu_read_lock_bh(), but to be used where the
> + * caller either is in an irq handler or has irqs disabled.  Note that
> + * this function assumes that PREEMPT_RT kernels run irq handlers at
> + * higher priority than softirq handlers!
> + */
> +static inline void rcu_read_lock_bh_irqsoff(void)
> +{
> +	rcu_read_unlock_bh_irqsoff_check();
> +	__acquire(RCU_BH);
> +	rcu_read_acquire_bh();
> +}

Thanks for the patch Paul!

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.

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

[Paul E. McKenney]

On Fri, Aug 13, 2010 at 10:39:04AM -0400, Herbert Xu wrote:
> On Thu, Aug 12, 2010 at 08:42:13AM -0700, Paul E. McKenney wrote:
> >
> > +/**
> > + * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
> > + *
> > + * This is equivalent of rcu_read_lock_bh(), but to be used where the
> > + * caller either is in an irq handler or has irqs disabled.  Note that
> > + * this function assumes that PREEMPT_RT kernels run irq handlers at
> > + * higher priority than softirq handlers!
> > + */
> > +static inline void rcu_read_lock_bh_irqsoff(void)
> > +{
> > +	rcu_read_unlock_bh_irqsoff_check();
> > +	__acquire(RCU_BH);
> > +	rcu_read_acquire_bh();
> > +}
> 
> Thanks for the patch Paul!
> 
> 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:

static inline void rcu_read_lock_bh_irqsoff(void)
{
	if (!in_irq() && !irqs_disabled())
		local_bh_disable();
	__acquire(RCU_BH);
	rcu_read_acquire_bh();
}

static inline void rcu_read_unlock_bh_irqsoff(void)
{
	rcu_read_release_bh();
	__release(RCU_BH);
	if (!in_irq() && !irqs_disabled())
		local_bh_enable();
}

If the state can change in the RCU-bh read-side critical section, then
I would have to record the state in the task structure or some such.

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?  ;-)

							Thanx, Paul

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

[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

[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

[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

[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

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