Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH -tip] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections

21 views
Skip to first unread message

Lai Jiangshan

unread,
Mar 16, 2010, 7:10:02 AM3/16/10
to
It is documented that local_irq_disable() also delimits
RCU_SCHED read-site critical sections.
See the document of synchronize_sched() or
Documentation/RCU/whatisRCU.txt.

So we have to test irqs_disabled() in rcu_read_lock_sched_held().
Otherwise rcu-lockdep brings incorrect complaint.

Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com>
---
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 3024050..2ce5674 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -160,7 +160,7 @@ static inline int rcu_read_lock_sched_held(void)
return 1;
if (debug_locks)
lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || preempt_count() != 0;
+ return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)
@@ -191,7 +191,7 @@ static inline int rcu_read_lock_bh_held(void)
#ifdef CONFIG_PREEMPT
static inline int rcu_read_lock_sched_held(void)
{
- return !rcu_scheduler_active || preempt_count() != 0;
+ return !rcu_scheduler_active || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Ingo Molnar

unread,
Mar 16, 2010, 7:30:01 AM3/16/10
to

* Lai Jiangshan <la...@cn.fujitsu.com> wrote:

> It is documented that local_irq_disable() also delimits
> RCU_SCHED read-site critical sections.
> See the document of synchronize_sched() or
> Documentation/RCU/whatisRCU.txt.
>
> So we have to test irqs_disabled() in rcu_read_lock_sched_held().
> Otherwise rcu-lockdep brings incorrect complaint.

It would be useful to include the warning in question in the changelog - so
that others who might be affected by it can see the fix and can track its
progress. (and dont start a parallel effort debugging/reporting/fixing it)

Thanks,

Ingo

Paul E. McKenney

unread,
Mar 16, 2010, 9:30:02 AM3/16/10
to
On Tue, Mar 16, 2010 at 07:09:21PM +0800, Lai Jiangshan wrote:
> It is documented that local_irq_disable() also delimits
> RCU_SCHED read-site critical sections.
> See the document of synchronize_sched() or
> Documentation/RCU/whatisRCU.txt.
>
> So we have to test irqs_disabled() in rcu_read_lock_sched_held().
> Otherwise rcu-lockdep brings incorrect complaint.

Interesting -- I was under the impression that preempt_count() covered
this as well, due to the following in include/linux/hardirq.h:

#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)

But irqs_disabled() does look to sample the actual interrupt hardware.

So, if there are cases where RCU is used where the interrupt hardware
is disabled, but preempt_count() has not been updated, this patch is
the right thing to do.

Thanx, Paul

Lai Jiangshan

unread,
Mar 16, 2010, 9:50:02 PM3/16/10
to
Paul E. McKenney wrote:
> On Tue, Mar 16, 2010 at 07:09:21PM +0800, Lai Jiangshan wrote:
>> It is documented that local_irq_disable() also delimits
>> RCU_SCHED read-site critical sections.
>> See the document of synchronize_sched() or
>> Documentation/RCU/whatisRCU.txt.
>>
>> So we have to test irqs_disabled() in rcu_read_lock_sched_held().
>> Otherwise rcu-lockdep brings incorrect complaint.
>
> Interesting -- I was under the impression that preempt_count() covered
> this as well, due to the following in include/linux/hardirq.h:
>
> #define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> #define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
>
> But irqs_disabled() does look to sample the actual interrupt hardware.
>
> So, if there are cases where RCU is used where the interrupt hardware
> is disabled, but preempt_count() has not been updated, this patch is
> the right thing to do.
>
> Thanx, Paul
>

local_irq_disable() does not touch preempt_count, it touchs
a register of current CPU.

The following quick test module is a case where RCU_SCHED is used where


the interrupt hardware is disabled, but preempt_count() has

not been updated, and it raises rcu-lockdep complaint.

#include <linux/module.h>
#include <linux/hardirq.h>
#include <linux/rcupdate.h>

void *test = &test;

int test_init(void)
{
local_irq_disable();
printk(KERN_INFO "%p\n", rcu_dereference_sched(test));
local_irq_enable();

return 0;
}

void test_exit(void) {}

module_init(test_init);
module_exit(test_exit);
MODULE_LICENSE("GPL");

Paul E. McKenney

unread,
Mar 16, 2010, 11:50:01 PM3/16/10
to
On Tue, Mar 16, 2010 at 06:21:56AM -0700, Paul E. McKenney wrote:
> On Tue, Mar 16, 2010 at 07:09:21PM +0800, Lai Jiangshan wrote:
> > It is documented that local_irq_disable() also delimits
> > RCU_SCHED read-site critical sections.
> > See the document of synchronize_sched() or
> > Documentation/RCU/whatisRCU.txt.
> >
> > So we have to test irqs_disabled() in rcu_read_lock_sched_held().
> > Otherwise rcu-lockdep brings incorrect complaint.
>
> Interesting -- I was under the impression that preempt_count() covered
> this as well, due to the following in include/linux/hardirq.h:
>
> #define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> #define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
>
> But irqs_disabled() does look to sample the actual interrupt hardware.
>
> So, if there are cases where RCU is used where the interrupt hardware
> is disabled, but preempt_count() has not been updated, this patch is
> the right thing to do.

In light of later emails in this thread, first a big "thank you!!!"
to Lai, and I will pull this one in. Good stuff!!!

Paul E. McKenney

unread,
Mar 18, 2010, 3:30:03 PM3/18/10
to
From: Lai Jiangshan <la...@cn.fujitsu.com>

It is documented that local_irq_disable() also delimits
RCU_SCHED read-site critical sections.
See the document of synchronize_sched() or
Documentation/RCU/whatisRCU.txt.

So we have to test irqs_disabled() in rcu_read_lock_sched_held().
Otherwise rcu-lockdep brings incorrect complaint.

Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com>
Signed-off-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e1bdc4b..872a98e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -149,7 +149,7 @@ static inline int rcu_read_lock_sched_held(void)


return 1;
if (debug_locks)
lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || preempt_count() != 0;
+ return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)

@@ -180,7 +180,7 @@ static inline int rcu_read_lock_bh_held(void)


#ifdef CONFIG_PREEMPT
static inline int rcu_read_lock_sched_held(void)
{
- return !rcu_scheduler_active || preempt_count() != 0;
+ return !rcu_scheduler_active || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)
--

1.7.0

tip-bot for Lai Jiangshan

unread,
Mar 18, 2010, 6:10:02 PM3/18/10
to
Commit-ID: 0cff810f54b3b52075c27f7a7021d5b195264b6c
Gitweb: http://git.kernel.org/tip/0cff810f54b3b52075c27f7a7021d5b195264b6c
Author: Lai Jiangshan <la...@cn.fujitsu.com>
AuthorDate: Thu, 18 Mar 2010 12:25:33 -0700
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Thu, 18 Mar 2010 21:25:32 +0100

rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives

It is documented that local_irq_disable() also delimits RCU_SCHED
read-site critical sections.

See the document of synchronize_sched() or
Documentation/RCU/whatisRCU.txt.

So we have to test irqs_disabled() in rcu_read_lock_sched_held().
Otherwise rcu-lockdep brings incorrect complaint.

Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com>
Signed-off-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

Cc: dipa...@in.ibm.com
Cc: mathieu....@polymtl.ca
Cc: jo...@joshtriplett.org
Cc: dvh...@us.ibm.com
Cc: n...@us.ibm.com
Cc: pet...@infradead.org
Cc: ros...@goodmis.org
Cc: Valdis.K...@vt.edu
Cc: dhow...@redhat.com
Cc: eric.d...@gmail.com
LKML-Reference: <1268940334-10892-1-g...@linux.vnet.ibm.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>


---
include/linux/rcupdate.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e1bdc4b..872a98e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -149,7 +149,7 @@ static inline int rcu_read_lock_sched_held(void)
return 1;
if (debug_locks)
lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || preempt_count() != 0;
+ return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)
@@ -180,7 +180,7 @@ static inline int rcu_read_lock_bh_held(void)
#ifdef CONFIG_PREEMPT
static inline int rcu_read_lock_sched_held(void)
{
- return !rcu_scheduler_active || preempt_count() != 0;
+ return !rcu_scheduler_active || preempt_count() != 0 || irqs_disabled();
}
#else /* #ifdef CONFIG_PREEMPT */
static inline int rcu_read_lock_sched_held(void)
--

0 new messages