drivers/perf: arm-pmu: fix RCU usage on resume from idle states

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Hi Lorenzo,

Quoted 58 lines · Lorenzo Pieralisi

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

Quoted 3 lines

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

This is horrible, but I think it's the best we can do without completely redesigning the way in which we save/restore the PMU state. We should do that, but not for 4.6!

Acked-by Will Deacon <[email protected]>

Quoted 65 lines · Will Deacon

Hi Lorenzo,

Quoted 61 lines · Lorenzo Pieralisi

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Quoted 5 lines

This is horrible, but I think it's the best we can do without completely redesigning the way in which we save/restore the PMU state. We should do that, but not for 4.6!

Acked-by Will Deacon <[email protected]>

Maybe RCU_NONIDLE() will help here?

Quoted 64 lines · Kevin Hilman

Quoted 63 lines · Will Deacon

Quoted 61 lines · Lorenzo Pieralisi

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Quoted 7 lines

Quoted 5 lines

This is horrible, but I think it's the best we can do without completely redesigning the way in which we save/restore the PMU state. We should do that, but not for 4.6!

Acked-by Will Deacon <[email protected]>

Maybe RCU_NONIDLE() will help here?

Thanks for chiming in.

CPU_PM notifiers are called from process context (which is not necessarily the idle thread) with IRQs disabled from:

- CPUidle drivers state enter calls
- syscore callbacks (ie suspend2RAM - suspend thread)
- bL switcher
- MCPM loopback

[+ Paul, Nico]

Quoted 67 lines · Kevin Hilman

Quoted 65 lines · Will Deacon

Hi Lorenzo,

Quoted 61 lines · Lorenzo Pieralisi

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Quoted 7 lines

Quoted 5 lines

This is horrible, but I think it's the best we can do without completely redesigning the way in which we save/restore the PMU state. We should do that, but not for 4.6!

Acked-by Will Deacon <[email protected]>

Maybe RCU_NONIDLE() will help here?

Thanks for chiming in.

CPU_PM notifiers are called from process context (which is not necessarily the idle thread) with IRQs disabled from:

- CPUidle drivers state enter calls
- syscore callbacks (ie suspend2RAM - suspend thread)
- bL switcher
- MCPM loopback

The questions I have are:

- Is it safe to wrap a call (in this case armpmu_start()) with RCU_NONIDLE if the core is not actually executing the idle thread ? The function requiring rcu locks/dereferences is perf_event_update_userpage().

- What are RCU_NONIDLE side-effects (ie what can be actually called from within an RCU_NONIDLE wrapper ?)

It would be nice if we can use it instead of merging this patch, I need more insights into RCU_NONIDLE usage though before proceeding.

Quoted 12 lines

Quoted 1 line

Maybe RCU_NONIDLE() will help here?

Thanks for chiming in.

CPU_PM notifiers are called from process context (which is not necessarily the idle thread) with IRQs disabled from:

- CPUidle drivers state enter calls
- syscore callbacks (ie suspend2RAM - suspend thread)
- bL switcher
- MCPM loopback

Quoted 5 lines

The questions I have are:

- Is it safe to wrap a call (in this case armpmu_start()) with RCU_NONIDLE if the core is not actually executing the idle thread ? The function requiring rcu locks/dereferences is perf_event_update_userpage().

Yes it is.

Quoted 2 lines

- What are RCU_NONIDLE side-effects (ie what can be actually called from within an RCU_NONIDLE wrapper ?)

There are a few restrictions:

1. Code within RCU_NONIDLE() cannot block. Then again, neither
can the idle task. ;-)

2. RCU_NONIDLE() can be nested, but not indefinitely. Then again,
given that the limit even on a 32-bit system is something like a million, I bet you hit compiler or stack-size limits long before you overflow RCU_NONIDLE()'s counter.

3. You can neither branch into the middle of RCU_NONIDLE()'s code

	nor branch out from the middle of RCU_NONIDLE()'s code.
	Calling functions is just fine, but things like this are not:

		RCU_NONIDLE({
			do_something();
			goto bad_idea;	/* BUG!!! */
			do_something_else();});
		do_yet_a_third_thing();
	bad_idea:

	Branching -within- the RCU_NONIDLE() code is just fine.

Yes, and I am adding this information to RCU_NONIDLE()'s header comment, apologies for its not being there to begin with! (See below for patches.)

Quoted 2 lines

It would be nice if we can use it instead of merging this patch, I need more insights into RCU_NONIDLE usage though before proceeding.

Please let me know if any of the above restrictions cause you a problem.

On Wed, Apr 20, 2016 at 09:23:54AM -0700, Paul E. McKenney wrote:

[...]

Quoted 46 lines

Quoted 12 lines

Quoted 1 line

Maybe RCU_NONIDLE() will help here?

Thanks for chiming in.

CPU_PM notifiers are called from process context (which is not necessarily the idle thread) with IRQs disabled from:

- CPUidle drivers state enter calls
- syscore callbacks (ie suspend2RAM - suspend thread)
- bL switcher
- MCPM loopback

Quoted 5 lines

The questions I have are:

- Is it safe to wrap a call (in this case armpmu_start()) with RCU_NONIDLE if the core is not actually executing the idle thread ? The function requiring rcu locks/dereferences is perf_event_update_userpage().

Yes it is.

Quoted 2 lines

- What are RCU_NONIDLE side-effects (ie what can be actually called from within an RCU_NONIDLE wrapper ?)

There are a few restrictions:

1. Code within RCU_NONIDLE() cannot block. Then again, neither
can the idle task. ;-)

2. RCU_NONIDLE() can be nested, but not indefinitely. Then again,
given that the limit even on a 32-bit system is something like a million, I bet you hit compiler or stack-size limits long before you overflow RCU_NONIDLE()'s counter.

3. You can neither branch into the middle of RCU_NONIDLE()'s code

	nor branch out from the middle of RCU_NONIDLE()'s code.
	Calling functions is just fine, but things like this are not:

		RCU_NONIDLE({
			do_something();
			goto bad_idea;	/* BUG!!! */
			do_something_else();});
		do_yet_a_third_thing();
	bad_idea:

	Branching -within- the RCU_NONIDLE() code is just fine.

Quoted 3 lines

Yes, and I am adding this information to RCU_NONIDLE()'s header comment, apologies for its not being there to begin with! (See below for patches.)

Thank you for the explanation, that's now clear. For my own understanding:
RCU_NONIDLE() is a way to inform the RCU subsystem that the CPU in question should be temporarily *watched* (ie it is not idle from an RCU standpoint), correct ?

Quoted 4 lines

Quoted 2 lines

It would be nice if we can use it instead of merging this patch, I need more insights into RCU_NONIDLE usage though before proceeding.

Please let me know if any of the above restrictions cause you a problem.

I can't think of any, perf_event_update_userpage(), that I will call indirectly through:

RCU_NONIDLE(armpmu_start());

is not allowed to block anyway, so I think we have a much better solution than this one, new patch coming, Catalin please drop this one.

Thank you all !

Quoted 58 lines · Lorenzo Pieralisi

Commit da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier") added code in the arm perf infrastructure that allows the kernel to save/restore perf counters whenever the CPU enters a low-power idle state. The kernel saves/restores the counters for each active event through the armpmu_{stop/start} ARM pmu API, so that the idle state enter/exit power cycle is emulated through pmu start/stop operations for each event in use.

However, calling armpmu_start() for each active event on power up executes code that requires RCU locking (perf_event_update_userpage()) to be functional, so, given that the core may call the CPU_PM notifiers while running the idle thread in an quiescent RCU state this is not allowed as detected through the following splat when kernel is run with CONFIG_PROVE_LOCKING enabled:

[   49.293286]
[   49.294761] ===============================
[   49.298895] [ INFO: suspicious RCU usage. ]
[   49.303031] 4.6.0-rc3+ #421 Not tainted
[   49.306821] -------------------------------
[   49.310956] include/linux/rcupdate.h:872 rcu_read_lock() used
illegally while idle!
[   49.318530]
[   49.318530] other info that might help us debug this:
[   49.318530]
[   49.326451]
[   49.326451] RCU used illegally from idle CPU!
[   49.326451] rcu_scheduler_active = 1, debug_locks = 0
[   49.337209] RCU used illegally from extended quiescent state!
[   49.342892] 2 locks held by swapper/2/0:
[   49.346768]  #0:  (cpu_pm_notifier_lock){......}, at:
[<ffffff8008163c28>] cpu_pm_exit+0x18/0x80
[   49.355492]  #1:  (rcu_read_lock){......}, at: [<ffffff800816dc38>]

perf_event_update_userpage+0x0/0x260

This patch refactors the perf CPU_PM notifiers to add a boolean flag to the function updating the counters event period, so that the userpage update can be skipped when resuming from low-power whilst keeping correct save/restore functionality for the running events.

As a side effect the kernel, while resuming from low-power with perf events enabled, runs with a userspace view of active counters that is not up-to-date with the kernel one, but since the core power down is not really a PMU event start/stop this can be considered acceptable and the userspace event snapshot will update the user view of counters on subsequent perf event updates requested by either the perf API or event counters overflow-triggered interrupts.

Fixes da4e4f18afe0 ("drivers/perf: arm_pmu: implement CPU_PM notifier")

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reported-by James Morse <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

Quoted 3 lines

---
 drivers/perf/arm_pmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

This is horrible, but I think it's the best we can do without completely redesigning the way in which we save/restore the PMU state. We should do that, but not for 4.6!

Acked-by Will Deacon <[email protected]>

I assume Catalin will take this as a fix?