[PATCH 0/4 v2] rcu: Detect rcu uses under extended quiescent state, and fix some

[PATCH 0/4 v2] rcu: Detect rcu uses under extended quiescent state, and fix some

[Frederic Weisbecker]

In this second version:

- rebase against latest rcu/dev branch of Paul's tree
- handle tiny RCU
- put acks

Frederic Weisbecker (4):
  rcu: Detect uses of rcu read side in extended quiescent states
  nohz: Split extended quiescent state handling from nohz switch
  x86: Don't call idle notifier inside rcu extended QS
  x86: Call idle_exit() after irq_enter()

 arch/arm/kernel/process.c                |    4 +-
 arch/avr32/kernel/process.c              |    4 +-
 arch/blackfin/kernel/process.c           |    4 +-
 arch/microblaze/kernel/process.c         |    4 +-
 arch/mips/kernel/process.c               |    4 +-
 arch/powerpc/kernel/idle.c               |    4 +-
 arch/powerpc/platforms/iseries/setup.c   |    8 +++---
 arch/s390/kernel/process.c               |    4 +-
 arch/sh/kernel/idle.c                    |    2 +-
 arch/sparc/kernel/process_64.c           |    4 +-
 arch/tile/kernel/process.c               |    4 +-
 arch/um/kernel/process.c                 |    4 +-
 arch/unicore32/kernel/process.c          |    4 +-
 arch/x86/kernel/apic/apic.c              |    6 ++--
 arch/x86/kernel/apic/io_apic.c           |    3 +-
 arch/x86/kernel/cpu/mcheck/mce.c         |    2 +-
 arch/x86/kernel/cpu/mcheck/therm_throt.c |    2 +-
 arch/x86/kernel/cpu/mcheck/threshold.c   |    2 +-
 arch/x86/kernel/irq.c                    |    5 +--
 arch/x86/kernel/process_32.c             |    4 +-
 arch/x86/kernel/process_64.c             |    7 +++++-
 include/linux/rcupdate.h                 |    9 +++++++
 include/linux/tick.h                     |   10 +++++--
 kernel/rcutiny.c                         |   13 ++++++++++
 kernel/rcutree.c                         |   14 +++++++++++
 kernel/time/tick-sched.c                 |   36 ++++++++++++++++++++++++++---
 26 files changed, 120 insertions(+), 47 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Frederic Weisbecker]

Detect uses of rcu that are not supposed to happen when we
are in an extended quiescent state.

This can happen for example if we use rcu between the time we
stop the tick and the time we restart it. Or inside an irq that
didn't use rcu_irq_enter,exit() or other possible kind of rcu API
misuse.

v2: Rebase against latest rcu changes, handle tiny RCU as well

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Milton Miller <miltonm@bga.com>
---
 include/linux/rcupdate.h |    9 +++++++++
 kernel/rcutiny.c         |   13 +++++++++++++
 kernel/rcutree.c         |   14 ++++++++++++++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 9db50df..6cad1f3 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -388,6 +388,12 @@ extern int rcu_my_thread_group_empty(void);
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
 
+#if defined(CONFIG_PROVE_RCU) && defined(CONFIG_NO_HZ)
+extern bool rcu_check_extended_qs(void);
+#else
+static inline bool rcu_check_extended_qs(void) { return false; }
+#endif
+
 /*
  * Helper functions for rcu_dereference_check(), rcu_dereference_protected()
  * and rcu_assign_pointer().  Some of these could be folded into their
@@ -415,6 +421,9 @@ extern int rcu_my_thread_group_empty(void);
 		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
 		rcu_lockdep_assert(c, "suspicious rcu_dereference_check()" \
 				      " usage"); \
+		rcu_lockdep_assert(!rcu_check_extended_qs(), \
+				   "use of rcu_dereference_check() in an extended" \
+				   " quiescent state");	\
 		rcu_dereference_sparse(p, space); \
 		smp_read_barrier_depends(); \
 		((typeof(*p) __force __kernel *)(_________p1)); \
diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index 775d69a..44a2a15 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -76,6 +76,19 @@ void rcu_exit_nohz(void)
 	rcu_dynticks_nesting++;
 }
 
+
+#ifdef CONFIG_PROVE_RCU
+
+bool rcu_check_extended_qs(void)
+{
+	if (!rcu_dynticks_nesting)
+		return true;
+
+	return false;
+}
+
+#endif
+
 #endif /* #ifdef CONFIG_NO_HZ */
 
 /*
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 305dfae..8211527 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -432,6 +432,20 @@ void rcu_irq_exit(void)
 	rcu_enter_nohz();
 }
 
+#ifdef CONFIG_PROVE_RCU
+
+bool rcu_check_extended_qs(void)
+{
+	struct rcu_dynticks *rdtp = &__get_cpu_var(rcu_dynticks);
+
+	if (atomic_read(&rdtp->dynticks) & 0x1)
+		return false;
+
+	return true;
+}
+
+#endif /* CONFIG_PROVE_RCU */
+
 #ifdef CONFIG_SMP
 
 /*
-- 
1.7.5.4

[PATCH 2/4] nohz: Split extended quiescent state handling from nohz switch

[Frederic Weisbecker]

It is assumed that rcu won't be used once we switch to tickless
mode and until we restart the tick. However this is not always
true, as in x86-64 where we dereference the idle notifiers after
the tick is stopped.

To prepare for fixing this, split the tickless mode switching and
RCU extended quiescent state logics.
Make tick_nohz_stop/restart_sched_tick() RCU agnostic but provide
a new pair of APIs tick_nohz_enter/exit_idle() that keep the
old behaviour by handling both the nohz mode and RCU extended
quiescent states, then convert every archs to use these.

Archs that want to switch to extended QS to some custom points
can do it later by using tick_nohz_stop_sched_tick() and
rcu_enter_nohz() seperately.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: David Miller <davem@davemloft.net>
Acked-by: Chris Metcalf <cmetcalf@tilera.com>
Acked-by: Guan Xuetao <gxt@mprc.pku.edu.cn>
Acked-by: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
Acked-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/arm/kernel/process.c              |    4 +-
 arch/avr32/kernel/process.c            |    4 +-
 arch/blackfin/kernel/process.c         |    4 +-
 arch/microblaze/kernel/process.c       |    4 +-
 arch/mips/kernel/process.c             |    4 +-
 arch/powerpc/kernel/idle.c             |    4 +-
 arch/powerpc/platforms/iseries/setup.c |    8 +++---
 arch/s390/kernel/process.c             |    4 +-
 arch/sh/kernel/idle.c                  |    2 +-
 arch/sparc/kernel/process_64.c         |    4 +-
 arch/tile/kernel/process.c             |    4 +-
 arch/um/kernel/process.c               |    4 +-
 arch/unicore32/kernel/process.c        |    4 +-
 arch/x86/kernel/process_32.c           |    4 +-
 arch/x86/kernel/process_64.c           |    4 +-
 include/linux/tick.h                   |   10 ++++++--
 kernel/time/tick-sched.c               |   36 ++++++++++++++++++++++++++++---
 17 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 5e1e541..27b68b0 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -182,7 +182,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		leds_event(led_idle_start);
 		while (!need_resched()) {
 #ifdef CONFIG_HOTPLUG_CPU
@@ -208,7 +208,7 @@ void cpu_idle(void)
 			}
 		}
 		leds_event(led_idle_end);
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/avr32/kernel/process.c b/arch/avr32/kernel/process.c
index ef5a2a0..e683a34 100644
--- a/arch/avr32/kernel/process.c
+++ b/arch/avr32/kernel/process.c
@@ -34,10 +34,10 @@ void cpu_idle(void)
 {
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched())
 			cpu_idle_sleep();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/blackfin/kernel/process.c b/arch/blackfin/kernel/process.c
index b407bc8..fd78345 100644
--- a/arch/blackfin/kernel/process.c
+++ b/arch/blackfin/kernel/process.c
@@ -88,10 +88,10 @@ void cpu_idle(void)
 #endif
 		if (!idle)
 			idle = default_idle;
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched())
 			idle();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/microblaze/kernel/process.c b/arch/microblaze/kernel/process.c
index 968648a..1b295b2 100644
--- a/arch/microblaze/kernel/process.c
+++ b/arch/microblaze/kernel/process.c
@@ -103,10 +103,10 @@ void cpu_idle(void)
 		if (!idle)
 			idle = default_idle;
 
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched())
 			idle();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 
 		preempt_enable_no_resched();
 		schedule();
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index d2112d3..cdbfa52 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -56,7 +56,7 @@ void __noreturn cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched() && cpu_online(cpu)) {
 #ifdef CONFIG_MIPS_MT_SMTC
 			extern void smtc_idle_loop_hook(void);
@@ -77,7 +77,7 @@ void __noreturn cpu_idle(void)
 		     system_state == SYSTEM_BOOTING))
 			play_dead();
 #endif
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
index 39a2baa..1108260 100644
--- a/arch/powerpc/kernel/idle.c
+++ b/arch/powerpc/kernel/idle.c
@@ -56,7 +56,7 @@ void cpu_idle(void)
 
 	set_thread_flag(TIF_POLLING_NRFLAG);
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched() && !cpu_should_die()) {
 			ppc64_runlatch_off();
 
@@ -93,7 +93,7 @@ void cpu_idle(void)
 
 		HMT_medium();
 		ppc64_runlatch_on();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		if (cpu_should_die())
 			cpu_die();
diff --git a/arch/powerpc/platforms/iseries/setup.c b/arch/powerpc/platforms/iseries/setup.c
index 2946ae1..50b5810 100644
--- a/arch/powerpc/platforms/iseries/setup.c
+++ b/arch/powerpc/platforms/iseries/setup.c
@@ -562,7 +562,7 @@ static void yield_shared_processor(void)
 static void iseries_shared_idle(void)
 {
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched() && !hvlpevent_is_pending()) {
 			local_irq_disable();
 			ppc64_runlatch_off();
@@ -576,7 +576,7 @@ static void iseries_shared_idle(void)
 		}
 
 		ppc64_runlatch_on();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 
 		if (hvlpevent_is_pending())
 			process_iSeries_events();
@@ -592,7 +592,7 @@ static void iseries_dedicated_idle(void)
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		if (!need_resched()) {
 			while (!need_resched()) {
 				ppc64_runlatch_off();
@@ -609,7 +609,7 @@ static void iseries_dedicated_idle(void)
 		}
 
 		ppc64_runlatch_on();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index a895e69..8c53c46 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -105,10 +105,10 @@ static void default_idle(void)
 void cpu_idle(void)
 {
 	for (;;) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched())
 			default_idle();
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/sh/kernel/idle.c b/arch/sh/kernel/idle.c
index 425d604..3957972 100644
--- a/arch/sh/kernel/idle.c
+++ b/arch/sh/kernel/idle.c
@@ -88,7 +88,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 
 		while (!need_resched()) {
 			check_pgt_cache();
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index c158a95..5c36632 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -95,12 +95,12 @@ void cpu_idle(void)
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
 	while(1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 
 		while (!need_resched() && !cpu_is_offline(cpu))
 			sparc64_yield(cpu);
 
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 
 		preempt_enable_no_resched();
 
diff --git a/arch/tile/kernel/process.c b/arch/tile/kernel/process.c
index d006510..1fc9578 100644
--- a/arch/tile/kernel/process.c
+++ b/arch/tile/kernel/process.c
@@ -82,7 +82,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched()) {
 			if (cpu_is_offline(cpu))
 				BUG();  /* no HOTPLUG_CPU */
@@ -102,7 +102,7 @@ void cpu_idle(void)
 				local_irq_enable();
 			current_thread_info()->status |= TS_POLLING;
 		}
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index fab4371..f1b3864 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -245,10 +245,10 @@ void default_idle(void)
 		if (need_resched())
 			schedule();
 
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		nsecs = disable_timer();
 		idle_sleep(nsecs);
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 	}
 }
 
diff --git a/arch/unicore32/kernel/process.c b/arch/unicore32/kernel/process.c
index ba401df..e2df91a 100644
--- a/arch/unicore32/kernel/process.c
+++ b/arch/unicore32/kernel/process.c
@@ -55,7 +55,7 @@ void cpu_idle(void)
 {
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched()) {
 			local_irq_disable();
 			stop_critical_timings();
@@ -63,7 +63,7 @@ void cpu_idle(void)
 			local_irq_enable();
 			start_critical_timings();
 		}
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 8d12878..41e7d1b 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -97,7 +97,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched()) {
 
 			check_pgt_cache();
@@ -112,7 +112,7 @@ void cpu_idle(void)
 			pm_idle();
 			start_critical_timings();
 		}
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 6c9dd92..3fe0883 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -120,7 +120,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_stop_sched_tick(1);
+		tick_nohz_enter_idle();
 		while (!need_resched()) {
 
 			rmb();
@@ -145,7 +145,7 @@ void cpu_idle(void)
 			__exit_idle();
 		}
 
-		tick_nohz_restart_sched_tick();
+		tick_nohz_exit_idle();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
diff --git a/include/linux/tick.h b/include/linux/tick.h
index b232ccc..ff31a71 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -121,14 +121,18 @@ static inline int tick_oneshot_mode_active(void) { return 0; }
 #endif /* !CONFIG_GENERIC_CLOCKEVENTS */
 
 # ifdef CONFIG_NO_HZ
-extern void tick_nohz_stop_sched_tick(int inidle);
+extern bool tick_nohz_stop_sched_tick(int inidle);
 extern void tick_nohz_restart_sched_tick(void);
+extern void tick_nohz_enter_idle(void);
+extern void tick_nohz_exit_idle(void);
 extern ktime_t tick_nohz_get_sleep_length(void);
 extern u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time);
 extern u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time);
 # else
-static inline void tick_nohz_stop_sched_tick(int inidle) { }
-static inline void tick_nohz_restart_sched_tick(void) { }
+static inline bool tick_nohz_stop_sched_tick(int inidle) { return false; }
+static inline void tick_nohz_restart_sched_tick(void) { return false; }
+static inline void tick_nohz_enter_idle(void) { }
+static inline void tick_nohz_exit_idle(void) { }
 static inline ktime_t tick_nohz_get_sleep_length(void)
 {
 	ktime_t len = { .tv64 = NSEC_PER_SEC/HZ };
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index d5097c4..8e81e9f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -254,12 +254,13 @@ EXPORT_SYMBOL_GPL(get_cpu_iowait_time_us);
  * Called either from the idle loop or from irq_exit() when an idle period was
  * just interrupted by an interrupt which did not cause a reschedule.
  */
-void tick_nohz_stop_sched_tick(int inidle)
+bool tick_nohz_stop_sched_tick(int inidle)
 {
 	unsigned long seq, last_jiffies, next_jiffies, delta_jiffies, flags;
 	struct tick_sched *ts;
 	ktime_t last_update, expires, now;
 	struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev;
+	int stopped = false;
 	u64 time_delta;
 	int cpu;
 
@@ -409,7 +410,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 			ts->idle_tick = hrtimer_get_expires(&ts->sched_timer);
 			ts->tick_stopped = 1;
 			ts->idle_jiffies = last_jiffies;
-			rcu_enter_nohz();
+			stopped = true;
 		}
 
 		ts->idle_sleeps++;
@@ -450,6 +451,22 @@ out:
 	ts->sleep_length = ktime_sub(dev->next_event, now);
 end:
 	local_irq_restore(flags);
+
+	return stopped;
+}
+
+
+/**
+ * tick_nohz_enter_idle - stop the tick and enter extended quiescent state
+ *
+ * Most arch may want to enter RCU extended state right after they switched
+ * to nohz mode. Beware though, no read side use of RCU can be done until we
+ * call tick_nohz_exit_idle().
+ */
+void tick_nohz_enter_idle(void)
+{
+	if (tick_nohz_stop_sched_tick(1))
+		rcu_enter_nohz();
 }
 
 /**
@@ -519,8 +536,6 @@ void tick_nohz_restart_sched_tick(void)
 
 	ts->inidle = 0;
 
-	rcu_exit_nohz();
-
 	/* Update jiffies first */
 	select_nohz_load_balancer(0);
 	tick_do_update_jiffies64(now);
@@ -552,6 +567,19 @@ void tick_nohz_restart_sched_tick(void)
 	local_irq_enable();
 }
 
+/**
+ * tick_nohz_exit_idle - restart the tick and exit extended quiescent state
+ */
+void tick_nohz_exit_idle(void)
+{
+	struct tick_sched *ts = &__raw_get_cpu_var(tick_cpu_sched);
+
+	if (ts->tick_stopped)
+		rcu_exit_nohz();
+
+	tick_nohz_restart_sched_tick();
+}
+
 static int tick_nohz_reprogram(struct tick_sched *ts, ktime_t now)
 {
 	hrtimer_forward(&ts->sched_timer, now, tick_period);
-- 
1.7.5.4

[PATCH 3/4] x86: Don't call idle notifier inside rcu extended QS

[Frederic Weisbecker]

The idle notifier in enter_idle / __exit_idle is called
inside the RCU extended quiescent state.

This results in the following warning:

[    2.081278] ===================================================
[    2.081281] [ INFO: suspicious rcu_dereference_check() usage. ]
[    2.081282] ---------------------------------------------------
[    2.081284] kernel/notifier.c:81 invoked rcu_dereference_check() while in RCU extended quiescent state!
[    2.081286]
[    2.081287] other info that might help us debug this:
[    2.081288]
[    2.081289]
[    2.081289] rcu_scheduler_active = 1, debug_locks = 0
[    2.081292] 1 lock held by kworker/0:0/0:
[    2.081293]  #0:  (rcu_read_lock){.+.+..}, at: [<ffffffff81785c60>] __atomic_notifier_call_chain+0x0/0xa0
[    2.081301]
[    2.081302] stack backtrace:
[    2.081305] Pid: 0, comm: kworker/0:0 Not tainted 3.0.0-rc1+ #103
[    2.081307] Call Trace:
[    2.081313]  [<ffffffff8108e0e4>] lockdep_rcu_dereference+0xd4/0x100
[    2.081316]  [<ffffffff81785c4a>] notifier_call_chain+0x13a/0x150
[    2.081319]  [<ffffffff81785cc7>] __atomic_notifier_call_chain+0x67/0xa0
[    2.081322]  [<ffffffff81785c60>] ? notifier_call_chain+0x150/0x150
[    2.081325]  [<ffffffff81785d11>] atomic_notifier_call_chain+0x11/0x20
[    2.081329]  [<ffffffff8100afa0>] enter_idle+0x20/0x30
[    2.081332]  [<ffffffff8100b05e>] cpu_idle+0xae/0x100
[    2.081335]  [<ffffffff8177951f>] start_secondary+0x1cf/0x1d6

Fix this by entering RCU extended QS later, after we call the
notifier. Exit it also sooner to call the exit idle notifier.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/process_64.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 3fe0883..a8bd222 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -120,7 +120,7 @@ void cpu_idle(void)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
-		tick_nohz_enter_idle();
+		tick_nohz_stop_sched_tick(1);
 		while (!need_resched()) {
 
 			rmb();
@@ -134,18 +134,23 @@ void cpu_idle(void)
 			 */
 			local_irq_disable();
 			enter_idle();
+			/*
+			 * We can't enter extended QS before due to idle
+			 * notifier that uses RCU.
+			 */
+			rcu_enter_nohz();
 			/* Don't trace irqs off for idle */
 			stop_critical_timings();
 			pm_idle();
 			start_critical_timings();
-
+			rcu_exit_nohz();
 			/* In many cases the interrupt that ended idle
 			   has already called exit_idle. But some idle
 			   loops can be woken up without interrupt. */
 			__exit_idle();
 		}
 
-		tick_nohz_exit_idle();
+		tick_nohz_restart_sched_tick();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();
-- 
1.7.5.4

[PATCH 4/4] x86: Call idle_exit() after irq_enter()

[Frederic Weisbecker]

idle_exit() calls the idle notifier, which uses RCU. But
this is called before we call irq_enter(), thus before we
exit the RCU extended quiescent state if we are in idle.

Fix this by calling the idle exit notifier after irq_enter().

This fixes the following warning:

[    0.559953] ===================================================
[    0.559954] [ INFO: suspicious rcu_dereference_check() usage. ]
[    0.559956] ---------------------------------------------------
[    0.559958] kernel/notifier.c:81 invoked rcu_dereference_check() while in RCU extended quiescent state!
[    0.559960]
[    0.559961] other info that might help us debug this:
[    0.559961]
[    0.559962]
[    0.559963] rcu_scheduler_active = 1, debug_locks = 1
[    0.559965] 1 lock held by kworker/0:0/0:
[    0.559966]  #0:  (rcu_read_lock){......}, at: [<ffffffff81785ce0>] __atomic_notifier_call_chain+0x0/0xa0
[    0.559976]
[    0.559977] stack backtrace:
[    0.559980] Pid: 0, comm: kworker/0:0 Not tainted 3.0.0-rc1+ #108
[    0.559981] Call Trace:
[    0.559983]  <IRQ>  [<ffffffff8108e176>] lockdep_rcu_dereference+0xb6/0xf0
[    0.559990]  [<ffffffff81785cca>] notifier_call_chain+0x13a/0x150
[    0.559993]  [<ffffffff81785d47>] __atomic_notifier_call_chain+0x67/0xa0
[    0.559996]  [<ffffffff81785ce0>] ? notifier_call_chain+0x150/0x150
[    0.560000]  [<ffffffff812a454e>] ? do_raw_spin_unlock+0x5e/0xb0
[    0.560000]  [<ffffffff81785d91>] atomic_notifier_call_chain+0x11/0x20
[    0.560000]  [<ffffffff8100af73>] exit_idle+0x43/0x50
[    0.560000]  [<ffffffff81027869>] smp_apic_timer_interrupt+0x39/0xa0
[    0.560000]  [<ffffffff81789e13>] apic_timer_interrupt+0x13/0x20
[    0.560000]  <EOI>  [<ffffffff810301e6>] ? native_safe_halt+0x6/0x10
[    0.560000]  [<ffffffff8108fecd>] ? trace_hardirqs_on+0xd/0x10
[    0.560000]  [<ffffffff810130ac>] default_idle+0x2c/0x50
[    0.560000]  [<ffffffff810131c8>] amd_e400_idle+0x58/0x130
[    0.560000]  [<ffffffff8100b069>] cpu_idle+0xb9/0x110
[    0.560000]  [<ffffffff817795af>] start_secondary+0x1cf/0x1d6

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/apic/apic.c              |    6 +++---
 arch/x86/kernel/apic/io_apic.c           |    3 ++-
 arch/x86/kernel/cpu/mcheck/mce.c         |    2 +-
 arch/x86/kernel/cpu/mcheck/therm_throt.c |    2 +-
 arch/x86/kernel/cpu/mcheck/threshold.c   |    2 +-
 arch/x86/kernel/irq.c                    |    5 ++---
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index fabf01e..d3db17c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -855,8 +855,8 @@ void __irq_entry smp_apic_timer_interrupt(struct pt_regs *regs)
 	 * Besides, if we don't timer interrupts ignore the global
 	 * interrupt lock, which is the WrongThing (tm) to do.
 	 */
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	local_apic_timer_interrupt();
 	irq_exit();
 
@@ -1788,8 +1788,8 @@ void smp_spurious_interrupt(struct pt_regs *regs)
 {
 	u32 v;
 
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	/*
 	 * Check if this really is a spurious interrupt and ACK it
 	 * if it is a vectored one.  Just in case...
@@ -1814,8 +1814,8 @@ void smp_error_interrupt(struct pt_regs *regs)
 {
 	u32 v, v1;
 
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	/* First tickle the hardware, only then report what went on. -- REW */
 	v = apic_read(APIC_ESR);
 	apic_write(APIC_ESR, 0);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 45fd33d..f0744a3 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2290,8 +2290,9 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 	unsigned vector, me;
 
 	ack_APIC_irq();
-	exit_idle();
+
 	irq_enter();
+	exit_idle();
 
 	me = smp_processor_id();
 	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 3385ea2..b3cdf7c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -478,8 +478,8 @@ static inline void mce_get_rip(struct mce *m, struct pt_regs *regs)
 asmlinkage void smp_mce_self_interrupt(struct pt_regs *regs)
 {
 	ack_APIC_irq();
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	mce_notify_irq();
 	mce_schedule_work();
 	irq_exit();
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 6f8c5e9..569c0fa 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -400,8 +400,8 @@ static void (*smp_thermal_vector)(void) = unexpected_thermal_interrupt;
 
 asmlinkage void smp_thermal_interrupt(struct pt_regs *regs)
 {
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	inc_irq_stat(irq_thermal_count);
 	smp_thermal_vector();
 	irq_exit();
diff --git a/arch/x86/kernel/cpu/mcheck/threshold.c b/arch/x86/kernel/cpu/mcheck/threshold.c
index d746df2..aa578ca 100644
--- a/arch/x86/kernel/cpu/mcheck/threshold.c
+++ b/arch/x86/kernel/cpu/mcheck/threshold.c
@@ -19,8 +19,8 @@ void (*mce_threshold_vector)(void) = default_threshold_interrupt;
 
 asmlinkage void smp_threshold_interrupt(void)
 {
-	exit_idle();
 	irq_enter();
+	exit_idle();
 	inc_irq_stat(irq_threshold_count);
 	mce_threshold_vector();
 	irq_exit();
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 1cb0b9f..08f947d 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -180,8 +180,8 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
 	unsigned vector = ~regs->orig_ax;
 	unsigned irq;
 
-	exit_idle();
 	irq_enter();
+	exit_idle();
 
 	irq = __this_cpu_read(vector_irq[vector]);
 
@@ -208,9 +208,8 @@ void smp_x86_platform_ipi(struct pt_regs *regs)
 
 	ack_APIC_irq();
 
-	exit_idle();
-
 	irq_enter();
+	exit_idle();
 
 	inc_irq_stat(x86_platform_ipis);
 
-- 
1.7.5.4

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Paul E. McKenney]

On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> Detect uses of rcu that are not supposed to happen when we
> are in an extended quiescent state.
> 
> This can happen for example if we use rcu between the time we
> stop the tick and the time we restart it. Or inside an irq that
> didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> misuse.
> 
> v2: Rebase against latest rcu changes, handle tiny RCU as well

Good idea on checking for RCU read-side critical sections happening
in dyntick-idle periods!

But wouldn't it be better to put the checks in rcu_read_lock() and
friends?  The problem I see with putting them in rcu_dereference_check()
is that someone can legitimately do something like the following
while in dyntick-idle mode:

	spin_lock(&mylock);
	/* do a bunch of stuff */
	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));

The logic below would complain about this usage, despite the fact
that it is perfectly safe because the update-side lock is held.

Make sense, or am I missing something?

						Thanx, Paul

> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Milton Miller <miltonm@bga.com>
> ---
>  include/linux/rcupdate.h |    9 +++++++++
>  kernel/rcutiny.c         |   13 +++++++++++++
>  kernel/rcutree.c         |   14 ++++++++++++++
>  3 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 9db50df..6cad1f3 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -388,6 +388,12 @@ extern int rcu_my_thread_group_empty(void);
> 
>  #endif /* #else #ifdef CONFIG_PROVE_RCU */
> 
> +#if defined(CONFIG_PROVE_RCU) && defined(CONFIG_NO_HZ)
> +extern bool rcu_check_extended_qs(void);
> +#else
> +static inline bool rcu_check_extended_qs(void) { return false; }
> +#endif
> +
>  /*
>   * Helper functions for rcu_dereference_check(), rcu_dereference_protected()
>   * and rcu_assign_pointer().  Some of these could be folded into their
> @@ -415,6 +421,9 @@ extern int rcu_my_thread_group_empty(void);
>  		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
>  		rcu_lockdep_assert(c, "suspicious rcu_dereference_check()" \
>  				      " usage"); \
> +		rcu_lockdep_assert(!rcu_check_extended_qs(), \
> +				   "use of rcu_dereference_check() in an extended" \
> +				   " quiescent state");	\
>  		rcu_dereference_sparse(p, space); \
>  		smp_read_barrier_depends(); \
>  		((typeof(*p) __force __kernel *)(_________p1)); \
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index 775d69a..44a2a15 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -76,6 +76,19 @@ void rcu_exit_nohz(void)
>  	rcu_dynticks_nesting++;
>  }
> 
> +
> +#ifdef CONFIG_PROVE_RCU
> +
> +bool rcu_check_extended_qs(void)
> +{
> +	if (!rcu_dynticks_nesting)
> +		return true;
> +
> +	return false;
> +}
> +
> +#endif
> +
>  #endif /* #ifdef CONFIG_NO_HZ */
> 
>  /*
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 305dfae..8211527 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -432,6 +432,20 @@ void rcu_irq_exit(void)
>  	rcu_enter_nohz();
>  }
> 
> +#ifdef CONFIG_PROVE_RCU
> +
> +bool rcu_check_extended_qs(void)
> +{
> +	struct rcu_dynticks *rdtp = &__get_cpu_var(rcu_dynticks);
> +
> +	if (atomic_read(&rdtp->dynticks) & 0x1)
> +		return false;
> +
> +	return true;
> +}
> +
> +#endif /* CONFIG_PROVE_RCU */
> +
>  #ifdef CONFIG_SMP
> 
>  /*
> -- 
> 1.7.5.4
> 

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Frederic Weisbecker]

On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > Detect uses of rcu that are not supposed to happen when we
> > are in an extended quiescent state.
> > 
> > This can happen for example if we use rcu between the time we
> > stop the tick and the time we restart it. Or inside an irq that
> > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > misuse.
> > 
> > v2: Rebase against latest rcu changes, handle tiny RCU as well
> 
> Good idea on checking for RCU read-side critical sections happening
> in dyntick-idle periods!
> 
> But wouldn't it be better to put the checks in rcu_read_lock() and
> friends?  The problem I see with putting them in rcu_dereference_check()
> is that someone can legitimately do something like the following
> while in dyntick-idle mode:
> 
> 	spin_lock(&mylock);
> 	/* do a bunch of stuff */
> 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> 
> The logic below would complain about this usage, despite the fact
> that it is perfectly safe because the update-side lock is held.
> 
> Make sense, or am I missing something?
> 
> 						Thanx, Paul

I'm an idiot. I put my check in rcu_dereference_check() on purpose because
it's always called from places that check one of the rcu locks are held,
but I forgot that's also used for custom conditions with the _check()
things.

That said, putting the check in rcu_read_lock() and alike  would only work
with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
checks there as it's fine to disabled irqs in dyntick idle.

What about the below? (untested yet)

And I would print the state of dynticks-idle mode in the final lockdep warning.

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 6cad1f3..b9e68ae 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -278,7 +278,7 @@ static inline int rcu_read_lock_held(void)
 {
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
-	return lock_is_held(&rcu_lock_map);
+	return lock_is_held(&rcu_lock_map) && !rcu_check_extended_qs();
 }
 
 /*
@@ -310,13 +310,13 @@ static inline int rcu_read_lock_sched_held(void)
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
 	if (debug_locks)
-		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
+		lockdep_opinion = lock_is_held(&rcu_sched_lock_map) && !rcu_check_extended_qs();
 	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
 }
 #else /* #ifdef CONFIG_PREEMPT */
 static inline int rcu_read_lock_sched_held(void)
 {
-	return 1;
+	return !rcu_check_extended_qs();
 }
 #endif /* #else #ifdef CONFIG_PREEMPT */
 
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index a088c90..20d6e7228 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -88,7 +88,7 @@ int rcu_read_lock_bh_held(void)
 {
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
-	return in_softirq() || irqs_disabled();
+	return !rcu_check_extended_qs() && (in_softirq() || irqs_disabled());
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
 

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Paul E. McKenney]

On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote:
> On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > > Detect uses of rcu that are not supposed to happen when we
> > > are in an extended quiescent state.
> > > 
> > > This can happen for example if we use rcu between the time we
> > > stop the tick and the time we restart it. Or inside an irq that
> > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > > misuse.
> > > 
> > > v2: Rebase against latest rcu changes, handle tiny RCU as well
> > 
> > Good idea on checking for RCU read-side critical sections happening
> > in dyntick-idle periods!
> > 
> > But wouldn't it be better to put the checks in rcu_read_lock() and
> > friends?  The problem I see with putting them in rcu_dereference_check()
> > is that someone can legitimately do something like the following
> > while in dyntick-idle mode:
> > 
> > 	spin_lock(&mylock);
> > 	/* do a bunch of stuff */
> > 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> > 
> > The logic below would complain about this usage, despite the fact
> > that it is perfectly safe because the update-side lock is held.
> > 
> > Make sense, or am I missing something?
> > 
> > 						Thanx, Paul
> 
> I'm an idiot. I put my check in rcu_dereference_check() on purpose because
> it's always called from places that check one of the rcu locks are held,
> but I forgot that's also used for custom conditions with the _check()
> things.
> 
> That said, putting the check in rcu_read_lock() and alike  would only work
> with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
> call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
> checks there as it's fine to disabled irqs in dyntick idle.
> 
> What about the below? (untested yet)
> 
> And I would print the state of dynticks-idle mode in the final lockdep warning.

Printing the dynticks-idle mode would be quite good!

However, it is possible to have an RCU read-side critical section that does
not have an rcu_dereference() or an rcu_read_lock_held().  So I do believe
that we really do need rcu_read_lock() and friends to do this checking.

That might seem to leave open the possibility of a stray rcu_dereference()
being executed from dyntick-idle mode, but the existing PROVE_RCU
checking will catch that, right?

So I believe that the simplest approach with the best coverage is to
put the checks into RCU's read-side critical-section-entry primitives.

Make sense, or am I confused?

							Thanx, Paul

> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 6cad1f3..b9e68ae 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -278,7 +278,7 @@ static inline int rcu_read_lock_held(void)
>  {
>  	if (!debug_lockdep_rcu_enabled())
>  		return 1;
> -	return lock_is_held(&rcu_lock_map);
> +	return lock_is_held(&rcu_lock_map) && !rcu_check_extended_qs();
>  }
> 
>  /*
> @@ -310,13 +310,13 @@ static inline int rcu_read_lock_sched_held(void)
>  	if (!debug_lockdep_rcu_enabled())
>  		return 1;
>  	if (debug_locks)
> -		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> +		lockdep_opinion = lock_is_held(&rcu_sched_lock_map) && !rcu_check_extended_qs();
>  	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
>  }
>  #else /* #ifdef CONFIG_PREEMPT */
>  static inline int rcu_read_lock_sched_held(void)
>  {
> -	return 1;
> +	return !rcu_check_extended_qs();
>  }
>  #endif /* #else #ifdef CONFIG_PREEMPT */
> 
> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> index a088c90..20d6e7228 100644
> --- a/kernel/rcupdate.c
> +++ b/kernel/rcupdate.c
> @@ -88,7 +88,7 @@ int rcu_read_lock_bh_held(void)
>  {
>  	if (!debug_lockdep_rcu_enabled())
>  		return 1;
> -	return in_softirq() || irqs_disabled();
> +	return !rcu_check_extended_qs() && (in_softirq() || irqs_disabled());
>  }
>  EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
> 

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Frederic Weisbecker]

On Fri, Jun 17, 2011 at 04:19:03PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote:
> > On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> > > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > > > Detect uses of rcu that are not supposed to happen when we
> > > > are in an extended quiescent state.
> > > > 
> > > > This can happen for example if we use rcu between the time we
> > > > stop the tick and the time we restart it. Or inside an irq that
> > > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > > > misuse.
> > > > 
> > > > v2: Rebase against latest rcu changes, handle tiny RCU as well
> > > 
> > > Good idea on checking for RCU read-side critical sections happening
> > > in dyntick-idle periods!
> > > 
> > > But wouldn't it be better to put the checks in rcu_read_lock() and
> > > friends?  The problem I see with putting them in rcu_dereference_check()
> > > is that someone can legitimately do something like the following
> > > while in dyntick-idle mode:
> > > 
> > > 	spin_lock(&mylock);
> > > 	/* do a bunch of stuff */
> > > 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> > > 
> > > The logic below would complain about this usage, despite the fact
> > > that it is perfectly safe because the update-side lock is held.
> > > 
> > > Make sense, or am I missing something?
> > > 
> > > 						Thanx, Paul
> > 
> > I'm an idiot. I put my check in rcu_dereference_check() on purpose because
> > it's always called from places that check one of the rcu locks are held,
> > but I forgot that's also used for custom conditions with the _check()
> > things.
> > 
> > That said, putting the check in rcu_read_lock() and alike  would only work
> > with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
> > call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
> > checks there as it's fine to disabled irqs in dyntick idle.
> > 
> > What about the below? (untested yet)
> > 
> > And I would print the state of dynticks-idle mode in the final lockdep warning.
> 
> Printing the dynticks-idle mode would be quite good!
> 
> However, it is possible to have an RCU read-side critical section that does
> not have an rcu_dereference() or an rcu_read_lock_held().  So I do believe
> that we really do need rcu_read_lock() and friends to do this checking.

Right, then we need to check everything: rcu_read_lock() and friends in case
we have no rcu_read_lock_held() check made (ie: no rcu_dereference_check()),
but also rcu_read_lock_held()/rcu_read_lock_sched_held()/... because preempt_disable(),
local_irq_disable(), local_bh_disable() can't be checked so for rcu sched and rcu bh
we can only check the ...held() things.

> 
> That might seem to leave open the possibility of a stray rcu_dereference()
> being executed from dyntick-idle mode, but the existing PROVE_RCU
> checking will catch that, right?
> 
> So I believe that the simplest approach with the best coverage is to
> put the checks into RCU's read-side critical-section-entry primitives.
> 
> Make sense, or am I confused?

If we also check the rcu_read_...._held() things then yeah that works.
But checking only rcu_read_..._lock() things in not sufficient like I said
above.

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Paul E. McKenney]

On Sat, Jun 18, 2011 at 04:23:58PM +0200, Frederic Weisbecker wrote:
> On Fri, Jun 17, 2011 at 04:19:03PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote:
> > > On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > > > > Detect uses of rcu that are not supposed to happen when we
> > > > > are in an extended quiescent state.
> > > > > 
> > > > > This can happen for example if we use rcu between the time we
> > > > > stop the tick and the time we restart it. Or inside an irq that
> > > > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > > > > misuse.
> > > > > 
> > > > > v2: Rebase against latest rcu changes, handle tiny RCU as well
> > > > 
> > > > Good idea on checking for RCU read-side critical sections happening
> > > > in dyntick-idle periods!
> > > > 
> > > > But wouldn't it be better to put the checks in rcu_read_lock() and
> > > > friends?  The problem I see with putting them in rcu_dereference_check()
> > > > is that someone can legitimately do something like the following
> > > > while in dyntick-idle mode:
> > > > 
> > > > 	spin_lock(&mylock);
> > > > 	/* do a bunch of stuff */
> > > > 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> > > > 
> > > > The logic below would complain about this usage, despite the fact
> > > > that it is perfectly safe because the update-side lock is held.
> > > > 
> > > > Make sense, or am I missing something?
> > > > 
> > > > 						Thanx, Paul
> > > 
> > > I'm an idiot. I put my check in rcu_dereference_check() on purpose because
> > > it's always called from places that check one of the rcu locks are held,
> > > but I forgot that's also used for custom conditions with the _check()
> > > things.
> > > 
> > > That said, putting the check in rcu_read_lock() and alike  would only work
> > > with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
> > > call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
> > > checks there as it's fine to disabled irqs in dyntick idle.
> > > 
> > > What about the below? (untested yet)
> > > 
> > > And I would print the state of dynticks-idle mode in the final lockdep warning.
> > 
> > Printing the dynticks-idle mode would be quite good!
> > 
> > However, it is possible to have an RCU read-side critical section that does
> > not have an rcu_dereference() or an rcu_read_lock_held().  So I do believe
> > that we really do need rcu_read_lock() and friends to do this checking.
> 
> Right, then we need to check everything: rcu_read_lock() and friends in case
> we have no rcu_read_lock_held() check made (ie: no rcu_dereference_check()),
> but also rcu_read_lock_held()/rcu_read_lock_sched_held()/... because preempt_disable(),
> local_irq_disable(), local_bh_disable() can't be checked so for rcu sched and rcu bh
> we can only check the ...held() things.

Good point.  To make sure I understand, we have different approaches
for the different types of RCU.  We need to instrument the following
primitives:

1.	rcu_read_lock() because a stray rcu_dereference() will already
	be caught by PROVE_RCU.
2.	rcu_read_lock_bh_held() because it is OK to do local_bh_disable()
	in dyntick-idle mode, so we cannot prohibit all of the
	read-acquisition cases.  We can also instrument rcu_read_lock_bh()
	to catch RCU-bh read-side critical sections that don't happen
	to contain rcu_dereference_bh().
3.	rcu_read_lock_sched_held() because it is OK to do preempt_disable()
	and local_irq_save() from dyntick-idle mode, so we again
	cannot prohibit all of the read-acquisition cases.  We can
	also instrument rcu_read_lock_sched() to catch RCU-sched
	read-side critical sections that don't happen to contain
	rcu_dereference_sched().
4.	srcu_read_lock() because a stray srcu_dereference() will already
	by caught by PROVE_RCU.

We miss a few cases, for example, an RCU-sched read-side critical
section that uses local_irq_disable(), but that also does not contain
an rcu_dereference_sched().  But still this sounds quite worthwhile.

> > That might seem to leave open the possibility of a stray rcu_dereference()
> > being executed from dyntick-idle mode, but the existing PROVE_RCU
> > checking will catch that, right?
> > 
> > So I believe that the simplest approach with the best coverage is to
> > put the checks into RCU's read-side critical-section-entry primitives.
> > 
> > Make sense, or am I confused?
> 
> If we also check the rcu_read_...._held() things then yeah that works.
> But checking only rcu_read_..._lock() things in not sufficient like I said
> above.

Got it!

							Thanx, Paul

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Frederic Weisbecker]

On Sat, Jun 18, 2011 at 09:04:27AM -0700, Paul E. McKenney wrote:
> On Sat, Jun 18, 2011 at 04:23:58PM +0200, Frederic Weisbecker wrote:
> > On Fri, Jun 17, 2011 at 04:19:03PM -0700, Paul E. McKenney wrote:
> > > On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote:
> > > > On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > > > > > Detect uses of rcu that are not supposed to happen when we
> > > > > > are in an extended quiescent state.
> > > > > > 
> > > > > > This can happen for example if we use rcu between the time we
> > > > > > stop the tick and the time we restart it. Or inside an irq that
> > > > > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > > > > > misuse.
> > > > > > 
> > > > > > v2: Rebase against latest rcu changes, handle tiny RCU as well
> > > > > 
> > > > > Good idea on checking for RCU read-side critical sections happening
> > > > > in dyntick-idle periods!
> > > > > 
> > > > > But wouldn't it be better to put the checks in rcu_read_lock() and
> > > > > friends?  The problem I see with putting them in rcu_dereference_check()
> > > > > is that someone can legitimately do something like the following
> > > > > while in dyntick-idle mode:
> > > > > 
> > > > > 	spin_lock(&mylock);
> > > > > 	/* do a bunch of stuff */
> > > > > 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> > > > > 
> > > > > The logic below would complain about this usage, despite the fact
> > > > > that it is perfectly safe because the update-side lock is held.
> > > > > 
> > > > > Make sense, or am I missing something?
> > > > > 
> > > > > 						Thanx, Paul
> > > > 
> > > > I'm an idiot. I put my check in rcu_dereference_check() on purpose because
> > > > it's always called from places that check one of the rcu locks are held,
> > > > but I forgot that's also used for custom conditions with the _check()
> > > > things.
> > > > 
> > > > That said, putting the check in rcu_read_lock() and alike  would only work
> > > > with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
> > > > call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
> > > > checks there as it's fine to disabled irqs in dyntick idle.
> > > > 
> > > > What about the below? (untested yet)
> > > > 
> > > > And I would print the state of dynticks-idle mode in the final lockdep warning.
> > > 
> > > Printing the dynticks-idle mode would be quite good!
> > > 
> > > However, it is possible to have an RCU read-side critical section that does
> > > not have an rcu_dereference() or an rcu_read_lock_held().  So I do believe
> > > that we really do need rcu_read_lock() and friends to do this checking.
> > 
> > Right, then we need to check everything: rcu_read_lock() and friends in case
> > we have no rcu_read_lock_held() check made (ie: no rcu_dereference_check()),
> > but also rcu_read_lock_held()/rcu_read_lock_sched_held()/... because preempt_disable(),
> > local_irq_disable(), local_bh_disable() can't be checked so for rcu sched and rcu bh
> > we can only check the ...held() things.
> 
> Good point.  To make sure I understand, we have different approaches
> for the different types of RCU.  We need to instrument the following
> primitives:
> 
> 1.	rcu_read_lock() because a stray rcu_dereference() will already
> 	be caught by PROVE_RCU.
> 2.	rcu_read_lock_bh_held() because it is OK to do local_bh_disable()
> 	in dyntick-idle mode, so we cannot prohibit all of the
> 	read-acquisition cases.  We can also instrument rcu_read_lock_bh()
> 	to catch RCU-bh read-side critical sections that don't happen
> 	to contain rcu_dereference_bh().
> 3.	rcu_read_lock_sched_held() because it is OK to do preempt_disable()
> 	and local_irq_save() from dyntick-idle mode, so we again
> 	cannot prohibit all of the read-acquisition cases.  We can
> 	also instrument rcu_read_lock_sched() to catch RCU-sched
> 	read-side critical sections that don't happen to contain
> 	rcu_dereference_sched().

Exactly!

> 4.	srcu_read_lock() because a stray srcu_dereference() will already
> 	by caught by PROVE_RCU.

Well I have no idea how srcu works so I'll first focus on the rcu part :)

> 
> We miss a few cases, for example, an RCU-sched read-side critical
> section that uses local_irq_disable(), but that also does not contain
> an rcu_dereference_sched().  But still this sounds quite worthwhile.

Right.

So I'll send a v3 that takes the above point into accounts.

Thanks.

Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

[Paul E. McKenney]

On Sat, Jun 18, 2011 at 06:10:36PM +0200, Frederic Weisbecker wrote:
> On Sat, Jun 18, 2011 at 09:04:27AM -0700, Paul E. McKenney wrote:
> > On Sat, Jun 18, 2011 at 04:23:58PM +0200, Frederic Weisbecker wrote:
> > > On Fri, Jun 17, 2011 at 04:19:03PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote:
> > > > > On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> > > > > > > Detect uses of rcu that are not supposed to happen when we
> > > > > > > are in an extended quiescent state.
> > > > > > > 
> > > > > > > This can happen for example if we use rcu between the time we
> > > > > > > stop the tick and the time we restart it. Or inside an irq that
> > > > > > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> > > > > > > misuse.
> > > > > > > 
> > > > > > > v2: Rebase against latest rcu changes, handle tiny RCU as well
> > > > > > 
> > > > > > Good idea on checking for RCU read-side critical sections happening
> > > > > > in dyntick-idle periods!
> > > > > > 
> > > > > > But wouldn't it be better to put the checks in rcu_read_lock() and
> > > > > > friends?  The problem I see with putting them in rcu_dereference_check()
> > > > > > is that someone can legitimately do something like the following
> > > > > > while in dyntick-idle mode:
> > > > > > 
> > > > > > 	spin_lock(&mylock);
> > > > > > 	/* do a bunch of stuff */
> > > > > > 	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));
> > > > > > 
> > > > > > The logic below would complain about this usage, despite the fact
> > > > > > that it is perfectly safe because the update-side lock is held.
> > > > > > 
> > > > > > Make sense, or am I missing something?
> > > > > > 
> > > > > > 						Thanx, Paul
> > > > > 
> > > > > I'm an idiot. I put my check in rcu_dereference_check() on purpose because
> > > > > it's always called from places that check one of the rcu locks are held,
> > > > > but I forgot that's also used for custom conditions with the _check()
> > > > > things.
> > > > > 
> > > > > That said, putting the check in rcu_read_lock() and alike  would only work
> > > > > with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually
> > > > > call it explicitely but rely on irq disabled or preempt disabled. And I can't put the
> > > > > checks there as it's fine to disabled irqs in dyntick idle.
> > > > > 
> > > > > What about the below? (untested yet)
> > > > > 
> > > > > And I would print the state of dynticks-idle mode in the final lockdep warning.
> > > > 
> > > > Printing the dynticks-idle mode would be quite good!
> > > > 
> > > > However, it is possible to have an RCU read-side critical section that does
> > > > not have an rcu_dereference() or an rcu_read_lock_held().  So I do believe
> > > > that we really do need rcu_read_lock() and friends to do this checking.
> > > 
> > > Right, then we need to check everything: rcu_read_lock() and friends in case
> > > we have no rcu_read_lock_held() check made (ie: no rcu_dereference_check()),
> > > but also rcu_read_lock_held()/rcu_read_lock_sched_held()/... because preempt_disable(),
> > > local_irq_disable(), local_bh_disable() can't be checked so for rcu sched and rcu bh
> > > we can only check the ...held() things.
> > 
> > Good point.  To make sure I understand, we have different approaches
> > for the different types of RCU.  We need to instrument the following
> > primitives:
> > 
> > 1.	rcu_read_lock() because a stray rcu_dereference() will already
> > 	be caught by PROVE_RCU.
> > 2.	rcu_read_lock_bh_held() because it is OK to do local_bh_disable()
> > 	in dyntick-idle mode, so we cannot prohibit all of the
> > 	read-acquisition cases.  We can also instrument rcu_read_lock_bh()
> > 	to catch RCU-bh read-side critical sections that don't happen
> > 	to contain rcu_dereference_bh().
> > 3.	rcu_read_lock_sched_held() because it is OK to do preempt_disable()
> > 	and local_irq_save() from dyntick-idle mode, so we again
> > 	cannot prohibit all of the read-acquisition cases.  We can
> > 	also instrument rcu_read_lock_sched() to catch RCU-sched
> > 	read-side critical sections that don't happen to contain
> > 	rcu_dereference_sched().
> 
> Exactly!
> 
> > 4.	srcu_read_lock() because a stray srcu_dereference() will already
> > 	by caught by PROVE_RCU.
> 
> Well I have no idea how srcu works so I'll first focus on the rcu part :)

Fair enough!  ;-)

> > We miss a few cases, for example, an RCU-sched read-side critical
> > section that uses local_irq_disable(), but that also does not contain
> > an rcu_dereference_sched().  But still this sounds quite worthwhile.
> 
> Right.
> 
> So I'll send a v3 that takes the above point into accounts.

Looking forward to seeing it!

							Thanx, Paul