[PATCH v2 00/11] TRACE_IRQFLAGS wreckage

[PATCH v2 00/11] TRACE_IRQFLAGS wreckage

[Peter Zijlstra]

TRACE_IRQFLAGS

local_irq_*() keeps a software state that mirrors the hardware state,
used for lockdep, includes tracepoints.

raw_local_irq_*() does not update the software state, no tracing.

---

Problem 1:

	raw_local_irq_save(); // software state on
	local_irq_save(); // software state off
	...
	local_irq_restore(); // software state still off, because we don't enable IRQs
	raw_local_irq_restore(); // software state still off, *whoopsie*

existing instances:

 - lock_acquire()
     raw_local_irq_save()
     __lock_acquire()
       arch_spin_lock(&graph_lock)
         pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
           local_irq_save()

 - trace_clock_global()
     raw_local_irq_save()
     arch_spin_lock()
       pv_wait() := kvm_wait()
	 local_irq_save()

 - apic_retrigger_irq()
     raw_local_irq_save()
     apic->send_IPI() := default_send_IPI_single_phys()
       local_irq_save()

Possible solutions:

 A) make it work by enabling the tracing inside raw_*()
 B) make it work by keeping tracing disabled inside raw_*()
 C) call it broken and clean it up now

Now, given that the only reason to use the raw_* variant is because you don't
want tracing. Therefore A) seems like a weird option (although it can be done).
C) is tempting, but OTOH it ends up converting a _lot_ of code to raw just
because there is one raw user, this strips the validation/tracing off for all
the other users.

So we pick B) and declare any code that ends up doing:

	raw_local_irq_save()
	local_irq_save()
	lockdep_assert_irqs_disabled();

broken. AFAICT this problem has existed forever, the only reason it came
up is because I changed IRQ tracing vs lockdep recursion and the first
instance is fairly common, the other cases hardly ever happen.

---

Problem 2:

	raw_local_irq_save(); // software state on
	trace_*()
          ...
          perf_tp_event()
            ...
            perf_callchain()
	    <#PF>
	      trace_hardirqs_off(); // software state off
	      ...
	      if (regs_irqs_disabled(regs)) // false
	        trace_hardirqs_on();
	    </#PF>
	raw_local_irq_restore(); // software state stays off, *whoopsie*

existing instances:

 - lock_acquire() / lock_release()
     raw_local_irq_save()
     trace_lock_acquire() / trace_lock_release()

 - function tracing

Possible solutions:

 A) fix every architecture's entry code
 B) only fix kernel/entry/common.c
 C) fix lockdep tracepoints and pray

This series does C, AFAICT this problem has existed forever.

---

Problem 3:

	raw_local_irq_save(); // software state on
	<#NMI>
	  trace_hardirqs_off(); // software state off
	  ...
	  if (regs_irqs_disabled(regs)) // false
	    trace_hardirqs_on();
	</#NMI>
	raw_local_irq_restore(); // software state stays off, *whoopsie*

Possible solutions:

This *should* not be a problem if an architecture has it's entry ordering
right. In particular we rely on the architecture doing nmi_enter() before
trace_hardirqs_off().

In that case, in_nmi() will be true, and lockdep_hardirqs_*() should NO-OP,
except if CONFIG_TRACE_IRQFLAGS_NMI (x86).

There might be a problem with using lockdep_assert_irqs_disabled() from NMI
context, if so, those needs a little TLC.

---

The patches in this series do (in reverse order):

 - 2C
 - 1B
 - fix fallout in idle due to the trace_lock_*() tracepoints suddenly
   being visible to rcu-lockdep.

---

Change since -v1:

 - typo (rostedt)
 - split WARN (rostedt)
 - reorder start_critical_section / rcu_idle_enter (rostedt)
 - added arm64 patch (kernel test robot)

---

 arch/arm/mach-omap2/pm34xx.c      |    4 --
 arch/arm64/include/asm/irqflags.h |    5 ++
 arch/arm64/kernel/process.c       |    2 -
 arch/nds32/include/asm/irqflags.h |    5 ++
 arch/powerpc/include/asm/hw_irq.h |   11 ++---
 arch/s390/kernel/idle.c           |    3 -
 arch/x86/entry/thunk_32.S         |    5 --
 arch/x86/include/asm/mmu.h        |    1 
 arch/x86/kernel/process.c         |    4 --
 arch/x86/mm/tlb.c                 |   13 +-----
 drivers/cpuidle/cpuidle.c         |   19 +++++++--
 drivers/idle/intel_idle.c         |   16 --------
 include/linux/cpuidle.h           |   13 +++---
 include/linux/irqflags.h          |   73 ++++++++++++++++++++------------------
 include/linux/lockdep.h           |   18 ++++++---
 include/linux/mmu_context.h       |    5 ++
 kernel/locking/lockdep.c          |   18 +++++----
 kernel/sched/idle.c               |   25 +++++--------
 18 files changed, 118 insertions(+), 122 deletions(-)

[PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables

[Peter Zijlstra]

Sven reported that commit a21ee6055c30 ("lockdep: Change
hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
s390 because their this_cpu_*() primitives disable preemption which
then lands back tracing.

On the one hand, per-cpu ops should use preempt_*able_notrace() and
raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
ops for this.

Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/irqflags.h |    6 +++---
 include/linux/lockdep.h  |   18 +++++++++++++-----
 kernel/locking/lockdep.c |    4 ++--
 3 files changed, 18 insertions(+), 10 deletions(-)

--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
   extern void trace_hardirqs_off_finish(void);
   extern void trace_hardirqs_on(void);
   extern void trace_hardirqs_off(void);
-# define lockdep_hardirq_context()	(this_cpu_read(hardirq_context))
+# define lockdep_hardirq_context()	(raw_cpu_read(hardirq_context))
 # define lockdep_softirq_context(p)	((p)->softirq_context)
 # define lockdep_hardirqs_enabled()	(this_cpu_read(hardirqs_enabled))
 # define lockdep_softirqs_enabled(p)	((p)->softirqs_enabled)
 # define lockdep_hardirq_enter()			\
 do {							\
-	if (this_cpu_inc_return(hardirq_context) == 1)	\
+	if (__this_cpu_inc_return(hardirq_context) == 1)\
 		current->hardirq_threaded = 0;		\
 } while (0)
 # define lockdep_hardirq_threaded()		\
@@ -68,7 +68,7 @@ do {						\
 } while (0)
 # define lockdep_hardirq_exit()			\
 do {						\
-	this_cpu_dec(hardirq_context);		\
+	__this_cpu_dec(hardirq_context);	\
 } while (0)
 # define lockdep_softirq_enter()		\
 do {						\
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -535,19 +535,27 @@ do {									\
 DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 
+/*
+ * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
+ * per-cpu variables. This is required because this_cpu_read() will potentially
+ * call into preempt/irq-disable and that obviously isn't right. This is also
+ * correct because when IRQs are enabled, it doesn't matter if we accidentally
+ * read the value from our previous CPU.
+ */
+
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled));	\
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context));	\
+	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context));	\
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
@@ -555,7 +563,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     debug_locks			&&		\
 		     (preempt_count() != 0		||		\
-		      !this_cpu_read(hardirqs_enabled)));		\
+		      !raw_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #define lockdep_assert_preemption_disabled()				\
@@ -563,7 +571,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     debug_locks			&&		\
 		     (preempt_count() == 0		&&		\
-		      this_cpu_read(hardirqs_enabled)));		\
+		      raw_cpu_read(hardirqs_enabled)));			\
 } while (0)
 
 #else
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigne
 
 skip_checks:
 	/* we'll do an OFF -> ON transition: */
-	this_cpu_write(hardirqs_enabled, 1);
+	__this_cpu_write(hardirqs_enabled, 1);
 	trace->hardirq_enable_ip = ip;
 	trace->hardirq_enable_event = ++trace->irq_events;
 	debug_atomic_inc(hardirqs_on_events);
@@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsign
 		/*
 		 * We have done an ON -> OFF transition:
 		 */
-		this_cpu_write(hardirqs_enabled, 0);
+		__this_cpu_write(hardirqs_enabled, 0);
 		trace->hardirq_disable_ip = ip;
 		trace->hardirq_disable_event = ++trace->irq_events;
 		debug_atomic_inc(hardirqs_off_events);

Re: [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables

[Joel Fernandes]

On Fri, Aug 21, 2020 at 10:47:39AM +0200, Peter Zijlstra wrote:
> Sven reported that commit a21ee6055c30 ("lockdep: Change
> hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
> s390 because their this_cpu_*() primitives disable preemption which
> then lands back tracing.
> 
> On the one hand, per-cpu ops should use preempt_*able_notrace() and
> raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
> ops for this.
> 
> Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
> Reported-by: Sven Schnelle <svens@linux.ibm.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/irqflags.h |    6 +++---
>  include/linux/lockdep.h  |   18 +++++++++++++-----
>  kernel/locking/lockdep.c |    4 ++--
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
>    extern void trace_hardirqs_off_finish(void);
>    extern void trace_hardirqs_on(void);
>    extern void trace_hardirqs_off(void);
> -# define lockdep_hardirq_context()	(this_cpu_read(hardirq_context))
> +# define lockdep_hardirq_context()	(raw_cpu_read(hardirq_context))
>  # define lockdep_softirq_context(p)	((p)->softirq_context)
>  # define lockdep_hardirqs_enabled()	(this_cpu_read(hardirqs_enabled))
>  # define lockdep_softirqs_enabled(p)	((p)->softirqs_enabled)
>  # define lockdep_hardirq_enter()			\
>  do {							\
> -	if (this_cpu_inc_return(hardirq_context) == 1)	\
> +	if (__this_cpu_inc_return(hardirq_context) == 1)\
>  		current->hardirq_threaded = 0;		\
>  } while (0)
>  # define lockdep_hardirq_threaded()		\
> @@ -68,7 +68,7 @@ do {						\
>  } while (0)
>  # define lockdep_hardirq_exit()			\
>  do {						\
> -	this_cpu_dec(hardirq_context);		\
> +	__this_cpu_dec(hardirq_context);	\
>  } while (0)
>  # define lockdep_softirq_enter()		\
>  do {						\
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -535,19 +535,27 @@ do {									\
>  DECLARE_PER_CPU(int, hardirqs_enabled);
>  DECLARE_PER_CPU(int, hardirq_context);
>  
> +/*
> + * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
> + * per-cpu variables. This is required because this_cpu_read() will potentially
> + * call into preempt/irq-disable and that obviously isn't right. This is also
> + * correct because when IRQs are enabled, it doesn't matter if we accidentally
> + * read the value from our previous CPU.
> + */
> +
>  #define lockdep_assert_irqs_enabled()					\
>  do {									\
> -	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled));	\
> +	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled));	\
>  } while (0)
>  
>  #define lockdep_assert_irqs_disabled()					\
>  do {									\
> -	WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled));	\
> +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
>  } while (0)
>  
>  #define lockdep_assert_in_irq()						\
>  do {									\
> -	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context));	\
> +	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context));	\
>  } while (0)
>  
>  #define lockdep_assert_preemption_enabled()				\
> @@ -555,7 +563,7 @@ do {									\
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
>  		     debug_locks			&&		\
>  		     (preempt_count() != 0		||		\
> -		      !this_cpu_read(hardirqs_enabled)));		\
> +		      !raw_cpu_read(hardirqs_enabled)));		\
>  } while (0)
>  
>  #define lockdep_assert_preemption_disabled()				\
> @@ -563,7 +571,7 @@ do {									\
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
>  		     debug_locks			&&		\
>  		     (preempt_count() == 0		&&		\
> -		      this_cpu_read(hardirqs_enabled)));		\
> +		      raw_cpu_read(hardirqs_enabled)));			\
>  } while (0)
>  
>  #else
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigne
>  
>  skip_checks:
>  	/* we'll do an OFF -> ON transition: */
> -	this_cpu_write(hardirqs_enabled, 1);
> +	__this_cpu_write(hardirqs_enabled, 1);
>  	trace->hardirq_enable_ip = ip;
>  	trace->hardirq_enable_event = ++trace->irq_events;
>  	debug_atomic_inc(hardirqs_on_events);
> @@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsign
>  		/*
>  		 * We have done an ON -> OFF transition:
>  		 */
> -		this_cpu_write(hardirqs_enabled, 0);
> +		__this_cpu_write(hardirqs_enabled, 0);
>  		trace->hardirq_disable_ip = ip;
>  		trace->hardirq_disable_event = ++trace->irq_events;
>  		debug_atomic_inc(hardirqs_off_events);
> 
> 

[tip: locking/core] lockdep: Use raw_cpu_*() for per-cpu variables

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     fddf9055a60dfcc97bda5ef03c8fa4108ed555c5
Gitweb:        https://git.kernel.org/tip/fddf9055a60dfcc97bda5ef03c8fa4108ed555c5
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 20 Aug 2020 09:13:30 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00

lockdep: Use raw_cpu_*() for per-cpu variables

Sven reported that commit a21ee6055c30 ("lockdep: Change
hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
s390 because their this_cpu_*() primitives disable preemption which
then lands back tracing.

On the one hand, per-cpu ops should use preempt_*able_notrace() and
raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
ops for this.

Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200821085348.192346882@infradead.org
---
 include/linux/irqflags.h |  6 +++---
 include/linux/lockdep.h  | 18 +++++++++++++-----
 kernel/locking/lockdep.c |  4 ++--
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index bd5c557..d7e50a2 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
   extern void trace_hardirqs_off_finish(void);
   extern void trace_hardirqs_on(void);
   extern void trace_hardirqs_off(void);
-# define lockdep_hardirq_context()	(this_cpu_read(hardirq_context))
+# define lockdep_hardirq_context()	(raw_cpu_read(hardirq_context))
 # define lockdep_softirq_context(p)	((p)->softirq_context)
 # define lockdep_hardirqs_enabled()	(this_cpu_read(hardirqs_enabled))
 # define lockdep_softirqs_enabled(p)	((p)->softirqs_enabled)
 # define lockdep_hardirq_enter()			\
 do {							\
-	if (this_cpu_inc_return(hardirq_context) == 1)	\
+	if (__this_cpu_inc_return(hardirq_context) == 1)\
 		current->hardirq_threaded = 0;		\
 } while (0)
 # define lockdep_hardirq_threaded()		\
@@ -68,7 +68,7 @@ do {						\
 } while (0)
 # define lockdep_hardirq_exit()			\
 do {						\
-	this_cpu_dec(hardirq_context);		\
+	__this_cpu_dec(hardirq_context);	\
 } while (0)
 # define lockdep_softirq_enter()		\
 do {						\
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 62a382d..6a584b3 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -535,19 +535,27 @@ do {									\
 DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 
+/*
+ * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
+ * per-cpu variables. This is required because this_cpu_read() will potentially
+ * call into preempt/irq-disable and that obviously isn't right. This is also
+ * correct because when IRQs are enabled, it doesn't matter if we accidentally
+ * read the value from our previous CPU.
+ */
+
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled));	\
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context));	\
+	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context));	\
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
@@ -555,7 +563,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     debug_locks			&&		\
 		     (preempt_count() != 0		||		\
-		      !this_cpu_read(hardirqs_enabled)));		\
+		      !raw_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #define lockdep_assert_preemption_disabled()				\
@@ -563,7 +571,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     debug_locks			&&		\
 		     (preempt_count() == 0		&&		\
-		      this_cpu_read(hardirqs_enabled)));		\
+		      raw_cpu_read(hardirqs_enabled)));			\
 } while (0)
 
 #else
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 2fad21d..c872e95 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigned long ip)
 
 skip_checks:
 	/* we'll do an OFF -> ON transition: */
-	this_cpu_write(hardirqs_enabled, 1);
+	__this_cpu_write(hardirqs_enabled, 1);
 	trace->hardirq_enable_ip = ip;
 	trace->hardirq_enable_event = ++trace->irq_events;
 	debug_atomic_inc(hardirqs_on_events);
@@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsigned long ip)
 		/*
 		 * We have done an ON -> OFF transition:
 		 */
-		this_cpu_write(hardirqs_enabled, 0);
+		__this_cpu_write(hardirqs_enabled, 0);
 		trace->hardirq_disable_ip = ip;
 		trace->hardirq_disable_event = ++trace->irq_events;
 		debug_atomic_inc(hardirqs_off_events);

[PATCH v2 02/11] cpuidle: Fixup IRQ state

[Peter Zijlstra]

Match the pattern elsewhere in this file.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 drivers/cpuidle/cpuidle.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
 	 */
 	stop_critical_timings();
 	drv->states[index].enter_s2idle(dev, drv, index);
-	WARN_ON(!irqs_disabled());
+	if (WARN_ON_ONCE(!irqs_disabled()))
+		local_irq_disable();
 	/*
 	 * timekeeping_resume() that will be called by tick_unfreeze() for the
 	 * first CPU executing it calls functions containing RCU read-side

Re: [PATCH v2 02/11] cpuidle: Fixup IRQ state

[Rafael J. Wysocki]

On Friday, August 21, 2020 10:47:40 AM CEST Peter Zijlstra wrote:
> Match the pattern elsewhere in this file.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>

For all patches in the series:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/cpuidle/cpuidle.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
>  	 */
>  	stop_critical_timings();
>  	drv->states[index].enter_s2idle(dev, drv, index);
> -	WARN_ON(!irqs_disabled());
> +	if (WARN_ON_ONCE(!irqs_disabled()))
> +		local_irq_disable();
>  	/*
>  	 * timekeeping_resume() that will be called by tick_unfreeze() for the
>  	 * first CPU executing it calls functions containing RCU read-side
> 
> 
> 

Re: [PATCH v2 02/11] cpuidle: Fixup IRQ state

[Joel Fernandes]

On Fri, Aug 21, 2020 at 07:36:43PM +0200, Rafael J. Wysocki wrote:
> On Friday, August 21, 2020 10:47:40 AM CEST Peter Zijlstra wrote:
> > Match the pattern elsewhere in this file.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Tested-by: Marco Elver <elver@google.com>
> 
> For all patches in the series:
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
> > ---
> >  drivers/cpuidle/cpuidle.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
> >  	 */
> >  	stop_critical_timings();
> >  	drv->states[index].enter_s2idle(dev, drv, index);
> > -	WARN_ON(!irqs_disabled());
> > +	if (WARN_ON_ONCE(!irqs_disabled()))
> > +		local_irq_disable();
> >  	/*
> >  	 * timekeeping_resume() that will be called by tick_unfreeze() for the
> >  	 * first CPU executing it calls functions containing RCU read-side
> > 
> > 
> > 
> 
> 
> 
> 

[tip: locking/core] cpuidle: Fixup IRQ state

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     49d9c5936314e44d314c605c39cce0fd947f9c3a
Gitweb:        https://git.kernel.org/tip/49d9c5936314e44d314c605c39cce0fd947f9c3a
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 20 Aug 2020 16:47:24 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00

cpuidle: Fixup IRQ state

Match the pattern elsewhere in this file.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.251340558@infradead.org
---
 drivers/cpuidle/cpuidle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8719731..2fe4f3c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
 	 */
 	stop_critical_timings();
 	drv->states[index].enter_s2idle(dev, drv, index);
-	WARN_ON(!irqs_disabled());
+	if (WARN_ON_ONCE(!irqs_disabled()))
+		local_irq_disable();
 	/*
 	 * timekeeping_resume() that will be called by tick_unfreeze() for the
 	 * first CPU executing it calls functions containing RCU read-side

[PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[Peter Zijlstra]

Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
that the locking tracepoints were using RCU.

Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.

Specifically, sched_clock_idle_wakeup_event() will use ktime which
will use seqlocks which will tickle lockdep, and
stop_critical_timings() uses lock.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 drivers/cpuidle/cpuidle.c |   12 ++++++++----
 kernel/sched/idle.c       |   22 ++++++++--------------
 2 files changed, 16 insertions(+), 18 deletions(-)

--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
 	 * executing it contains RCU usage regarded as invalid in the idle
 	 * context, so tell RCU about that.
 	 */
-	RCU_NONIDLE(tick_freeze());
+	tick_freeze();
 	/*
 	 * The state used here cannot be a "coupled" one, because the "coupled"
 	 * cpuidle mechanism enables interrupts and doing that with timekeeping
 	 * suspended is generally unsafe.
 	 */
 	stop_critical_timings();
+	rcu_idle_enter();
 	drv->states[index].enter_s2idle(dev, drv, index);
 	if (WARN_ON_ONCE(!irqs_disabled()))
 		local_irq_disable();
@@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
 	 * first CPU executing it calls functions containing RCU read-side
 	 * critical sections, so tell RCU about that.
 	 */
-	RCU_NONIDLE(tick_unfreeze());
+	rcu_idle_exit();
+	tick_unfreeze();
 	start_critical_timings();
 
 	time_end = ns_to_ktime(local_clock());
@@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
 	/* Take note of the planned idle state. */
 	sched_idle_set_state(target_state);
 
-	trace_cpu_idle_rcuidle(index, dev->cpu);
+	trace_cpu_idle(index, dev->cpu);
 	time_start = ns_to_ktime(local_clock());
 
 	stop_critical_timings();
+	rcu_idle_enter();
 	entered_state = target_state->enter(dev, drv, index);
+	rcu_idle_exit();
 	start_critical_timings();
 
 	sched_clock_idle_wakeup_event();
 	time_end = ns_to_ktime(local_clock());
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 
 	/* The cpu is no longer idle or about to enter idle. */
 	sched_idle_set_state(NULL);
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
 
 static noinline int __cpuidle cpu_idle_poll(void)
 {
+	trace_cpu_idle(0, smp_processor_id());
+	stop_critical_timings();
 	rcu_idle_enter();
-	trace_cpu_idle_rcuidle(0, smp_processor_id());
 	local_irq_enable();
-	stop_critical_timings();
 
 	while (!tif_need_resched() &&
-		(cpu_idle_force_poll || tick_check_broadcast_expired()))
+	       (cpu_idle_force_poll || tick_check_broadcast_expired()))
 		cpu_relax();
-	start_critical_timings();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
+
 	rcu_idle_exit();
+	start_critical_timings();
+	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 
 	return 1;
 }
@@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
 		local_irq_enable();
 	} else {
 		stop_critical_timings();
+		rcu_idle_enter();
 		arch_cpu_idle();
+		rcu_idle_exit();
 		start_critical_timings();
 	}
 }
@@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
 
 	if (cpuidle_not_available(drv, dev)) {
 		tick_nohz_idle_stop_tick();
-		rcu_idle_enter();
 
 		default_idle_call();
 		goto exit_idle;
@@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
 		u64 max_latency_ns;
 
 		if (idle_should_enter_s2idle()) {
-			rcu_idle_enter();
 
 			entered_state = call_cpuidle_s2idle(drv, dev);
 			if (entered_state > 0)
 				goto exit_idle;
 
-			rcu_idle_exit();
-
 			max_latency_ns = U64_MAX;
 		} else {
 			max_latency_ns = dev->forced_idle_latency_limit_ns;
 		}
 
 		tick_nohz_idle_stop_tick();
-		rcu_idle_enter();
 
 		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
 		call_cpuidle(drv, dev, next_state);
@@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
 		else
 			tick_nohz_idle_retain_tick();
 
-		rcu_idle_enter();
-
 		entered_state = call_cpuidle(drv, dev, next_state);
 		/*
 		 * Give the governor an opportunity to reflect on the outcome
@@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
 	 */
 	if (WARN_ON_ONCE(irqs_disabled()))
 		local_irq_enable();
-
-	rcu_idle_exit();
 }
 
 /*

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[Joel Fernandes]

On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> that the locking tracepoints were using RCU.
> 
> Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> 
> Specifically, sched_clock_idle_wakeup_event() will use ktime which
> will use seqlocks which will tickle lockdep, and
> stop_critical_timings() uses lock.

I was wondering if those tracepoints should just use _rcuidle variant of the
trace call. But that's a terrible idea considering that would add unwanted
overhead I think.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>
> ---
>  drivers/cpuidle/cpuidle.c |   12 ++++++++----
>  kernel/sched/idle.c       |   22 ++++++++--------------
>  2 files changed, 16 insertions(+), 18 deletions(-)
> 
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
>  	 * executing it contains RCU usage regarded as invalid in the idle
>  	 * context, so tell RCU about that.
>  	 */
> -	RCU_NONIDLE(tick_freeze());
> +	tick_freeze();
>  	/*
>  	 * The state used here cannot be a "coupled" one, because the "coupled"
>  	 * cpuidle mechanism enables interrupts and doing that with timekeeping
>  	 * suspended is generally unsafe.
>  	 */
>  	stop_critical_timings();
> +	rcu_idle_enter();
>  	drv->states[index].enter_s2idle(dev, drv, index);
>  	if (WARN_ON_ONCE(!irqs_disabled()))
>  		local_irq_disable();
> @@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
>  	 * first CPU executing it calls functions containing RCU read-side
>  	 * critical sections, so tell RCU about that.
>  	 */
> -	RCU_NONIDLE(tick_unfreeze());
> +	rcu_idle_exit();
> +	tick_unfreeze();
>  	start_critical_timings();
>  
>  	time_end = ns_to_ktime(local_clock());
> @@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
>  	/* Take note of the planned idle state. */
>  	sched_idle_set_state(target_state);
>  
> -	trace_cpu_idle_rcuidle(index, dev->cpu);
> +	trace_cpu_idle(index, dev->cpu);
>  	time_start = ns_to_ktime(local_clock());
>  
>  	stop_critical_timings();
> +	rcu_idle_enter();
>  	entered_state = target_state->enter(dev, drv, index);
> +	rcu_idle_exit();
>  	start_critical_timings();
>  
>  	sched_clock_idle_wakeup_event();
>  	time_end = ns_to_ktime(local_clock());
> -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> +	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
>  
>  	/* The cpu is no longer idle or about to enter idle. */
>  	sched_idle_set_state(NULL);
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
>  
>  static noinline int __cpuidle cpu_idle_poll(void)
>  {
> +	trace_cpu_idle(0, smp_processor_id());
> +	stop_critical_timings();
>  	rcu_idle_enter();
> -	trace_cpu_idle_rcuidle(0, smp_processor_id());
>  	local_irq_enable();
> -	stop_critical_timings();
>  
>  	while (!tif_need_resched() &&
> -		(cpu_idle_force_poll || tick_check_broadcast_expired()))
> +	       (cpu_idle_force_poll || tick_check_broadcast_expired()))
>  		cpu_relax();
> -	start_critical_timings();
> -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> +
>  	rcu_idle_exit();
> +	start_critical_timings();
> +	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
>  
>  	return 1;
>  }
> @@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
>  		local_irq_enable();
>  	} else {
>  		stop_critical_timings();
> +		rcu_idle_enter();
>  		arch_cpu_idle();
> +		rcu_idle_exit();
>  		start_critical_timings();
>  	}
>  }
> @@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
>  
>  	if (cpuidle_not_available(drv, dev)) {
>  		tick_nohz_idle_stop_tick();
> -		rcu_idle_enter();
>  
>  		default_idle_call();
>  		goto exit_idle;
> @@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
>  		u64 max_latency_ns;
>  
>  		if (idle_should_enter_s2idle()) {
> -			rcu_idle_enter();
>  
>  			entered_state = call_cpuidle_s2idle(drv, dev);
>  			if (entered_state > 0)
>  				goto exit_idle;
>  
> -			rcu_idle_exit();
> -
>  			max_latency_ns = U64_MAX;
>  		} else {
>  			max_latency_ns = dev->forced_idle_latency_limit_ns;
>  		}
>  
>  		tick_nohz_idle_stop_tick();
> -		rcu_idle_enter();
>  
>  		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
>  		call_cpuidle(drv, dev, next_state);
> @@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
>  		else
>  			tick_nohz_idle_retain_tick();
>  
> -		rcu_idle_enter();
> -
>  		entered_state = call_cpuidle(drv, dev, next_state);
>  		/*
>  		 * Give the governor an opportunity to reflect on the outcome
> @@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
>  	 */
>  	if (WARN_ON_ONCE(irqs_disabled()))
>  		local_irq_enable();
> -
> -	rcu_idle_exit();
>  }
>  
>  /*
> 
> 

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[Joel Fernandes]

On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > that the locking tracepoints were using RCU.
> > 
> > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> > 
> > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > will use seqlocks which will tickle lockdep, and
> > stop_critical_timings() uses lock.
> 
> I was wondering if those tracepoints should just use _rcuidle variant of the
> trace call. But that's a terrible idea considering that would add unwanted
> overhead I think.
> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
issues go away, no? That RCU flavor is always watching.

thanks,

 - Joel

> 
> thanks,
> 
>  - Joel
> 
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Tested-by: Marco Elver <elver@google.com>
> > ---
> >  drivers/cpuidle/cpuidle.c |   12 ++++++++----
> >  kernel/sched/idle.c       |   22 ++++++++--------------
> >  2 files changed, 16 insertions(+), 18 deletions(-)
> > 
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
> >  	 * executing it contains RCU usage regarded as invalid in the idle
> >  	 * context, so tell RCU about that.
> >  	 */
> > -	RCU_NONIDLE(tick_freeze());
> > +	tick_freeze();
> >  	/*
> >  	 * The state used here cannot be a "coupled" one, because the "coupled"
> >  	 * cpuidle mechanism enables interrupts and doing that with timekeeping
> >  	 * suspended is generally unsafe.
> >  	 */
> >  	stop_critical_timings();
> > +	rcu_idle_enter();
> >  	drv->states[index].enter_s2idle(dev, drv, index);
> >  	if (WARN_ON_ONCE(!irqs_disabled()))
> >  		local_irq_disable();
> > @@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
> >  	 * first CPU executing it calls functions containing RCU read-side
> >  	 * critical sections, so tell RCU about that.
> >  	 */
> > -	RCU_NONIDLE(tick_unfreeze());
> > +	rcu_idle_exit();
> > +	tick_unfreeze();
> >  	start_critical_timings();
> >  
> >  	time_end = ns_to_ktime(local_clock());
> > @@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
> >  	/* Take note of the planned idle state. */
> >  	sched_idle_set_state(target_state);
> >  
> > -	trace_cpu_idle_rcuidle(index, dev->cpu);
> > +	trace_cpu_idle(index, dev->cpu);
> >  	time_start = ns_to_ktime(local_clock());
> >  
> >  	stop_critical_timings();
> > +	rcu_idle_enter();
> >  	entered_state = target_state->enter(dev, drv, index);
> > +	rcu_idle_exit();
> >  	start_critical_timings();
> >  
> >  	sched_clock_idle_wakeup_event();
> >  	time_end = ns_to_ktime(local_clock());
> > -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> > +	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
> >  
> >  	/* The cpu is no longer idle or about to enter idle. */
> >  	sched_idle_set_state(NULL);
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
> >  
> >  static noinline int __cpuidle cpu_idle_poll(void)
> >  {
> > +	trace_cpu_idle(0, smp_processor_id());
> > +	stop_critical_timings();
> >  	rcu_idle_enter();
> > -	trace_cpu_idle_rcuidle(0, smp_processor_id());
> >  	local_irq_enable();
> > -	stop_critical_timings();
> >  
> >  	while (!tif_need_resched() &&
> > -		(cpu_idle_force_poll || tick_check_broadcast_expired()))
> > +	       (cpu_idle_force_poll || tick_check_broadcast_expired()))
> >  		cpu_relax();
> > -	start_critical_timings();
> > -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> > +
> >  	rcu_idle_exit();
> > +	start_critical_timings();
> > +	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> >  
> >  	return 1;
> >  }
> > @@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
> >  		local_irq_enable();
> >  	} else {
> >  		stop_critical_timings();
> > +		rcu_idle_enter();
> >  		arch_cpu_idle();
> > +		rcu_idle_exit();
> >  		start_critical_timings();
> >  	}
> >  }
> > @@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
> >  
> >  	if (cpuidle_not_available(drv, dev)) {
> >  		tick_nohz_idle_stop_tick();
> > -		rcu_idle_enter();
> >  
> >  		default_idle_call();
> >  		goto exit_idle;
> > @@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
> >  		u64 max_latency_ns;
> >  
> >  		if (idle_should_enter_s2idle()) {
> > -			rcu_idle_enter();
> >  
> >  			entered_state = call_cpuidle_s2idle(drv, dev);
> >  			if (entered_state > 0)
> >  				goto exit_idle;
> >  
> > -			rcu_idle_exit();
> > -
> >  			max_latency_ns = U64_MAX;
> >  		} else {
> >  			max_latency_ns = dev->forced_idle_latency_limit_ns;
> >  		}
> >  
> >  		tick_nohz_idle_stop_tick();
> > -		rcu_idle_enter();
> >  
> >  		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> >  		call_cpuidle(drv, dev, next_state);
> > @@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
> >  		else
> >  			tick_nohz_idle_retain_tick();
> >  
> > -		rcu_idle_enter();
> > -
> >  		entered_state = call_cpuidle(drv, dev, next_state);
> >  		/*
> >  		 * Give the governor an opportunity to reflect on the outcome
> > @@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
> >  	 */
> >  	if (WARN_ON_ONCE(irqs_disabled()))
> >  		local_irq_enable();
> > -
> > -	rcu_idle_exit();
> >  }
> >  
> >  /*
> > 
> > 

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[peterz]

On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
> On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > > that the locking tracepoints were using RCU.
> > > 
> > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> > > 
> > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > > will use seqlocks which will tickle lockdep, and
> > > stop_critical_timings() uses lock.
> > 
> > I was wondering if those tracepoints should just use _rcuidle variant of the
> > trace call. But that's a terrible idea considering that would add unwanted
> > overhead I think.
> > 
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
> issues go away, no? That RCU flavor is always watching.

All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.

Ideally RCU-trace goes away too.

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[Thomas Gleixner]

On Thu, Aug 27 2020 at 09:47, peterz wrote:
> On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
>> On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
>> > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
>> > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
>> > > that the locking tracepoints were using RCU.
>> > > 
>> > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
>> > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
>> > > 
>> > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
>> > > will use seqlocks which will tickle lockdep, and
>> > > stop_critical_timings() uses lock.
>> > 
>> > I was wondering if those tracepoints should just use _rcuidle variant of the
>> > trace call. But that's a terrible idea considering that would add unwanted
>> > overhead I think.
>> > 
>> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> 
>> BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
>> issues go away, no? That RCU flavor is always watching.
>
> All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.

It's the same problem as low level entry/exit. And that stuff is a hack
which papers over the problem instead of fixing it from ground up. But
we are talking about tracing, right?

Thanks,

        tglx

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[Joel Fernandes]

On Thu, Aug 27, 2020 at 09:47:48AM +0200, peterz@infradead.org wrote:
> On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> > > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > > > that the locking tracepoints were using RCU.
> > > > 
> > > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> > > > 
> > > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > > > will use seqlocks which will tickle lockdep, and
> > > > stop_critical_timings() uses lock.
> > > 
> > > I was wondering if those tracepoints should just use _rcuidle variant of the
> > > trace call. But that's a terrible idea considering that would add unwanted
> > > overhead I think.
> > > 
> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
> > issues go away, no? That RCU flavor is always watching.
> 
> All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
> 
> Ideally RCU-trace goes away too.

I was thinking that unless the rcu_idle_enter/exit calls coincide with points
in the kernel where instrumentation is not allowed, there is always a chance
somebody wants to use tracepoints after rcu_idle_enter or before exit. In
this case, trace_*_rcuidle() is unavoidable unless you can move the
rcu_idle_enter/exit calls deeper as you are doing.

Maybe objtool can help with that?

The other solution is RCU-trace if you can't push the rcu_idle_enter/exit
calls any deeper and still want to get rid of trace_*_rcuidle thingies. Me
and Mathieu were talking at LPC about tracepoint conversion to RCU-trace and
we can work on it if that's the right direction.

thanks,

 - Joel

Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path

[peterz]

On Thu, Aug 27, 2020 at 06:30:01PM -0400, Joel Fernandes wrote:
> On Thu, Aug 27, 2020 at 09:47:48AM +0200, peterz@infradead.org wrote:

> > All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
> > 
> > Ideally RCU-trace goes away too.
> 
> I was thinking that unless the rcu_idle_enter/exit calls coincide with points
> in the kernel where instrumentation is not allowed, there is always a chance
> somebody wants to use tracepoints after rcu_idle_enter or before exit. In
> this case, trace_*_rcuidle() is unavoidable unless you can move the
> rcu_idle_enter/exit calls deeper as you are doing.
> 
> Maybe objtool can help with that?

The eventual goal is to mark idle code noinstr too. There's more work
before we can do that.

[tip: locking/core] sched,idle,rcu: Push rcu_idle deeper into the idle path

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     1098582a0f6c4e8fd28da0a6305f9233d02c9c1d
Gitweb:        https://git.kernel.org/tip/1098582a0f6c4e8fd28da0a6305f9233d02c9c1d
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Fri, 07 Aug 2020 20:50:19 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00

sched,idle,rcu: Push rcu_idle deeper into the idle path

Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
that the locking tracepoints were using RCU.

Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.

Specifically, sched_clock_idle_wakeup_event() will use ktime which
will use seqlocks which will tickle lockdep, and
stop_critical_timings() uses lock.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.310943801@infradead.org
---
 drivers/cpuidle/cpuidle.c | 12 ++++++++----
 kernel/sched/idle.c       | 22 ++++++++--------------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2fe4f3c..9bcda41 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
 	 * executing it contains RCU usage regarded as invalid in the idle
 	 * context, so tell RCU about that.
 	 */
-	RCU_NONIDLE(tick_freeze());
+	tick_freeze();
 	/*
 	 * The state used here cannot be a "coupled" one, because the "coupled"
 	 * cpuidle mechanism enables interrupts and doing that with timekeeping
 	 * suspended is generally unsafe.
 	 */
 	stop_critical_timings();
+	rcu_idle_enter();
 	drv->states[index].enter_s2idle(dev, drv, index);
 	if (WARN_ON_ONCE(!irqs_disabled()))
 		local_irq_disable();
@@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
 	 * first CPU executing it calls functions containing RCU read-side
 	 * critical sections, so tell RCU about that.
 	 */
-	RCU_NONIDLE(tick_unfreeze());
+	rcu_idle_exit();
+	tick_unfreeze();
 	start_critical_timings();
 
 	time_end = ns_to_ktime(local_clock());
@@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	/* Take note of the planned idle state. */
 	sched_idle_set_state(target_state);
 
-	trace_cpu_idle_rcuidle(index, dev->cpu);
+	trace_cpu_idle(index, dev->cpu);
 	time_start = ns_to_ktime(local_clock());
 
 	stop_critical_timings();
+	rcu_idle_enter();
 	entered_state = target_state->enter(dev, drv, index);
+	rcu_idle_exit();
 	start_critical_timings();
 
 	sched_clock_idle_wakeup_event();
 	time_end = ns_to_ktime(local_clock());
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 
 	/* The cpu is no longer idle or about to enter idle. */
 	sched_idle_set_state(NULL);
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 6bf3498..ea3a098 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
 
 static noinline int __cpuidle cpu_idle_poll(void)
 {
+	trace_cpu_idle(0, smp_processor_id());
+	stop_critical_timings();
 	rcu_idle_enter();
-	trace_cpu_idle_rcuidle(0, smp_processor_id());
 	local_irq_enable();
-	stop_critical_timings();
 
 	while (!tif_need_resched() &&
-		(cpu_idle_force_poll || tick_check_broadcast_expired()))
+	       (cpu_idle_force_poll || tick_check_broadcast_expired()))
 		cpu_relax();
-	start_critical_timings();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
+
 	rcu_idle_exit();
+	start_critical_timings();
+	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 
 	return 1;
 }
@@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
 		local_irq_enable();
 	} else {
 		stop_critical_timings();
+		rcu_idle_enter();
 		arch_cpu_idle();
+		rcu_idle_exit();
 		start_critical_timings();
 	}
 }
@@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
 
 	if (cpuidle_not_available(drv, dev)) {
 		tick_nohz_idle_stop_tick();
-		rcu_idle_enter();
 
 		default_idle_call();
 		goto exit_idle;
@@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
 		u64 max_latency_ns;
 
 		if (idle_should_enter_s2idle()) {
-			rcu_idle_enter();
 
 			entered_state = call_cpuidle_s2idle(drv, dev);
 			if (entered_state > 0)
 				goto exit_idle;
 
-			rcu_idle_exit();
-
 			max_latency_ns = U64_MAX;
 		} else {
 			max_latency_ns = dev->forced_idle_latency_limit_ns;
 		}
 
 		tick_nohz_idle_stop_tick();
-		rcu_idle_enter();
 
 		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
 		call_cpuidle(drv, dev, next_state);
@@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
 		else
 			tick_nohz_idle_retain_tick();
 
-		rcu_idle_enter();
-
 		entered_state = call_cpuidle(drv, dev, next_state);
 		/*
 		 * Give the governor an opportunity to reflect on the outcome
@@ -226,8 +222,6 @@ exit_idle:
 	 */
 	if (WARN_ON_ONCE(irqs_disabled()))
 		local_irq_enable();
-
-	rcu_idle_exit();
 }
 
 /*

[PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic

[Peter Zijlstra]

This allows moving the leave_mm() call into generic code before
rcu_idle_enter(). Gets rid of more trace_*_rcuidle() users.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 arch/x86/include/asm/mmu.h  |    1 +
 arch/x86/mm/tlb.c           |   13 ++-----------
 drivers/cpuidle/cpuidle.c   |    4 ++++
 drivers/idle/intel_idle.c   |   16 ----------------
 include/linux/cpuidle.h     |   13 +++++++------
 include/linux/mmu_context.h |    5 +++++
 6 files changed, 19 insertions(+), 33 deletions(-)

--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -59,5 +59,6 @@ typedef struct {
 	}
 
 void leave_mm(int cpu);
+#define leave_mm leave_mm
 
 #endif /* _ASM_X86_MMU_H */
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -555,21 +555,12 @@ void switch_mm_irqs_off(struct mm_struct
 		this_cpu_write(cpu_tlbstate.ctxs[new_asid].tlb_gen, next_tlb_gen);
 		load_new_mm_cr3(next->pgd, new_asid, true);
 
-		/*
-		 * NB: This gets called via leave_mm() in the idle path
-		 * where RCU functions differently.  Tracing normally
-		 * uses RCU, so we need to use the _rcuidle variant.
-		 *
-		 * (There is no good reason for this.  The idle code should
-		 *  be rearranged to call this before rcu_idle_enter().)
-		 */
-		trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+		trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
 	} else {
 		/* The new ASID is already up to date. */
 		load_new_mm_cr3(next->pgd, new_asid, false);
 
-		/* See above wrt _rcuidle. */
-		trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, 0);
+		trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, 0);
 	}
 
 	/* Make sure we write CR3 before loaded_mm. */
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/suspend.h>
 #include <linux/tick.h>
+#include <linux/mmu_context.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -230,6 +231,9 @@ int cpuidle_enter_state(struct cpuidle_d
 		broadcast = false;
 	}
 
+	if (target_state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
+		leave_mm(dev->cpu);
+
 	/* Take note of the planned idle state. */
 	sched_idle_set_state(target_state);
 
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -90,14 +90,6 @@ static unsigned int mwait_substates __in
 #define CPUIDLE_FLAG_ALWAYS_ENABLE	BIT(15)
 
 /*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED	BIT(16)
-
-/*
  * MWAIT takes an 8-bit "hint" in EAX "suggesting"
  * the C-state (top nibble) and sub-state (bottom nibble)
  * 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc.
@@ -131,14 +123,6 @@ static __cpuidle int intel_idle(struct c
 	unsigned long eax = flg2MWAIT(state->flags);
 	unsigned long ecx = 1; /* break on interrupt flag */
 	bool tick;
-	int cpu = smp_processor_id();
-
-	/*
-	 * leave_mm() to avoid costly and often unnecessary wakeups
-	 * for flushing the user TLB's associated with the active mm.
-	 */
-	if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
-		leave_mm(cpu);
 
 	if (!static_cpu_has(X86_FEATURE_ARAT)) {
 		/*
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -75,12 +75,13 @@ struct cpuidle_state {
 };
 
 /* Idle State Flags */
-#define CPUIDLE_FLAG_NONE       (0x00)
-#define CPUIDLE_FLAG_POLLING	BIT(0) /* polling state */
-#define CPUIDLE_FLAG_COUPLED	BIT(1) /* state applies to multiple cpus */
-#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
-#define CPUIDLE_FLAG_UNUSABLE	BIT(3) /* avoid using this state */
-#define CPUIDLE_FLAG_OFF	BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_NONE       	(0x00)
+#define CPUIDLE_FLAG_POLLING		BIT(0) /* polling state */
+#define CPUIDLE_FLAG_COUPLED		BIT(1) /* state applies to multiple cpus */
+#define CPUIDLE_FLAG_TIMER_STOP 	BIT(2) /* timer is stopped on this state */
+#define CPUIDLE_FLAG_UNUSABLE		BIT(3) /* avoid using this state */
+#define CPUIDLE_FLAG_OFF		BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_TLB_FLUSHED	BIT(5) /* idle-state flushes TLBs */
 
 struct cpuidle_device_kobj;
 struct cpuidle_state_kobj;
--- a/include/linux/mmu_context.h
+++ b/include/linux/mmu_context.h
@@ -3,10 +3,15 @@
 #define _LINUX_MMU_CONTEXT_H
 
 #include <asm/mmu_context.h>
+#include <asm/mmu.h>
 
 /* Architectures that care about IRQ state in switch_mm can override this. */
 #ifndef switch_mm_irqs_off
 # define switch_mm_irqs_off switch_mm
 #endif
 
+#ifndef leave_mm
+static inline void leave_mm(int cpu) { }
+#endif
+
 #endif

[tip: locking/core] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     bf9282dc26e7fe2a0736edc568762f0f05d12416
Gitweb:        https://git.kernel.org/tip/bf9282dc26e7fe2a0736edc568762f0f05d12416
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 12 Aug 2020 12:22:17 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00

cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic

This allows moving the leave_mm() call into generic code before
rcu_idle_enter(). Gets rid of more trace_*_rcuidle() users.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.369441600@infradead.org
---
 arch/x86/include/asm/mmu.h  |  1 +
 arch/x86/mm/tlb.c           | 13 ++-----------
 drivers/cpuidle/cpuidle.c   |  4 ++++
 drivers/idle/intel_idle.c   | 16 ----------------
 include/linux/cpuidle.h     | 13 +++++++------
 include/linux/mmu_context.h |  5 +++++
 6 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 0a301ad..9257667 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -59,5 +59,6 @@ typedef struct {
 	}
 
 void leave_mm(int cpu);
+#define leave_mm leave_mm
 
 #endif /* _ASM_X86_MMU_H */
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 1a3569b..0951b47 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -555,21 +555,12 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 		this_cpu_write(cpu_tlbstate.ctxs[new_asid].tlb_gen, next_tlb_gen);
 		load_new_mm_cr3(next->pgd, new_asid, true);
 
-		/*
-		 * NB: This gets called via leave_mm() in the idle path
-		 * where RCU functions differently.  Tracing normally
-		 * uses RCU, so we need to use the _rcuidle variant.
-		 *
-		 * (There is no good reason for this.  The idle code should
-		 *  be rearranged to call this before rcu_idle_enter().)
-		 */
-		trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+		trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
 	} else {
 		/* The new ASID is already up to date. */
 		load_new_mm_cr3(next->pgd, new_asid, false);
 
-		/* See above wrt _rcuidle. */
-		trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, 0);
+		trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, 0);
 	}
 
 	/* Make sure we write CR3 before loaded_mm. */
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 9bcda41..04becd7 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/suspend.h>
 #include <linux/tick.h>
+#include <linux/mmu_context.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -228,6 +229,9 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		broadcast = false;
 	}
 
+	if (target_state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
+		leave_mm(dev->cpu);
+
 	/* Take note of the planned idle state. */
 	sched_idle_set_state(target_state);
 
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 8e0fb1a..9a810e4 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -90,14 +90,6 @@ static unsigned int mwait_substates __initdata;
 #define CPUIDLE_FLAG_ALWAYS_ENABLE	BIT(15)
 
 /*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED	BIT(16)
-
-/*
  * MWAIT takes an 8-bit "hint" in EAX "suggesting"
  * the C-state (top nibble) and sub-state (bottom nibble)
  * 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc.
@@ -131,14 +123,6 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev,
 	unsigned long eax = flg2MWAIT(state->flags);
 	unsigned long ecx = 1; /* break on interrupt flag */
 	bool tick;
-	int cpu = smp_processor_id();
-
-	/*
-	 * leave_mm() to avoid costly and often unnecessary wakeups
-	 * for flushing the user TLB's associated with the active mm.
-	 */
-	if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
-		leave_mm(cpu);
 
 	if (!static_cpu_has(X86_FEATURE_ARAT)) {
 		/*
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b65909a..75895e6 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -75,12 +75,13 @@ struct cpuidle_state {
 };
 
 /* Idle State Flags */
-#define CPUIDLE_FLAG_NONE       (0x00)
-#define CPUIDLE_FLAG_POLLING	BIT(0) /* polling state */
-#define CPUIDLE_FLAG_COUPLED	BIT(1) /* state applies to multiple cpus */
-#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
-#define CPUIDLE_FLAG_UNUSABLE	BIT(3) /* avoid using this state */
-#define CPUIDLE_FLAG_OFF	BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_NONE       	(0x00)
+#define CPUIDLE_FLAG_POLLING		BIT(0) /* polling state */
+#define CPUIDLE_FLAG_COUPLED		BIT(1) /* state applies to multiple cpus */
+#define CPUIDLE_FLAG_TIMER_STOP 	BIT(2) /* timer is stopped on this state */
+#define CPUIDLE_FLAG_UNUSABLE		BIT(3) /* avoid using this state */
+#define CPUIDLE_FLAG_OFF		BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_TLB_FLUSHED	BIT(5) /* idle-state flushes TLBs */
 
 struct cpuidle_device_kobj;
 struct cpuidle_state_kobj;
diff --git a/include/linux/mmu_context.h b/include/linux/mmu_context.h
index c51a841..03dee12 100644
--- a/include/linux/mmu_context.h
+++ b/include/linux/mmu_context.h
@@ -3,10 +3,15 @@
 #define _LINUX_MMU_CONTEXT_H
 
 #include <asm/mmu_context.h>
+#include <asm/mmu.h>
 
 /* Architectures that care about IRQ state in switch_mm can override this. */
 #ifndef switch_mm_irqs_off
 # define switch_mm_irqs_off switch_mm
 #endif
 
+#ifndef leave_mm
+static inline void leave_mm(int cpu) { }
+#endif
+
 #endif

[PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code

[Peter Zijlstra]

Remove trace_cpu_idle() from the arch_cpu_idle() implementations and
put it in the generic code, right before disabling RCU. Gets rid of
more trace_*_rcuidle() users.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ----
 arch/arm64/kernel/process.c  |    2 --
 arch/s390/kernel/idle.c      |    3 +--
 arch/x86/kernel/process.c    |    4 ----
 kernel/sched/idle.c          |    3 +++
 5 files changed, 4 insertions(+), 12 deletions(-)

--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -298,11 +298,7 @@ static void omap3_pm_idle(void)
 	if (omap_irq_pending())
 		return;
 
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
-
 	omap_sram_idle();
-
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 #ifdef CONFIG_SUSPEND
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -123,10 +123,8 @@ void arch_cpu_idle(void)
 	 * This should do all the clock switching and wait for interrupt
 	 * tricks
 	 */
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	cpu_do_idle();
 	local_irq_enable();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -33,14 +33,13 @@ void enabled_wait(void)
 		PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK;
 	clear_cpu_flag(CIF_NOHZ_DELAY);
 
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	local_irq_save(flags);
 	/* Call the assembler magic in entry.S */
 	psw_idle(idle, psw_mask);
 	local_irq_restore(flags);
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 
 	/* Account time spent with enabled wait psw loaded as idle time. */
+	/* XXX seqcount has tracepoints that require RCU */
 	write_seqcount_begin(&idle->seqcount);
 	idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
 	idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -684,9 +684,7 @@ void arch_cpu_idle(void)
  */
 void __cpuidle default_idle(void)
 {
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	safe_halt();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 #if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
 EXPORT_SYMBOL(default_idle);
@@ -792,7 +790,6 @@ static int prefer_mwait_c1_over_halt(con
 static __cpuidle void mwait_idle(void)
 {
 	if (!current_set_polling_and_test()) {
-		trace_cpu_idle_rcuidle(1, smp_processor_id());
 		if (this_cpu_has(X86_BUG_CLFLUSH_MONITOR)) {
 			mb(); /* quirk */
 			clflush((void *)&current_thread_info()->flags);
@@ -804,7 +801,6 @@ static __cpuidle void mwait_idle(void)
 			__sti_mwait(0, 0);
 		else
 			local_irq_enable();
-		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 	} else {
 		local_irq_enable();
 	}
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -91,11 +91,14 @@ void __cpuidle default_idle_call(void)
 	if (current_clr_polling_and_test()) {
 		local_irq_enable();
 	} else {
+
+		trace_cpu_idle(1, smp_processor_id());
 		stop_critical_timings();
 		rcu_idle_enter();
 		arch_cpu_idle();
 		rcu_idle_exit();
 		start_critical_timings();
+		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 	}
 }
 

[tip: locking/core] cpuidle: Move trace_cpu_idle() into generic code

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     9864f5b5943ab0f1f835f21dc3f9f068d06f5b52
Gitweb:        https://git.kernel.org/tip/9864f5b5943ab0f1f835f21dc3f9f068d06f5b52
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 12 Aug 2020 12:27:10 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00

cpuidle: Move trace_cpu_idle() into generic code

Remove trace_cpu_idle() from the arch_cpu_idle() implementations and
put it in the generic code, right before disabling RCU. Gets rid of
more trace_*_rcuidle() users.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.428433395@infradead.org
---
 arch/arm/mach-omap2/pm34xx.c | 4 ----
 arch/arm64/kernel/process.c  | 2 --
 arch/s390/kernel/idle.c      | 3 +--
 arch/x86/kernel/process.c    | 4 ----
 kernel/sched/idle.c          | 3 +++
 5 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 6df395f..f5dfddf 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -298,11 +298,7 @@ static void omap3_pm_idle(void)
 	if (omap_irq_pending())
 		return;
 
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
-
 	omap_sram_idle();
-
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 #ifdef CONFIG_SUSPEND
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index b63ce4c..f180449 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -123,10 +123,8 @@ void arch_cpu_idle(void)
 	 * This should do all the clock switching and wait for interrupt
 	 * tricks
 	 */
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	cpu_do_idle();
 	local_irq_enable();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
index 88bb42c..c73f506 100644
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -33,14 +33,13 @@ void enabled_wait(void)
 		PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK;
 	clear_cpu_flag(CIF_NOHZ_DELAY);
 
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	local_irq_save(flags);
 	/* Call the assembler magic in entry.S */
 	psw_idle(idle, psw_mask);
 	local_irq_restore(flags);
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 
 	/* Account time spent with enabled wait psw loaded as idle time. */
+	/* XXX seqcount has tracepoints that require RCU */
 	write_seqcount_begin(&idle->seqcount);
 	idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
 	idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 994d839..13ce616 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -684,9 +684,7 @@ void arch_cpu_idle(void)
  */
 void __cpuidle default_idle(void)
 {
-	trace_cpu_idle_rcuidle(1, smp_processor_id());
 	safe_halt();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 #if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
 EXPORT_SYMBOL(default_idle);
@@ -792,7 +790,6 @@ static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 static __cpuidle void mwait_idle(void)
 {
 	if (!current_set_polling_and_test()) {
-		trace_cpu_idle_rcuidle(1, smp_processor_id());
 		if (this_cpu_has(X86_BUG_CLFLUSH_MONITOR)) {
 			mb(); /* quirk */
 			clflush((void *)&current_thread_info()->flags);
@@ -804,7 +801,6 @@ static __cpuidle void mwait_idle(void)
 			__sti_mwait(0, 0);
 		else
 			local_irq_enable();
-		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 	} else {
 		local_irq_enable();
 	}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index ea3a098..f324dc3 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -91,11 +91,14 @@ void __cpuidle default_idle_call(void)
 	if (current_clr_polling_and_test()) {
 		local_irq_enable();
 	} else {
+
+		trace_cpu_idle(1, smp_processor_id());
 		stop_critical_timings();
 		rcu_idle_enter();
 		arch_cpu_idle();
 		rcu_idle_exit();
 		start_critical_timings();
+		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 	}
 }
 

[PATCH v2 06/11] x86/entry: Remove unused THUNKs

[Peter Zijlstra]

Unused remnants

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
--- a/arch/x86/entry/thunk_32.S
+++ b/arch/x86/entry/thunk_32.S
@@ -29,11 +29,6 @@ SYM_CODE_START_NOALIGN(\name)
 SYM_CODE_END(\name)
 	.endm
 
-#ifdef CONFIG_TRACE_IRQFLAGS
-	THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
-	THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
-#endif
-
 #ifdef CONFIG_PREEMPTION
 	THUNK preempt_schedule_thunk, preempt_schedule
 	THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace

[tip: locking/core] x86/entry: Remove unused THUNKs

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     7da93f379330f2be1122ca7f54ab1eb44ef9aa59
Gitweb:        https://git.kernel.org/tip/7da93f379330f2be1122ca7f54ab1eb44ef9aa59
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 12 Aug 2020 19:28:07 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00

x86/entry: Remove unused THUNKs

Unused remnants

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.487040689@infradead.org
---
 arch/x86/entry/thunk_32.S | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/entry/thunk_32.S b/arch/x86/entry/thunk_32.S
index 3a07ce3..f1f96d4 100644
--- a/arch/x86/entry/thunk_32.S
+++ b/arch/x86/entry/thunk_32.S
@@ -29,11 +29,6 @@ SYM_CODE_START_NOALIGN(\name)
 SYM_CODE_END(\name)
 	.endm
 
-#ifdef CONFIG_TRACE_IRQFLAGS
-	THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
-	THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
-#endif
-
 #ifdef CONFIG_PREEMPTION
 	THUNK preempt_schedule_thunk, preempt_schedule
 	THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace

[PATCH v2 07/11] locking/lockdep: Cleanup

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 include/linux/irqflags.h |   54 ++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -49,10 +49,11 @@ struct irqtrace_events {
 DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 
-  extern void trace_hardirqs_on_prepare(void);
-  extern void trace_hardirqs_off_finish(void);
-  extern void trace_hardirqs_on(void);
-  extern void trace_hardirqs_off(void);
+extern void trace_hardirqs_on_prepare(void);
+extern void trace_hardirqs_off_finish(void);
+extern void trace_hardirqs_on(void);
+extern void trace_hardirqs_off(void);
+
 # define lockdep_hardirq_context()	(raw_cpu_read(hardirq_context))
 # define lockdep_softirq_context(p)	((p)->softirq_context)
 # define lockdep_hardirqs_enabled()	(this_cpu_read(hardirqs_enabled))
@@ -120,17 +121,17 @@ do {						\
 #else
 # define trace_hardirqs_on_prepare()		do { } while (0)
 # define trace_hardirqs_off_finish()		do { } while (0)
-# define trace_hardirqs_on()		do { } while (0)
-# define trace_hardirqs_off()		do { } while (0)
-# define lockdep_hardirq_context()	0
-# define lockdep_softirq_context(p)	0
-# define lockdep_hardirqs_enabled()	0
-# define lockdep_softirqs_enabled(p)	0
-# define lockdep_hardirq_enter()	do { } while (0)
-# define lockdep_hardirq_threaded()	do { } while (0)
-# define lockdep_hardirq_exit()		do { } while (0)
-# define lockdep_softirq_enter()	do { } while (0)
-# define lockdep_softirq_exit()		do { } while (0)
+# define trace_hardirqs_on()			do { } while (0)
+# define trace_hardirqs_off()			do { } while (0)
+# define lockdep_hardirq_context()		0
+# define lockdep_softirq_context(p)		0
+# define lockdep_hardirqs_enabled()		0
+# define lockdep_softirqs_enabled(p)		0
+# define lockdep_hardirq_enter()		do { } while (0)
+# define lockdep_hardirq_threaded()		do { } while (0)
+# define lockdep_hardirq_exit()			do { } while (0)
+# define lockdep_softirq_enter()		do { } while (0)
+# define lockdep_softirq_exit()			do { } while (0)
 # define lockdep_hrtimer_enter(__hrtimer)	false
 # define lockdep_hrtimer_exit(__context)	do { } while (0)
 # define lockdep_posixtimer_enter()		do { } while (0)
@@ -181,17 +182,25 @@ do {						\
  * if !TRACE_IRQFLAGS.
  */
 #ifdef CONFIG_TRACE_IRQFLAGS
-#define local_irq_enable() \
-	do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
-#define local_irq_disable() \
-	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+
+#define local_irq_enable()				\
+	do {						\
+		trace_hardirqs_on();			\
+		raw_local_irq_enable();			\
+	} while (0)
+
+#define local_irq_disable()				\
+	do {						\
+		raw_local_irq_disable();		\
+		trace_hardirqs_off();			\
+	} while (0)
+
 #define local_irq_save(flags)				\
 	do {						\
 		raw_local_irq_save(flags);		\
 		trace_hardirqs_off();			\
 	} while (0)
 
-
 #define local_irq_restore(flags)			\
 	do {						\
 		if (raw_irqs_disabled_flags(flags)) {	\
@@ -214,10 +223,7 @@ do {						\
 
 #define local_irq_enable()	do { raw_local_irq_enable(); } while (0)
 #define local_irq_disable()	do { raw_local_irq_disable(); } while (0)
-#define local_irq_save(flags)					\
-	do {							\
-		raw_local_irq_save(flags);			\
-	} while (0)
+#define local_irq_save(flags)	do { raw_local_irq_save(flags); } while (0)
 #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
 #define safe_halt()		do { raw_safe_halt(); } while (0)
 

[tip: locking/core] locking/lockdep: Cleanup

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     00b0ed2d4997af6d0a93edef820386951fd66d94
Gitweb:        https://git.kernel.org/tip/00b0ed2d4997af6d0a93edef820386951fd66d94
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 12 Aug 2020 19:28:06 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00

locking/lockdep: Cleanup

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.546087214@infradead.org
---
 include/linux/irqflags.h | 54 +++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index d7e50a2..00d553d 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -49,10 +49,11 @@ struct irqtrace_events {
 DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 
-  extern void trace_hardirqs_on_prepare(void);
-  extern void trace_hardirqs_off_finish(void);
-  extern void trace_hardirqs_on(void);
-  extern void trace_hardirqs_off(void);
+extern void trace_hardirqs_on_prepare(void);
+extern void trace_hardirqs_off_finish(void);
+extern void trace_hardirqs_on(void);
+extern void trace_hardirqs_off(void);
+
 # define lockdep_hardirq_context()	(raw_cpu_read(hardirq_context))
 # define lockdep_softirq_context(p)	((p)->softirq_context)
 # define lockdep_hardirqs_enabled()	(this_cpu_read(hardirqs_enabled))
@@ -120,17 +121,17 @@ do {						\
 #else
 # define trace_hardirqs_on_prepare()		do { } while (0)
 # define trace_hardirqs_off_finish()		do { } while (0)
-# define trace_hardirqs_on()		do { } while (0)
-# define trace_hardirqs_off()		do { } while (0)
-# define lockdep_hardirq_context()	0
-# define lockdep_softirq_context(p)	0
-# define lockdep_hardirqs_enabled()	0
-# define lockdep_softirqs_enabled(p)	0
-# define lockdep_hardirq_enter()	do { } while (0)
-# define lockdep_hardirq_threaded()	do { } while (0)
-# define lockdep_hardirq_exit()		do { } while (0)
-# define lockdep_softirq_enter()	do { } while (0)
-# define lockdep_softirq_exit()		do { } while (0)
+# define trace_hardirqs_on()			do { } while (0)
+# define trace_hardirqs_off()			do { } while (0)
+# define lockdep_hardirq_context()		0
+# define lockdep_softirq_context(p)		0
+# define lockdep_hardirqs_enabled()		0
+# define lockdep_softirqs_enabled(p)		0
+# define lockdep_hardirq_enter()		do { } while (0)
+# define lockdep_hardirq_threaded()		do { } while (0)
+# define lockdep_hardirq_exit()			do { } while (0)
+# define lockdep_softirq_enter()		do { } while (0)
+# define lockdep_softirq_exit()			do { } while (0)
 # define lockdep_hrtimer_enter(__hrtimer)	false
 # define lockdep_hrtimer_exit(__context)	do { } while (0)
 # define lockdep_posixtimer_enter()		do { } while (0)
@@ -181,17 +182,25 @@ do {						\
  * if !TRACE_IRQFLAGS.
  */
 #ifdef CONFIG_TRACE_IRQFLAGS
-#define local_irq_enable() \
-	do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
-#define local_irq_disable() \
-	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+
+#define local_irq_enable()				\
+	do {						\
+		trace_hardirqs_on();			\
+		raw_local_irq_enable();			\
+	} while (0)
+
+#define local_irq_disable()				\
+	do {						\
+		raw_local_irq_disable();		\
+		trace_hardirqs_off();			\
+	} while (0)
+
 #define local_irq_save(flags)				\
 	do {						\
 		raw_local_irq_save(flags);		\
 		trace_hardirqs_off();			\
 	} while (0)
 
-
 #define local_irq_restore(flags)			\
 	do {						\
 		if (raw_irqs_disabled_flags(flags)) {	\
@@ -214,10 +223,7 @@ do {						\
 
 #define local_irq_enable()	do { raw_local_irq_enable(); } while (0)
 #define local_irq_disable()	do { raw_local_irq_disable(); } while (0)
-#define local_irq_save(flags)					\
-	do {							\
-		raw_local_irq_save(flags);			\
-	} while (0)
+#define local_irq_save(flags)	do { raw_local_irq_save(flags); } while (0)
 #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
 #define safe_halt()		do { raw_safe_halt(); } while (0)
 

[PATCH v2 08/11] nds32: Implement arch_irqs_disabled()

[Peter Zijlstra]

Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <green.hu@gmail.com>
Cc: Vincent Chen <deanbo422@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 arch/nds32/include/asm/irqflags.h |    5 +++++
 1 file changed, 5 insertions(+)

--- a/arch/nds32/include/asm/irqflags.h
+++ b/arch/nds32/include/asm/irqflags.h
@@ -34,3 +34,8 @@ static inline int arch_irqs_disabled_fla
 {
 	return !flags;
 }
+
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}

[tip: locking/core] nds32: Implement arch_irqs_disabled()

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     36206b588bc815e5f64e8da72d7ab79e00b76281
Gitweb:        https://git.kernel.org/tip/36206b588bc815e5f64e8da72d7ab79e00b76281
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 20 Aug 2020 09:27:52 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00

nds32: Implement arch_irqs_disabled()

Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <green.hu@gmail.com>
Cc: Vincent Chen <deanbo422@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.604899379@infradead.org
---
 arch/nds32/include/asm/irqflags.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/nds32/include/asm/irqflags.h b/arch/nds32/include/asm/irqflags.h
index fb45ec4..51ef800 100644
--- a/arch/nds32/include/asm/irqflags.h
+++ b/arch/nds32/include/asm/irqflags.h
@@ -34,3 +34,8 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
 {
 	return !flags;
 }
+
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}

[PATCH v2 09/11] arm64: Implement arch_irqs_disabled()

[Peter Zijlstra]

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm64/include/asm/irqflags.h |    5 +++++
 1 file changed, 5 insertions(+)

--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -95,6 +95,11 @@ static inline int arch_irqs_disabled_fla
 	return res;
 }
 
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags;

[tip: locking/core] arm64: Implement arch_irqs_disabled()

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     021c109330ebc1f54b546c63a078ea3c31356ecb
Gitweb:        https://git.kernel.org/tip/021c109330ebc1f54b546c63a078ea3c31356ecb
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Fri, 21 Aug 2020 10:40:49 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00

arm64: Implement arch_irqs_disabled()

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Link: https://lkml.kernel.org/r/20200821085348.664425120@infradead.org
---
 arch/arm64/include/asm/irqflags.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index aa4b652..ff328e5 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -95,6 +95,11 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
 	return res;
 }
 
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags;

[PATCH v2 10/11] lockdep: Only trace IRQ edges

[Peter Zijlstra]

From: Nicholas Piggin <npiggin@gmail.com>

Problem:

	raw_local_irq_save();
	local_irq_save();
	...
	local_irq_restore();
	raw_local_irq_restore();

existing instances:

 - lock_acquire()
     raw_local_irq_save()
     __lock_acquire()
       arch_spin_lock(&graph_lock)
         pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
           local_irq_save()

 - trace_clock_global()
     raw_local_irq_save()
     arch_spin_lock()
       pv_wait() := kvm_wait()
	 local_irq_save()

 - apic_retrigger_irq()
     raw_local_irq_save()
     apic->send_IPI() := default_send_IPI_single_phys()
       local_irq_save()

Possible solutions:

 A) make it work by enabling the tracing inside raw_*()
 B) make it work by keeping tracing disabled inside raw_*()

Now, given that the only reason to use the raw_* variant is because you don't
want tracing, A) seems like a weird option (although it can be done), so we
pick B) and declare any code that ends up doing:

	raw_local_irq_save()
	local_irq_save()
	lockdep_assert_irqs_disabled();

broken. AFAICT this problem has existed forever, the only reason it came
up is because I changed IRQ tracing vs lockdep recursion and the first
instance is fairly common, the other cases hardly ever happen.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
[rewrote changelog]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200723105615.1268126-1-npiggin@gmail.com
---

 arch/powerpc/include/asm/hw_irq.h |   11 ++++-------
 include/linux/irqflags.h          |   15 +++++++--------
 2 files changed, 11 insertions(+), 15 deletions(-)

--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(vo
 #define powerpc_local_irq_pmu_save(flags)			\
 	 do {							\
 		raw_local_irq_pmu_save(flags);			\
-		trace_hardirqs_off();				\
+		if (!raw_irqs_disabled_flags(flags))		\
+			trace_hardirqs_off();			\
 	} while(0)
 #define powerpc_local_irq_pmu_restore(flags)			\
 	do {							\
-		if (raw_irqs_disabled_flags(flags)) {		\
-			raw_local_irq_pmu_restore(flags);	\
-			trace_hardirqs_off();			\
-		} else {					\
+		if (!raw_irqs_disabled_flags(flags))		\
 			trace_hardirqs_on();			\
-			raw_local_irq_pmu_restore(flags);	\
-		}						\
+		raw_local_irq_pmu_restore(flags);		\
 	} while(0)
 #else
 #define powerpc_local_irq_pmu_save(flags)			\
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -191,25 +191,24 @@ do {						\
 
 #define local_irq_disable()				\
 	do {						\
+		bool was_disabled = raw_irqs_disabled();\
 		raw_local_irq_disable();		\
-		trace_hardirqs_off();			\
+		if (!was_disabled)			\
+			trace_hardirqs_off();		\
 	} while (0)
 
 #define local_irq_save(flags)				\
 	do {						\
 		raw_local_irq_save(flags);		\
-		trace_hardirqs_off();			\
+		if (!raw_irqs_disabled_flags(flags))	\
+			trace_hardirqs_off();		\
 	} while (0)
 
 #define local_irq_restore(flags)			\
 	do {						\
-		if (raw_irqs_disabled_flags(flags)) {	\
-			raw_local_irq_restore(flags);	\
-			trace_hardirqs_off();		\
-		} else {				\
+		if (!raw_irqs_disabled_flags(flags))	\
 			trace_hardirqs_on();		\
-			raw_local_irq_restore(flags);	\
-		}					\
+		raw_local_irq_restore(flags);		\
 	} while (0)
 
 #define safe_halt()				\

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[Thomas Gleixner]

On Fri, Aug 21 2020 at 10:47, Peter Zijlstra wrote:
> Now, given that the only reason to use the raw_* variant is because you don't
> want tracing, A) seems like a weird option (although it can be done), so we
> pick B) and declare any code that ends up doing:
>
> 	raw_local_irq_save()
> 	local_irq_save()
> 	lockdep_assert_irqs_disabled();
>
> broken. AFAICT this problem has existed forever, the only reason it came
> up is because I changed IRQ tracing vs lockdep recursion and the first

Who is 'I'? I know you made that change and you also rewrote the
changelog, but I only figured that out after scrolling further down. As
this patch is authored by Nick, the above is simply inconsistent. Can we
please have just a reference to the commit which changed that code
instead of a puzzle?

Other than that:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[Guenter Roeck]

On Fri, Aug 21, 2020 at 10:47:48AM +0200, Peter Zijlstra wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
> 
> Problem:
> 
> 	raw_local_irq_save();
> 	local_irq_save();
> 	...
> 	local_irq_restore();
> 	raw_local_irq_restore();
> 
> existing instances:
> 
>  - lock_acquire()
>      raw_local_irq_save()
>      __lock_acquire()
>        arch_spin_lock(&graph_lock)
>          pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
>            local_irq_save()
> 
>  - trace_clock_global()
>      raw_local_irq_save()
>      arch_spin_lock()
>        pv_wait() := kvm_wait()
> 	 local_irq_save()
> 
>  - apic_retrigger_irq()
>      raw_local_irq_save()
>      apic->send_IPI() := default_send_IPI_single_phys()
>        local_irq_save()
> 
> Possible solutions:
> 
>  A) make it work by enabling the tracing inside raw_*()
>  B) make it work by keeping tracing disabled inside raw_*()
> 
> Now, given that the only reason to use the raw_* variant is because you don't
> want tracing, A) seems like a weird option (although it can be done), so we
> pick B) and declare any code that ends up doing:
> 
> 	raw_local_irq_save()
> 	local_irq_save()
> 	lockdep_assert_irqs_disabled();
> 
> broken. AFAICT this problem has existed forever, the only reason it came
> up is because I changed IRQ tracing vs lockdep recursion and the first
> instance is fairly common, the other cases hardly ever happen.
> 

On sparc64, this patch results in the traceback below. The traceback is gone
after reverting the patch.

Guenter

---
[    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
[    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-rc3 #1
[    0.000000] Call Trace:
[    0.000000] [<0000000000469890>] __warn+0xb0/0xe0
[    0.000000] [<00000000004698fc>] warn_slowpath_fmt+0x3c/0x80
[    0.000000] [<00000000004cfce0>] check_flags.part.39+0x280/0x2a0
[    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
[    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
[    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
[    0.000000] [<00000000009872cc>] prom_getproplen+0x4c/0x60
[    0.000000] [<0000000000987308>] prom_getproperty+0x8/0x80
[    0.000000] [<0000000000987390>] prom_getint+0x10/0x40
[    0.000000] [<00000000017df4b4>] prom_init+0x38/0x8c
[    0.000000] [<0000000000d6b558>] tlb_fixup_done+0x44/0x6c
[    0.000000] [<00000000ffd0e930>] 0xffd0e930
[    0.000000] irq event stamp: 1
[    0.000000] hardirqs last  enabled at (1): [<0000000000987124>] p1275_cmd_direct+0x44/0x60
[    0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
[    0.000000] softirqs last  enabled at (0): [<0000000000000000>] 0x0
[    0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
[    0.000000] random: get_random_bytes called from print_oops_end_marker+0x30/0x60 with crng_init=0
[    0.000000] ---[ end trace 0000000000000000 ]---
[    0.000000] possible reason: unannotated irqs-off.

---
bisect log:

# bad: [f75aef392f869018f78cfedf3c320a6b3fcfda6b] Linux 5.9-rc3
# good: [1127b219ce9481c84edad9711626d856127d5e51] Merge tag 'fallthrough-fixes-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux
git bisect start 'f75aef392f86' '1127b219ce94'
# good: [8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d] Merge tag 'powerpc-5.9-4' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d
# good: [ceb2465c51195967f11f6507538579816ac67cb8] Merge tag 'irqchip-fixes-5.9-2' of git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms into irq/urgent
git bisect good ceb2465c51195967f11f6507538579816ac67cb8
# bad: [b69bea8a657b681442765b06be92a2607b1bd875] Merge tag 'locking-urgent-2020-08-30' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad b69bea8a657b681442765b06be92a2607b1bd875
# good: [00b0ed2d4997af6d0a93edef820386951fd66d94] locking/lockdep: Cleanup
git bisect good 00b0ed2d4997af6d0a93edef820386951fd66d94
# bad: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges
git bisect bad 044d0d6de9f50192f9697583504a382347ee95ca
# good: [021c109330ebc1f54b546c63a078ea3c31356ecb] arm64: Implement arch_irqs_disabled()
git bisect good 021c109330ebc1f54b546c63a078ea3c31356ecb
# good: [99dc56feb7932020502d40107a712fa302b32082] mips: Implement arch_irqs_disabled()
git bisect good 99dc56feb7932020502d40107a712fa302b32082
# first bad commit: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[peterz]

On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> [    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())

> [    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> [    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> [    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60

Lol! yes, I can see that going side-ways... let me poke at that.

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[peterz]

On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> > [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> > [    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
> 
> > [    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> > [    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> > [    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
> 
> Lol! yes, I can see that going side-ways... let me poke at that.

I suspect this will do.

diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
index 889aa602f8d8..7cfe88e30b52 100644
--- a/arch/sparc/prom/p1275.c
+++ b/arch/sparc/prom/p1275.c
@@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
 	unsigned long flags;

 	local_save_flags(flags);
-	local_irq_restore((unsigned long)PIL_NMI);
+	arch_local_irq_restore((unsigned long)PIL_NMI);
 	raw_spin_lock(&prom_entry_lock);

 	prom_world(1);

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[Guenter Roeck]

On 9/2/20 2:12 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
>> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
>>> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
>>> [    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
>>
>>> [    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
>>> [    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
>>> [    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
>>
>> Lol! yes, I can see that going side-ways... let me poke at that.
> 
> I suspect this will do.
> 
> diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
> index 889aa602f8d8..7cfe88e30b52 100644
> --- a/arch/sparc/prom/p1275.c
> +++ b/arch/sparc/prom/p1275.c
> @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
>  	unsigned long flags;
> 
>  	local_save_flags(flags);
> -	local_irq_restore((unsigned long)PIL_NMI);
> +	arch_local_irq_restore((unsigned long)PIL_NMI);
>  	raw_spin_lock(&prom_entry_lock);
> 
>  	prom_world(1);
> 
No, that doesn't help. Even removing that line entirely doesn't help.
The problem seems to be that interrupts are not enabled in the first
place. But why wasn't this a problem before ?

Guenter

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[peterz]

On Wed, Sep 02, 2020 at 06:48:30AM -0700, Guenter Roeck wrote:
> On 9/2/20 2:12 AM, peterz@infradead.org wrote:
> > On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
> >> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> >>> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> >>> [    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
> >>
> >>> [    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> >>> [    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> >>> [    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
> >>
> >> Lol! yes, I can see that going side-ways... let me poke at that.
> > 
> > I suspect this will do.
> > 
> > diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
> > index 889aa602f8d8..7cfe88e30b52 100644
> > --- a/arch/sparc/prom/p1275.c
> > +++ b/arch/sparc/prom/p1275.c
> > @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
> >  	unsigned long flags;
> > 
> >  	local_save_flags(flags);
> > -	local_irq_restore((unsigned long)PIL_NMI);
> > +	arch_local_irq_restore((unsigned long)PIL_NMI);
> >  	raw_spin_lock(&prom_entry_lock);
> > 
> >  	prom_world(1);
> > 
> No, that doesn't help. Even removing that line entirely doesn't help.
> The problem seems to be that interrupts are not enabled in the first
> place. But why wasn't this a problem before ?

Previously every interrupt opt would disable/enable things, now we only
update state when something actually changes.

Anyway, I'm struggling with qemu-system-sparc64, I've got a sparc64
cross booting to mount, but I'm not seeing this, could you get me your
specific qemu cmdline please?

Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges

[Guenter Roeck]

On 9/8/20 7:22 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 06:48:30AM -0700, Guenter Roeck wrote:
>> On 9/2/20 2:12 AM, peterz@infradead.org wrote:
>>> On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
>>>> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
>>>>> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
>>>>> [    0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
>>>>
>>>>> [    0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
>>>>> [    0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
>>>>> [    0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
>>>>
>>>> Lol! yes, I can see that going side-ways... let me poke at that.
>>>
>>> I suspect this will do.
>>>
>>> diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
>>> index 889aa602f8d8..7cfe88e30b52 100644
>>> --- a/arch/sparc/prom/p1275.c
>>> +++ b/arch/sparc/prom/p1275.c
>>> @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
>>>  	unsigned long flags;
>>>
>>>  	local_save_flags(flags);
>>> -	local_irq_restore((unsigned long)PIL_NMI);
>>> +	arch_local_irq_restore((unsigned long)PIL_NMI);
>>>  	raw_spin_lock(&prom_entry_lock);
>>>
>>>  	prom_world(1);
>>>
>> No, that doesn't help. Even removing that line entirely doesn't help.
>> The problem seems to be that interrupts are not enabled in the first
>> place. But why wasn't this a problem before ?
> 
> Previously every interrupt opt would disable/enable things, now we only
> update state when something actually changes.
> 
> Anyway, I'm struggling with qemu-system-sparc64, I've got a sparc64
> cross booting to mount, but I'm not seeing this, could you get me your
> specific qemu cmdline please?
> 

initrd:

qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
    -initrd rootfs.cpio \
    -kernel arch/sparc/boot/image -no-reboot \
    -append "panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0" \
    -nographic -monitor none

root file system:

qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
    -snapshot =drive file=rootfs.ext2,format=raw,if=ide \
    -kernel arch/sparc/boot/image -no-reboot \
    -append "panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttyS0" \
    -nographic -monitor none

Some of it, like the CPU, should not be needed. qemu version is v5.1,
but v5.0 should do as well. Some older qemu versions won't accept the
kernel from the command line.

Did you enable lockdep debugging ? In my configuration I enable lots
of debug options on top of defconfig. See [1], function __setup_fragment(),
for details.

Some root file systems are at [2] if needed. The complete script used
to build and run the code is at [3].

Guenter

---
[1] https://github.com/groeck/linux-build-test/blob/master/rootfs/scripts/common.sh
[2] https://github.com/groeck/linux-build-test/tree/master/rootfs/sparc64
[3] https://github.com/groeck/linux-build-test/blob/master/rootfs/sparc64/run-qemu-sparc64.sh

[PATCH] sparc64: Fix irqtrace warnings on Ultra-S

[peterz]

On Tue, Sep 08, 2020 at 07:40:23AM -0700, Guenter Roeck wrote:
> qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
>     -initrd rootfs.cpio \
>     -kernel arch/sparc/boot/image -no-reboot \
>     -append "panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0" \
>     -nographic -monitor none

Thanks I got it. Also enabling DEBUG_LOCKDEP helps (-:

---
Subject: sparc64: Fix irqtrace warnings on Ultra-S

Recent changes in Lockdep's IRQTRACE broke Ultra-S.

In order avoid redundant IRQ state changes, local_irq_restore() lost the
ability to trace a disable. Change the code to use local_irq_save() to
disable IRQs and then use arch_local_irq_restore() to further disable
NMIs.

This result in slightly suboptimal code, but given this code uses a
global spinlock, performance cannot be its primary purpose.

Fixes: 044d0d6de9f5 ("lockdep: Only trace IRQ edges")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
A possible alternative would be:

	local_save_flags(flags);
	arch_local_irq_restore((unsigned long)PIL_NMI);
	if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS))
		trace_hardirqs_off();

which generates optimal code, but is more verbose.

 arch/sparc/prom/p1275.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
index 889aa602f8d8..e22233fcf741 100644
--- a/arch/sparc/prom/p1275.c
+++ b/arch/sparc/prom/p1275.c
@@ -37,16 +37,15 @@ void p1275_cmd_direct(unsigned long *args)
 {
 	unsigned long flags;
 
-	local_save_flags(flags);
-	local_irq_restore((unsigned long)PIL_NMI);
+	local_irq_save(flags);
+	arch_local_irq_restore((unsigned long)PIL_NMI);
 	raw_spin_lock(&prom_entry_lock);
 
 	prom_world(1);
 	prom_cif_direct(args);
 	prom_world(0);
 
-	raw_spin_unlock(&prom_entry_lock);
-	local_irq_restore(flags);
+	raw_spin_unlock_irqrestore(&prom_entry_lock, flags);
 }
 
 void prom_cif_init(void *cif_handler, void *cif_stack)

[PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Peter Zijlstra]

The lockdep tracepoints are under the lockdep recursion counter, this
has a bunch of nasty side effects:

 - TRACE_IRQFLAGS doesn't work across the entire tracepoint

 - RCU-lockdep doesn't see the tracepoints either, hiding numerous
   "suspicious RCU usage" warnings.

Pull the trace_lock_*() tracepoints completely out from under the
lockdep recursion handling and completely rely on the trace level
recusion handling -- also, tracing *SHOULD* not be taking locks in any
case.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
 kernel/locking/lockdep.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5044,6 +5044,8 @@ void lock_acquire(struct lockdep_map *lo
 {
 	unsigned long flags;
 
+	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
+
 	if (unlikely(current->lockdep_recursion)) {
 		/* XXX allow trylock from NMI ?!? */
 		if (lockdep_nmi() && !trylock) {
@@ -5068,7 +5070,6 @@ void lock_acquire(struct lockdep_map *lo
 	check_flags(flags);
 
 	current->lockdep_recursion++;
-	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
 	__lock_acquire(lock, subclass, trylock, read, check,
 		       irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
 	lockdep_recursion_finish();
@@ -5080,13 +5081,15 @@ void lock_release(struct lockdep_map *lo
 {
 	unsigned long flags;
 
+	trace_lock_release(lock, ip);
+
 	if (unlikely(current->lockdep_recursion))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
+
 	current->lockdep_recursion++;
-	trace_lock_release(lock, ip);
 	if (__lock_release(lock, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
@@ -5272,8 +5275,6 @@ __lock_acquired(struct lockdep_map *lock
 		hlock->holdtime_stamp = now;
 	}
 
-	trace_lock_acquired(lock, ip);
-
 	stats = get_lock_stats(hlock_class(hlock));
 	if (waittime) {
 		if (hlock->read)
@@ -5292,6 +5293,8 @@ void lock_contended(struct lockdep_map *
 {
 	unsigned long flags;
 
+	trace_lock_contended(lock, ip);
+
 	if (unlikely(!lock_stat || !debug_locks))
 		return;
 
@@ -5301,7 +5304,6 @@ void lock_contended(struct lockdep_map *
 	raw_local_irq_save(flags);
 	check_flags(flags);
 	current->lockdep_recursion++;
-	trace_lock_contended(lock, ip);
 	__lock_contended(lock, ip);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5312,6 +5314,8 @@ void lock_acquired(struct lockdep_map *l
 {
 	unsigned long flags;
 
+	trace_lock_acquired(lock, ip);
+
 	if (unlikely(!lock_stat || !debug_locks))
 		return;
 

[tip: locking/core] lockdep,trace: Expose tracepoints

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     eb1f00237aca2e368b93db79303f8433d1976d10
Gitweb:        https://git.kernel.org/tip/eb1f00237aca2e368b93db79303f8433d1976d10
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Fri, 07 Aug 2020 20:53:16 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:56 +02:00

lockdep,trace: Expose tracepoints

The lockdep tracepoints are under the lockdep recursion counter, this
has a bunch of nasty side effects:

 - TRACE_IRQFLAGS doesn't work across the entire tracepoint

 - RCU-lockdep doesn't see the tracepoints either, hiding numerous
   "suspicious RCU usage" warnings.

Pull the trace_lock_*() tracepoints completely out from under the
lockdep recursion handling and completely rely on the trace level
recusion handling -- also, tracing *SHOULD* not be taking locks in any
case.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.782688941@infradead.org
---
 kernel/locking/lockdep.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index c872e95..54b74fa 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4977,6 +4977,8 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 {
 	unsigned long flags;
 
+	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
+
 	if (unlikely(current->lockdep_recursion)) {
 		/* XXX allow trylock from NMI ?!? */
 		if (lockdep_nmi() && !trylock) {
@@ -5001,7 +5003,6 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	check_flags(flags);
 
 	current->lockdep_recursion++;
-	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
 	__lock_acquire(lock, subclass, trylock, read, check,
 		       irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
 	lockdep_recursion_finish();
@@ -5013,13 +5014,15 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
+	trace_lock_release(lock, ip);
+
 	if (unlikely(current->lockdep_recursion))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
+
 	current->lockdep_recursion++;
-	trace_lock_release(lock, ip);
 	if (__lock_release(lock, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
@@ -5205,8 +5208,6 @@ __lock_acquired(struct lockdep_map *lock, unsigned long ip)
 		hlock->holdtime_stamp = now;
 	}
 
-	trace_lock_acquired(lock, ip);
-
 	stats = get_lock_stats(hlock_class(hlock));
 	if (waittime) {
 		if (hlock->read)
@@ -5225,6 +5226,8 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
+	trace_lock_acquired(lock, ip);
+
 	if (unlikely(!lock_stat || !debug_locks))
 		return;
 
@@ -5234,7 +5237,6 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 	raw_local_irq_save(flags);
 	check_flags(flags);
 	current->lockdep_recursion++;
-	trace_lock_contended(lock, ip);
 	__lock_contended(lock, ip);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5245,6 +5247,8 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
+	trace_lock_contended(lock, ip);
+
 	if (unlikely(!lock_stat || !debug_locks))
 		return;
 

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Guenter Roeck]

On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
> The lockdep tracepoints are under the lockdep recursion counter, this
> has a bunch of nasty side effects:
> 
>  - TRACE_IRQFLAGS doesn't work across the entire tracepoint
> 
>  - RCU-lockdep doesn't see the tracepoints either, hiding numerous
>    "suspicious RCU usage" warnings.
> 
> Pull the trace_lock_*() tracepoints completely out from under the
> lockdep recursion handling and completely rely on the trace level
> recusion handling -- also, tracing *SHOULD* not be taking locks in any
> case.
> 

Wonder what is worse - the problem or its fix. This patch results in
a number of WARNING backtraces for several archtectures/platforms.
Reverting it fixes the problems.

Guenter

---
arm:

[   27.055084]
[   27.056213] =============================
[   27.056274] WARNING: suspicious RCU usage
[   27.056335] 5.9.0-rc3 #1 Not tainted
[   27.056396] -----------------------------
[   27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
[   27.056549]
[   27.056549] other info that might help us debug this:
[   27.056549]
[   27.056640]
[   27.056640] rcu_scheduler_active = 2, debug_locks = 1
[   27.056732] RCU used illegally from extended quiescent state!
[   27.056793] no locks held by swapper/0/0.
[   27.056854]
[   27.056854] stack backtrace:
[   27.056915] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[   27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
[   27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
[   27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
[   27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
[   27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
[   27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
[   27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
[   27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
[   27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
[   27.058197] [<c0f740d4>] (cpuidle_enter_state) from [<c0f745ec>] (cpuidle_enter+0x50/0x54)
[   27.058349] [<c0f745ec>] (cpuidle_enter) from [<c0383aac>] (do_idle+0x228/0x2b8)
[   27.058471] [<c0383aac>] (do_idle) from [<c0383f34>] (cpu_startup_entry+0x18/0x1c)
[   27.058624] [<c0383f34>] (cpu_startup_entry) from [<c1c00e8c>] (start_kernel+0x518/0x558)
[   27.059692]
[   27.059753] =============================
[   27.059753] WARNING: suspicious RCU usage
[   27.059753] 5.9.0-rc3 #1 Not tainted
[   27.059753] -----------------------------
[   27.059753] include/trace/events/lock.h:58 suspicious rcu_dereference_check() usage!
[   27.059783]
[   27.059783] other info that might help us debug this:
[   27.059783]
[   27.059783]
[   27.059783] rcu_scheduler_active = 2, debug_locks = 1
[   27.059783] RCU used illegally from extended quiescent state!
[   27.059783] 1 lock held by swapper/0/0:
[   27.059814]  #0: c1e30f3c (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x60/0x38c
[   27.059906]
[   27.059936] stack backtrace:
[   27.059936] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[   27.059936] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
[   27.059936] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
[   27.059936] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
[   27.059936] [<c08e7a4c>] (dump_stack) from [<c03b4e70>] (lock_release+0x41c/0x420)
[   27.059936] [<c03b4e70>] (lock_release) from [<c124598c>] (_raw_spin_unlock+0x14/0x38)
[   27.059967] [<c124598c>] (_raw_spin_unlock) from [<c03c34c8>] (vprintk_emit+0xb4/0x38c)
[   27.059967] [<c03c34c8>] (vprintk_emit) from [<c03c37c0>] (vprintk_default+0x20/0x28)
[   27.059967] [<c03c37c0>] (vprintk_default) from [<c03c4088>] (printk+0x30/0x5c)
[   27.059967] [<c03c4088>] (printk) from [<c03b6c0c>] (lockdep_rcu_suspicious+0x2c/0xec)
[   27.059967] [<c03b6c0c>] (lockdep_rcu_suspicious) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
[   27.059967] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
[   27.059997] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
[   27.059997] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
[   27.059997] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
[   27.059997] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
[   27.059997] [<c0f740d4>] (cpuidle_enter_state) from [<c0f745ec>] (cpuidle_enter+0x50/0x54)
[   27.059997] [<c0f745ec>] (cpuidle_enter) from [<c0383aac>] (do_idle+0x228/0x2b8)
[   27.059997] [<c0383aac>] (do_idle) from [<c0383f34>] (cpu_startup_entry+0x18/0x1c)
[   27.060028] [<c0383f34>] (cpu_startup_entry) from [<c1c00e8c>] (start_kernel+0x518/0x558)

s390:

[   19.490586] =============================
[   19.490752] WARNING: suspicious RCU usage
[   19.490921] 5.9.0-rc3 #1 Not tainted
[   19.491086] -----------------------------
[   19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
[   19.491418]
[   19.491418] other info that might help us debug this:
[   19.491418]
[   19.491613]
[   19.491613] rcu_scheduler_active = 2, debug_locks = 1
[   19.491779] RCU used illegally from extended quiescent state!
[   19.491950] no locks held by swapper/0/0.
[   19.492118]
[   19.492118] stack backtrace:
[   19.492292] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[   19.492458] Hardware name: QEMU 2964 QEMU (KVM/Linux)
[   19.492622] Call Trace:
[   19.492798]  [<00000000001164f2>] show_stack+0x9a/0x100
[   19.492972]  [<0000000000982b94>] dump_stack+0x9c/0xd0
[   19.493147]  [<00000000001d5de2>] lock_acquire+0x41a/0x498
[   19.493320]  [<0000000000103b72>] enabled_wait+0xca/0x198
[   19.493493]  [<0000000000103f80>] arch_cpu_idle+0x20/0x38
[   19.493667]  [<0000000000e03c30>] default_idle_call+0x98/0x300
[   19.493847]  [<000000000019958c>] do_idle+0xe4/0x190
[   19.494021]  [<00000000001998f4>] cpu_startup_entry+0x34/0x38
[   19.494196]  [<00000000013a297a>] arch_call_rest_init+0x92/0x98
[   19.494362] no locks held by swapper/0/0.
[   19.497570]
[   19.497595] =============================
[   19.497605] WARNING: suspicious RCU usage
[   19.497614] 5.9.0-rc3 #1 Not tainted
[   19.497624] -----------------------------
[   19.497634] include/trace/events/lock.h:63 suspicious rcu_dereference_check() usage!
[   19.497643]
[   19.497652] other info that might help us debug this:
[   19.497661]
[   19.497669]
[   19.497679] rcu_scheduler_active = 2, debug_locks = 1
[   19.497689] RCU used illegally from extended quiescent state!
[   19.497698] 1 lock held by swapper/0/0:
[   19.497712]  #0: 0000000001252750 (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x70/0x368
[   19.497779]
[   19.497788] stack backtrace:
[   19.497797] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[   19.497807] Hardware name: QEMU 2964 QEMU (KVM/Linux)
[   19.497816] Call Trace:
[   19.497825]  [<00000000001164f2>] show_stack+0x9a/0x100
[   19.497835]  [<0000000000982b94>] dump_stack+0x9c/0xd0
[   19.497844]  [<00000000001d9914>] lock_release+0x334/0x350
[   19.497854]  [<0000000000e04354>] _raw_spin_unlock+0x2c/0x50
[   19.497863]  [<00000000001e6d5a>] vprintk_emit+0xb2/0x368
[   19.497873]  [<00000000001e704a>] vprintk_default+0x3a/0x48
[   19.497882]  [<00000000001e7b8c>] printk+0x4c/0x58
[   19.497892]  [<00000000001db3c2>] lockdep_rcu_suspicious+0x32/0x110
[   19.497902]  [<00000000001d5de2>] lock_acquire+0x41a/0x498
[   19.497911]  [<0000000000103b72>] enabled_wait+0xca/0x198
[   19.497921]  [<0000000000103f80>] arch_cpu_idle+0x20/0x38
[   19.497932]  [<0000000000e03c30>] default_idle_call+0x98/0x300
[   19.497943]  [<000000000019958c>] do_idle+0xe4/0x190
[   19.497954]  [<00000000001998f4>] cpu_startup_entry+0x34/0x38
[   19.497964]  [<00000000013a297a>] arch_call_rest_init+0x92/0x98
[   19.497974] 1 lock held by swapper/0/0:
[   19.497982]  #0: 0000000001252750 (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x70/0x368

Bisect log (for arm):

# bad: [f75aef392f869018f78cfedf3c320a6b3fcfda6b] Linux 5.9-rc3
# good: [1127b219ce9481c84edad9711626d856127d5e51] Merge tag 'fallthrough-fixes-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux
git bisect start 'f75aef392f86' '1127b219ce94'
# good: [8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d] Merge tag 'powerpc-5.9-4' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d
# good: [ceb2465c51195967f11f6507538579816ac67cb8] Merge tag 'irqchip-fixes-5.9-2' of git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms into irq/urgent
git bisect good ceb2465c51195967f11f6507538579816ac67cb8
# bad: [b69bea8a657b681442765b06be92a2607b1bd875] Merge tag 'locking-urgent-2020-08-30' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad b69bea8a657b681442765b06be92a2607b1bd875
# good: [00b0ed2d4997af6d0a93edef820386951fd66d94] locking/lockdep: Cleanup
git bisect good 00b0ed2d4997af6d0a93edef820386951fd66d94
# good: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges
git bisect good 044d0d6de9f50192f9697583504a382347ee95ca
# good: [3edd8db2d53fe6c96ad66248bb1479ae62807268] Merge tag '5.9-rc2-smb-fix' of git://git.samba.org/sfrench/cifs-2.6
git bisect good 3edd8db2d53fe6c96ad66248bb1479ae62807268
# bad: [eb1f00237aca2e368b93db79303f8433d1976d10] lockdep,trace: Expose tracepoints
git bisect bad eb1f00237aca2e368b93db79303f8433d1976d10
# first bad commit: [eb1f00237aca2e368b93db79303f8433d1976d10] lockdep,trace: Expose tracepoints

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[peterz]

On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
> > The lockdep tracepoints are under the lockdep recursion counter, this
> > has a bunch of nasty side effects:
> > 
> >  - TRACE_IRQFLAGS doesn't work across the entire tracepoint
> > 
> >  - RCU-lockdep doesn't see the tracepoints either, hiding numerous
> >    "suspicious RCU usage" warnings.
> > 
> > Pull the trace_lock_*() tracepoints completely out from under the
> > lockdep recursion handling and completely rely on the trace level
> > recusion handling -- also, tracing *SHOULD* not be taking locks in any
> > case.
> > 
> 
> Wonder what is worse - the problem or its fix. This patch results in
> a number of WARNING backtraces for several archtectures/platforms.
> Reverting it fixes the problems.

Without all this there was a recursion that could crash. But yes,
tedious.

OTOH the warnings are about real bugs that were pre-existing, we now see
them and can fix them.

I'll reply to ARM separately, but let's have a peek at s390.

> s390:
> 
> [   19.490586] =============================
> [   19.490752] WARNING: suspicious RCU usage
> [   19.490921] 5.9.0-rc3 #1 Not tainted
> [   19.491086] -----------------------------
> [   19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!

> [   19.493147]  [<00000000001d5de2>] lock_acquire+0x41a/0x498
> [   19.493320]  [<0000000000103b72>] enabled_wait+0xca/0x198
> [   19.493493]  [<0000000000103f80>] arch_cpu_idle+0x20/0x38

Does this help?

---

diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
index c73f50649e7e..f7f1e64e0d98 100644
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -39,14 +39,13 @@ void enabled_wait(void)
 	local_irq_restore(flags);
 
 	/* Account time spent with enabled wait psw loaded as idle time. */
-	/* XXX seqcount has tracepoints that require RCU */
-	write_seqcount_begin(&idle->seqcount);
+	raw_write_seqcount_begin(&idle->seqcount);
 	idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
 	idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
 	idle->idle_time += idle_time;
 	idle->idle_count++;
 	account_idle_time(cputime_to_nsecs(idle_time));
-	write_seqcount_end(&idle->seqcount);
+	raw_write_seqcount_end(&idle->seqcount);
 }
 NOKPROBE_SYMBOL(enabled_wait);
 

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Guenter Roeck]

On 9/2/20 12:24 AM, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
>> On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
>>> The lockdep tracepoints are under the lockdep recursion counter, this
>>> has a bunch of nasty side effects:
>>>
>>>  - TRACE_IRQFLAGS doesn't work across the entire tracepoint
>>>
>>>  - RCU-lockdep doesn't see the tracepoints either, hiding numerous
>>>    "suspicious RCU usage" warnings.
>>>
>>> Pull the trace_lock_*() tracepoints completely out from under the
>>> lockdep recursion handling and completely rely on the trace level
>>> recusion handling -- also, tracing *SHOULD* not be taking locks in any
>>> case.
>>>
>>
>> Wonder what is worse - the problem or its fix. This patch results in
>> a number of WARNING backtraces for several archtectures/platforms.
>> Reverting it fixes the problems.
> 
> Without all this there was a recursion that could crash. But yes,
> tedious.
> 
> OTOH the warnings are about real bugs that were pre-existing, we now see
> them and can fix them.
> 
> I'll reply to ARM separately, but let's have a peek at s390.
> 
>> s390:
>>
>> [   19.490586] =============================
>> [   19.490752] WARNING: suspicious RCU usage
>> [   19.490921] 5.9.0-rc3 #1 Not tainted
>> [   19.491086] -----------------------------
>> [   19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
> 
>> [   19.493147]  [<00000000001d5de2>] lock_acquire+0x41a/0x498
>> [   19.493320]  [<0000000000103b72>] enabled_wait+0xca/0x198
>> [   19.493493]  [<0000000000103f80>] arch_cpu_idle+0x20/0x38
> 
> Does this help?
> 
> ---
> 
> diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
> index c73f50649e7e..f7f1e64e0d98 100644
> --- a/arch/s390/kernel/idle.c
> +++ b/arch/s390/kernel/idle.c
> @@ -39,14 +39,13 @@ void enabled_wait(void)
>  	local_irq_restore(flags);
>  
>  	/* Account time spent with enabled wait psw loaded as idle time. */
> -	/* XXX seqcount has tracepoints that require RCU */
> -	write_seqcount_begin(&idle->seqcount);
> +	raw_write_seqcount_begin(&idle->seqcount);
>  	idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
>  	idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
>  	idle->idle_time += idle_time;
>  	idle->idle_count++;
>  	account_idle_time(cputime_to_nsecs(idle_time));
> -	write_seqcount_end(&idle->seqcount);
> +	raw_write_seqcount_end(&idle->seqcount);
>  }
>  NOKPROBE_SYMBOL(enabled_wait);
>  

Yes, it does.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[peterz]

On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:

> [   27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!

> [   27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
> [   27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
> [   27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
> [   27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
> [   27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
> [   27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
> [   27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
> [   27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
> [   27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)

ARM cpuidle is a trainwreck :/

So it looks like we have:

 - clkdm_
 - pwrdm_
 - cpu_pm_
 - pm_runtime_

In that approximate order, and then there's the coupled idle muck.
Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
around in drivers.

How about we unconditionally kill tracing when RCU is idle? Yes this is
a hack, and we really should turn it into a WARN in due time.

The thing is, we're shutting down clock/power domains and god knows
what, the CPU is in a crap state, we'll not take interrupts, but tracing
must happen! Hell no.

Let's make the rule that if you want something traced, you get to pull
it out of the cpuidle driver and stick it in the generic code with a
CPUIDLE_FLAG, before rcu_idle_enter().

Totally untested patch below..

---

diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
  *         SOFTIRQ_MASK:	0x0000ff00
  *         HARDIRQ_MASK:	0x000f0000
  *             NMI_MASK:	0x00f00000
+ *         RCUIDLE_MASK:	0x01000000
  * PREEMPT_NEED_RESCHED:	0x80000000
  */
 #define PREEMPT_BITS	8
 #define SOFTIRQ_BITS	8
 #define HARDIRQ_BITS	4
 #define NMI_BITS	4
+#define RCUIDLE_BITS	1
 
 #define PREEMPT_SHIFT	0
 #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
 #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
 
 #define __IRQ_MASK(x)	((1UL << (x))-1)
 
@@ -45,11 +48,13 @@
 #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)
+#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
 
 #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
 #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
 
 #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
 
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 598fec9f9dbf..997472c662c4 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -164,7 +164,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		void *__data;						\
 		int __maybe_unused __idx = 0;				\
 									\
-		if (!(cond))						\
+		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
 			return;						\
 									\
 		/* srcu can't be used from NMI */			\
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
 	/* Better not have special action (TLB flush) pending! */
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     (seq & RCU_DYNTICK_CTRL_MASK));
+
+	__preempt_count_add(RCUIDLE_OFFSET);
 }
 
 /*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 	int seq;
 
+	__preempt_count_sub(RCUIDLE_OFFSET);
+
 	/*
 	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
 	 * and we also must force ordering with the next RCU read-side

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Guenter Roeck]

On 9/2/20 1:56 AM, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> 
>> [   27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
> 
>> [   27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
>> [   27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
>> [   27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
>> [   27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
>> [   27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
>> [   27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
>> [   27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
>> [   27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
>> [   27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
> 
> ARM cpuidle is a trainwreck :/
> 
> So it looks like we have:
> 
>  - clkdm_
>  - pwrdm_
>  - cpu_pm_
>  - pm_runtime_
> 
> In that approximate order, and then there's the coupled idle muck.
> Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
> around in drivers.
> 
> How about we unconditionally kill tracing when RCU is idle? Yes this is
> a hack, and we really should turn it into a WARN in due time.
> 
> The thing is, we're shutting down clock/power domains and god knows
> what, the CPU is in a crap state, we'll not take interrupts, but tracing
> must happen! Hell no.
> 
> Let's make the rule that if you want something traced, you get to pull
> it out of the cpuidle driver and stick it in the generic code with a
> CPUIDLE_FLAG, before rcu_idle_enter().
> 
> Totally untested patch below..
> 

Unfortunately, that patch does not make a difference; I still see the
same tracebacks with it applied.

Guenter

> ---
> 
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
>   *         SOFTIRQ_MASK:	0x0000ff00
>   *         HARDIRQ_MASK:	0x000f0000
>   *             NMI_MASK:	0x00f00000
> + *         RCUIDLE_MASK:	0x01000000
>   * PREEMPT_NEED_RESCHED:	0x80000000
>   */
>  #define PREEMPT_BITS	8
>  #define SOFTIRQ_BITS	8
>  #define HARDIRQ_BITS	4
>  #define NMI_BITS	4
> +#define RCUIDLE_BITS	1
>  
>  #define PREEMPT_SHIFT	0
>  #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
>  #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
>  #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
>  
>  #define __IRQ_MASK(x)	((1UL << (x))-1)
>  
> @@ -45,11 +48,13 @@
>  #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)
> +#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>  
>  #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
>  #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
>  #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
>  #define NMI_OFFSET	(1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
>  
>  #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
>  
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 598fec9f9dbf..997472c662c4 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -164,7 +164,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  		void *__data;						\
>  		int __maybe_unused __idx = 0;				\
>  									\
> -		if (!(cond))						\
> +		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
>  			return;						\
>  									\
>  		/* srcu can't be used from NMI */			\
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
>  	/* Better not have special action (TLB flush) pending! */
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>  		     (seq & RCU_DYNTICK_CTRL_MASK));
> +
> +	__preempt_count_add(RCUIDLE_OFFSET);
>  }
>  
>  /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  	int seq;
>  
> +	__preempt_count_sub(RCUIDLE_OFFSET);
> +
>  	/*
>  	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
>  	 * and we also must force ordering with the next RCU read-side
> 

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[peterz]

On Wed, Sep 02, 2020 at 06:57:36AM -0700, Guenter Roeck wrote:
> On 9/2/20 1:56 AM, peterz@infradead.org wrote:
> > On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> > 
> >> [   27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
> > 
> >> [   27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
> >> [   27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
> >> [   27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
> >> [   27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
> >> [   27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
> >> [   27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
> >> [   27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
> >> [   27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
> >> [   27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
> > 
> > ARM cpuidle is a trainwreck :/
> > 
> > So it looks like we have:
> > 
> >  - clkdm_
> >  - pwrdm_
> >  - cpu_pm_
> >  - pm_runtime_
> > 
> > In that approximate order, and then there's the coupled idle muck.
> > Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
> > around in drivers.
> > 
> > How about we unconditionally kill tracing when RCU is idle? Yes this is
> > a hack, and we really should turn it into a WARN in due time.
> > 
> > The thing is, we're shutting down clock/power domains and god knows
> > what, the CPU is in a crap state, we'll not take interrupts, but tracing
> > must happen! Hell no.
> > 
> > Let's make the rule that if you want something traced, you get to pull
> > it out of the cpuidle driver and stick it in the generic code with a
> > CPUIDLE_FLAG, before rcu_idle_enter().
> > 
> > Totally untested patch below..
> > 
> 
> Unfortunately, that patch does not make a difference; I still see the
> same tracebacks with it applied.

I stuck a tracepoint in intel_idle and had a rummage around. The below
seems to work for me now.

---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
  *         SOFTIRQ_MASK:	0x0000ff00
  *         HARDIRQ_MASK:	0x000f0000
  *             NMI_MASK:	0x00f00000
+ *         RCUIDLE_MASK:	0x01000000
  * PREEMPT_NEED_RESCHED:	0x80000000
  */
 #define PREEMPT_BITS	8
 #define SOFTIRQ_BITS	8
 #define HARDIRQ_BITS	4
 #define NMI_BITS	4
+#define RCUIDLE_BITS	1
 
 #define PREEMPT_SHIFT	0
 #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
 #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
 
 #define __IRQ_MASK(x)	((1UL << (x))-1)
 
@@ -45,11 +48,13 @@
 #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)
+#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
 
 #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
 #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
 
 #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
 
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 3722a10fc46d..5bc45f6750f5 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -172,12 +172,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		int __maybe_unused __idx = 0;				\
 		void *__data;						\
 									\
-		if (!(cond))						\
+		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
 			return;						\
 									\
 		/* srcu can't be used from NMI */			\
 		WARN_ON_ONCE(rcuidle && in_nmi());			\
 									\
+		if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) {		\
+			rcu_read_lock_sched_notrace();			\
+			rcu_dereference_sched(__tracepoint_##name.funcs);\
+			rcu_read_unlock_sched_notrace();		\
+		}							\
+									\
 		/* keep srcu and sched-rcu usage consistent */		\
 		preempt_disable_notrace();				\
 									\
@@ -242,11 +248,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 				TP_PROTO(data_proto),			\
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond), 0);			\
-		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
-			rcu_read_lock_sched_notrace();			\
-			rcu_dereference_sched(__tracepoint_##name.funcs);\
-			rcu_read_unlock_sched_notrace();		\
-		}							\
 	}								\
 	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
 		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
 	/* Better not have special action (TLB flush) pending! */
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     (seq & RCU_DYNTICK_CTRL_MASK));
+
+	__preempt_count_add(RCUIDLE_OFFSET);
 }
 
 /*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 	int seq;
 
+	__preempt_count_sub(RCUIDLE_OFFSET);
+
 	/*
 	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
 	 * and we also must force ordering with the next RCU read-side

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[peterz]

On Thu, Sep 03, 2020 at 04:00:47PM +0200, peterz@infradead.org wrote:
> I stuck a tracepoint in intel_idle and had a rummage around. The below
> seems to work for me now.

Note that this will insta-kill all trace_*_rcuidle() tracepoint that are
actually used in rcuidle.

A git-grep seems to suggest the remaining users are:

 - arch/arm
 - arch/arm64 -- IPI tracepoint that are ordered incorrectly in
                 their entry code,

 - runtime pm
 - common clk -- these both need re-ordering somehow

 - trace_preemptirq -- this is going to be interesting for
                       those archs that idle with IRQs enabled

 - printk -- we shouldn't be printk()-ing from idle, and if we are,
             missing this is the least of our problems.

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Guenter Roeck]

On 9/3/20 7:00 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 06:57:36AM -0700, Guenter Roeck wrote:
>> On 9/2/20 1:56 AM, peterz@infradead.org wrote:
>>> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
>>>
>>>> [   27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
>>>
>>>> [   27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
>>>> [   27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
>>>> [   27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
>>>> [   27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
>>>> [   27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
>>>> [   27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
>>>> [   27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
>>>> [   27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
>>>> [   27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
>>>
>>> ARM cpuidle is a trainwreck :/
>>>
>>> So it looks like we have:
>>>
>>>  - clkdm_
>>>  - pwrdm_
>>>  - cpu_pm_
>>>  - pm_runtime_
>>>
>>> In that approximate order, and then there's the coupled idle muck.
>>> Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
>>> around in drivers.
>>>
>>> How about we unconditionally kill tracing when RCU is idle? Yes this is
>>> a hack, and we really should turn it into a WARN in due time.
>>>
>>> The thing is, we're shutting down clock/power domains and god knows
>>> what, the CPU is in a crap state, we'll not take interrupts, but tracing
>>> must happen! Hell no.
>>>
>>> Let's make the rule that if you want something traced, you get to pull
>>> it out of the cpuidle driver and stick it in the generic code with a
>>> CPUIDLE_FLAG, before rcu_idle_enter().
>>>
>>> Totally untested patch below..
>>>
>>
>> Unfortunately, that patch does not make a difference; I still see the
>> same tracebacks with it applied.
> 
> I stuck a tracepoint in intel_idle and had a rummage around. The below
> seems to work for me now.
> 
> ---
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
>   *         SOFTIRQ_MASK:	0x0000ff00
>   *         HARDIRQ_MASK:	0x000f0000
>   *             NMI_MASK:	0x00f00000
> + *         RCUIDLE_MASK:	0x01000000
>   * PREEMPT_NEED_RESCHED:	0x80000000
>   */
>  #define PREEMPT_BITS	8
>  #define SOFTIRQ_BITS	8
>  #define HARDIRQ_BITS	4
>  #define NMI_BITS	4
> +#define RCUIDLE_BITS	1
>  
>  #define PREEMPT_SHIFT	0
>  #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
>  #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
>  #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
>  
>  #define __IRQ_MASK(x)	((1UL << (x))-1)
>  
> @@ -45,11 +48,13 @@
>  #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)
> +#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>  
>  #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
>  #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
>  #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
>  #define NMI_OFFSET	(1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
>  
>  #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
>  
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 3722a10fc46d..5bc45f6750f5 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -172,12 +172,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  		int __maybe_unused __idx = 0;				\
>  		void *__data;						\
>  									\
> -		if (!(cond))						\
> +		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
>  			return;						\
>  									\
>  		/* srcu can't be used from NMI */			\
>  		WARN_ON_ONCE(rcuidle && in_nmi());			\
>  									\
> +		if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) {		\
> +			rcu_read_lock_sched_notrace();			\
> +			rcu_dereference_sched(__tracepoint_##name.funcs);\

This doesn't compile for me - there is no "name" parameter in __DO_TRACE().

Error log:
In file included from ./include/linux/rculist.h:11,
                 from ./include/linux/pid.h:5,
                 from ./include/linux/sched.h:14,
                 from ./include/linux/sched/numa_balancing.h:10,
                 from ./include/trace/events/sched.h:8,
                 from kernel/sched/core.c:10:
./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared

I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
a different branch ?

Guenter

> +			rcu_read_unlock_sched_notrace();		\
> +		}							\
> +									\
>  		/* keep srcu and sched-rcu usage consistent */		\
>  		preempt_disable_notrace();				\
>  									\
> @@ -242,11 +248,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  				TP_PROTO(data_proto),			\
>  				TP_ARGS(data_args),			\
>  				TP_CONDITION(cond), 0);			\
> -		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
> -			rcu_read_lock_sched_notrace();			\
> -			rcu_dereference_sched(__tracepoint_##name.funcs);\
> -			rcu_read_unlock_sched_notrace();		\
> -		}							\
>  	}								\
>  	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
>  		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
>  	/* Better not have special action (TLB flush) pending! */
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>  		     (seq & RCU_DYNTICK_CTRL_MASK));
> +
> +	__preempt_count_add(RCUIDLE_OFFSET);
>  }
>  
>  /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  	int seq;
>  
> +	__preempt_count_sub(RCUIDLE_OFFSET);
> +
>  	/*
>  	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
>  	 * and we also must force ordering with the next RCU read-side
> 

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[peterz]

On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
> This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
> 
> Error log:
> In file included from ./include/linux/rculist.h:11,
>                  from ./include/linux/pid.h:5,
>                  from ./include/linux/sched.h:14,
>                  from ./include/linux/sched/numa_balancing.h:10,
>                  from ./include/trace/events/sched.h:8,
>                  from kernel/sched/core.c:10:
> ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
> ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
> 
> I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
> a different branch ?

Argh, I was on tip/master... easy fix though.

---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
  *         SOFTIRQ_MASK:	0x0000ff00
  *         HARDIRQ_MASK:	0x000f0000
  *             NMI_MASK:	0x00f00000
+ *         RCUIDLE_MASK:	0x01000000
  * PREEMPT_NEED_RESCHED:	0x80000000
  */
 #define PREEMPT_BITS	8
 #define SOFTIRQ_BITS	8
 #define HARDIRQ_BITS	4
 #define NMI_BITS	4
+#define RCUIDLE_BITS	1
 
 #define PREEMPT_SHIFT	0
 #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
 #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
 
 #define __IRQ_MASK(x)	((1UL << (x))-1)
 
@@ -45,11 +48,13 @@
 #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)
+#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
 
 #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
 #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
 
 #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
 
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 598fec9f9dbf..0469bc1c24fc 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -164,12 +164,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		void *__data;						\
 		int __maybe_unused __idx = 0;				\
 									\
-		if (!(cond))						\
+		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
 			return;						\
 									\
 		/* srcu can't be used from NMI */			\
 		WARN_ON_ONCE(rcuidle && in_nmi());			\
 									\
+		if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) {		\
+			rcu_read_lock_sched_notrace();			\
+			rcu_dereference_sched((tp)->funcs);		\
+			rcu_read_unlock_sched_notrace();		\
+		}							\
+									\
 		/* keep srcu and sched-rcu usage consistent */		\
 		preempt_disable_notrace();				\
 									\
@@ -235,11 +241,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 				TP_PROTO(data_proto),			\
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond), 0);			\
-		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
-			rcu_read_lock_sched_notrace();			\
-			rcu_dereference_sched(__tracepoint_##name.funcs);\
-			rcu_read_unlock_sched_notrace();		\
-		}							\
 	}								\
 	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
 		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
 	/* Better not have special action (TLB flush) pending! */
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     (seq & RCU_DYNTICK_CTRL_MASK));
+
+	__preempt_count_add(RCUIDLE_OFFSET);
 }
 
 /*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 	int seq;
 
+	__preempt_count_sub(RCUIDLE_OFFSET);
+
 	/*
 	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
 	 * and we also must force ordering with the next RCU read-side

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Paul E. McKenney]

On Thu, Sep 03, 2020 at 05:36:49PM +0200, peterz@infradead.org wrote:
> On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
> > This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
> > 
> > Error log:
> > In file included from ./include/linux/rculist.h:11,
> >                  from ./include/linux/pid.h:5,
> >                  from ./include/linux/sched.h:14,
> >                  from ./include/linux/sched/numa_balancing.h:10,
> >                  from ./include/trace/events/sched.h:8,
> >                  from kernel/sched/core.c:10:
> > ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
> > ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
> > 
> > I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
> > a different branch ?
> 
> Argh, I was on tip/master... easy fix though.
> 
> ---
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
>   *         SOFTIRQ_MASK:	0x0000ff00
>   *         HARDIRQ_MASK:	0x000f0000
>   *             NMI_MASK:	0x00f00000
> + *         RCUIDLE_MASK:	0x01000000
>   * PREEMPT_NEED_RESCHED:	0x80000000
>   */
>  #define PREEMPT_BITS	8
>  #define SOFTIRQ_BITS	8
>  #define HARDIRQ_BITS	4
>  #define NMI_BITS	4
> +#define RCUIDLE_BITS	1
>  
>  #define PREEMPT_SHIFT	0
>  #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
>  #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
>  #define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT	(NMI_SHIFT     + NMI_BITS)
>  
>  #define __IRQ_MASK(x)	((1UL << (x))-1)
>  
> @@ -45,11 +48,13 @@
>  #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)
> +#define RCUIDLE_MASK	(__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>  
>  #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
>  #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
>  #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
>  #define NMI_OFFSET	(1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET	(1UL << RCUIDLE_SHIFT)
>  
>  #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
>  
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 598fec9f9dbf..0469bc1c24fc 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -164,12 +164,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  		void *__data;						\
>  		int __maybe_unused __idx = 0;				\
>  									\
> -		if (!(cond))						\
> +		if (!(cond) || (preempt_count() & RCUIDLE_MASK))	\
>  			return;						\
>  									\
>  		/* srcu can't be used from NMI */			\
>  		WARN_ON_ONCE(rcuidle && in_nmi());			\
>  									\
> +		if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) {		\
> +			rcu_read_lock_sched_notrace();			\
> +			rcu_dereference_sched((tp)->funcs);		\
> +			rcu_read_unlock_sched_notrace();		\
> +		}							\
> +									\
>  		/* keep srcu and sched-rcu usage consistent */		\
>  		preempt_disable_notrace();				\
>  									\
> @@ -235,11 +241,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  				TP_PROTO(data_proto),			\
>  				TP_ARGS(data_args),			\
>  				TP_CONDITION(cond), 0);			\
> -		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
> -			rcu_read_lock_sched_notrace();			\
> -			rcu_dereference_sched(__tracepoint_##name.funcs);\
> -			rcu_read_unlock_sched_notrace();		\
> -		}							\
>  	}								\
>  	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
>  		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
>  	/* Better not have special action (TLB flush) pending! */
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>  		     (seq & RCU_DYNTICK_CTRL_MASK));
> +
> +	__preempt_count_add(RCUIDLE_OFFSET);

But the rcu_data structure's ->dynticks field is still to be used in cases
where the RCU grace-period kthread must do off-CPU access to the CPU's
RCU-idle state, correct?  (Otherwise, some memory barriers are needed.)

							Thanx, Paul

>  }
>  
>  /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  	int seq;
>  
> +	__preempt_count_sub(RCUIDLE_OFFSET);
> +
>  	/*
>  	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
>  	 * and we also must force ordering with the next RCU read-side
> 
> 

Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints

[Guenter Roeck]

On 9/3/20 8:36 AM, peterz@infradead.org wrote:
> On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
>> This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
>>
>> Error log:
>> In file included from ./include/linux/rculist.h:11,
>>                  from ./include/linux/pid.h:5,
>>                  from ./include/linux/sched.h:14,
>>                  from ./include/linux/sched/numa_balancing.h:10,
>>                  from ./include/trace/events/sched.h:8,
>>                  from kernel/sched/core.c:10:
>> ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
>> ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
>>
>> I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
>> a different branch ?
> 
> Argh, I was on tip/master... easy fix though.

That version silences the tracebacks on arm. I can't test on real hardware,
so I don't know if it also fixes the problem reported at [1] (which seems
related).

Guenter

---
[1] https://lore.kernel.org/lkml/CA+G9fYuiJwN1ad955Xw4ShamX2=373r+56KsbpeverEs+i_NAg@mail.gmail.com/t/#u

Re: [PATCH v2 00/11] TRACE_IRQFLAGS wreckage

[peterz]

On Fri, Aug 21, 2020 at 10:47:38AM +0200, Peter Zijlstra wrote:

> TRACE_IRQFLAGS

> ---
> 
>  arch/arm/mach-omap2/pm34xx.c      |    4 --
>  arch/arm64/include/asm/irqflags.h |    5 ++
>  arch/arm64/kernel/process.c       |    2 -
>  arch/nds32/include/asm/irqflags.h |    5 ++
>  arch/powerpc/include/asm/hw_irq.h |   11 ++---
>  arch/s390/kernel/idle.c           |    3 -
>  arch/x86/entry/thunk_32.S         |    5 --
>  arch/x86/include/asm/mmu.h        |    1 
>  arch/x86/kernel/process.c         |    4 --
>  arch/x86/mm/tlb.c                 |   13 +-----
>  drivers/cpuidle/cpuidle.c         |   19 +++++++--
>  drivers/idle/intel_idle.c         |   16 --------
>  include/linux/cpuidle.h           |   13 +++---
>  include/linux/irqflags.h          |   73 ++++++++++++++++++++------------------
>  include/linux/lockdep.h           |   18 ++++++---
>  include/linux/mmu_context.h       |    5 ++
>  kernel/locking/lockdep.c          |   18 +++++----
>  kernel/sched/idle.c               |   25 +++++--------
>  18 files changed, 118 insertions(+), 122 deletions(-)

Whole set also available at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/wip

[PATCH v2 12/11] mips: Implement arch_irqs_disabled()

[peterz]

Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/mips/include/asm/irqflags.h |    5 +++++
 1 file changed, 5 insertions(+)

--- a/arch/mips/include/asm/irqflags.h
+++ b/arch/mips/include/asm/irqflags.h
@@ -137,6 +137,11 @@ static inline int arch_irqs_disabled_fla
 	return !(flags & 1);
 }
 
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
 #endif /* #ifndef __ASSEMBLY__ */
 
 /*

[tip: locking/core] mips: Implement arch_irqs_disabled()

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     99dc56feb7932020502d40107a712fa302b32082
Gitweb:        https://git.kernel.org/tip/99dc56feb7932020502d40107a712fa302b32082
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Sat, 22 Aug 2020 18:04:15 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00

mips: Implement arch_irqs_disabled()

Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200826101653.GE1362448@hirez.programming.kicks-ass.net
---
 arch/mips/include/asm/irqflags.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/include/asm/irqflags.h b/arch/mips/include/asm/irqflags.h
index 47a8ffc..f5b8300 100644
--- a/arch/mips/include/asm/irqflags.h
+++ b/arch/mips/include/asm/irqflags.h
@@ -137,6 +137,11 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
 	return !(flags & 1);
 }
 
+static inline int arch_irqs_disabled(void)
+{
+	return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
 #endif /* #ifndef __ASSEMBLY__ */
 
 /*