| Message ID | 20220919101520.869531945@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpuidle,rcu: Clean up the mess | expand |
On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > Doing RCU-idle outside the driver, only to then temporarily enable it > again, at least twice, before going idle is daft. Hmm, what ends up calling RCU_IDLE() here? Also what about cpu_do_idle()? Thanks. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui > cpu_pm_enter(); > cpu_cluster_pm_enter(); > > + ct_idle_enter(); > cpu_suspend(0, imx6sx_idle_finish); > + ct_idle_exit(); > > cpu_cluster_pm_exit(); > cpu_pm_exit(); > @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui > */ > .exit_latency = 300, > .target_residency = 500, > - .flags = CPUIDLE_FLAG_TIMER_STOP, > + .flags = CPUIDLE_FLAG_TIMER_STOP | > + CPUIDLE_FLAG_RCU_IDLE, > .enter = imx6sx_enter_wait, > .name = "LOW-POWER-IDLE", > .desc = "ARM power off", > >
On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > Doing RCU-idle outside the driver, only to then temporarily enable it > again, at least twice, before going idle is daft. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui > cpu_pm_enter(); > cpu_cluster_pm_enter(); > > + ct_idle_enter(); > cpu_suspend(0, imx6sx_idle_finish); > + ct_idle_exit(); > > cpu_cluster_pm_exit(); > cpu_pm_exit(); > @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui > */ > .exit_latency = 300, > .target_residency = 500, > - .flags = CPUIDLE_FLAG_TIMER_STOP, > + .flags = CPUIDLE_FLAG_TIMER_STOP | > + CPUIDLE_FLAG_RCU_IDLE, > .enter = imx6sx_enter_wait, There is a second one below that also uses imx6sx_enter_wait. Thanks. > .name = "LOW-POWER-IDLE", > .desc = "ARM power off", > >
On Mon, Sep 19, 2022 at 04:21:23PM +0200, Frederic Weisbecker wrote: > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > Doing RCU-idle outside the driver, only to then temporarily enable it > > again, at least twice, before going idle is daft. > > Hmm, what ends up calling RCU_IDLE() here? Also what about > cpu_do_idle()? Both cpu_pm_enter() and cpu_cluster_pm_enter() use ct_irq_enter_irqson() which is another way to spell RCU_NONIDLE().
On Mon, Sep 19, 2022 at 04:49:41PM +0200, Frederic Weisbecker wrote: > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > Doing RCU-idle outside the driver, only to then temporarily enable it > > again, at least twice, before going idle is daft. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > > @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui > > cpu_pm_enter(); > > cpu_cluster_pm_enter(); > > > > + ct_idle_enter(); > > cpu_suspend(0, imx6sx_idle_finish); > > + ct_idle_exit(); > > > > cpu_cluster_pm_exit(); > > cpu_pm_exit(); > > @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui > > */ > > .exit_latency = 300, > > .target_residency = 500, > > - .flags = CPUIDLE_FLAG_TIMER_STOP, > > + .flags = CPUIDLE_FLAG_TIMER_STOP | > > + CPUIDLE_FLAG_RCU_IDLE, > > .enter = imx6sx_enter_wait, > > There is a second one below that also uses imx6sx_enter_wait. Duh, thanks!
On Mon, Sep 19, 2022 at 04:49:41PM +0200, Frederic Weisbecker wrote: > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > Doing RCU-idle outside the driver, only to then temporarily enable it > > again, at least twice, before going idle is daft. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > > @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui > > cpu_pm_enter(); > > cpu_cluster_pm_enter(); > > > > + ct_idle_enter(); > > cpu_suspend(0, imx6sx_idle_finish); > > + ct_idle_exit(); > > > > cpu_cluster_pm_exit(); > > cpu_pm_exit(); > > @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui > > */ > > .exit_latency = 300, > > .target_residency = 500, > > - .flags = CPUIDLE_FLAG_TIMER_STOP, > > + .flags = CPUIDLE_FLAG_TIMER_STOP | > > + CPUIDLE_FLAG_RCU_IDLE, > > .enter = imx6sx_enter_wait, > > There is a second one below that also uses imx6sx_enter_wait. Oh, above you mean; but only @index==2 gets us into the whole PM crud. @index==1 is fine afaict.
On Mon, Sep 19, 2022 at 05:03:04PM +0200, Peter Zijlstra wrote: > On Mon, Sep 19, 2022 at 04:49:41PM +0200, Frederic Weisbecker wrote: > > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > > Doing RCU-idle outside the driver, only to then temporarily enable it > > > again, at least twice, before going idle is daft. > > > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > --- > > > arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > > > @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui > > > cpu_pm_enter(); > > > cpu_cluster_pm_enter(); > > > > > > + ct_idle_enter(); > > > cpu_suspend(0, imx6sx_idle_finish); > > > + ct_idle_exit(); > > > > > > cpu_cluster_pm_exit(); > > > cpu_pm_exit(); > > > @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui > > > */ > > > .exit_latency = 300, > > > .target_residency = 500, > > > - .flags = CPUIDLE_FLAG_TIMER_STOP, > > > + .flags = CPUIDLE_FLAG_TIMER_STOP | > > > + CPUIDLE_FLAG_RCU_IDLE, > > > .enter = imx6sx_enter_wait, > > > > There is a second one below that also uses imx6sx_enter_wait. > > Oh, above you mean; but only @index==2 gets us into the whole PM crud. > @index==1 is fine afaict. Ah ok, got it, hence why you didn't touch cpu_do_idle()... May need to comment that somewhere... Reviewed-by: Frederic Weisbecker <frederic@kernel.org> Thanks!
On Mon, Sep 19, 2022 at 04:21:23PM +0200, Frederic Weisbecker wrote: > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > Doing RCU-idle outside the driver, only to then temporarily enable it > > again, at least twice, before going idle is daft. > > Hmm, what ends up calling RCU_IDLE() here? Also what about > cpu_do_idle()? I've ammended patches 5-12 with a comment like: Notably both cpu_pm_enter() and cpu_cluster_pm_enter() implicity re-enable RCU. (each noting the specific sites for the relevant patch).
On Tue, Sep 20, 2022 at 10:58:59AM +0200, Peter Zijlstra wrote: > On Mon, Sep 19, 2022 at 04:21:23PM +0200, Frederic Weisbecker wrote: > > On Mon, Sep 19, 2022 at 11:59:47AM +0200, Peter Zijlstra wrote: > > > Doing RCU-idle outside the driver, only to then temporarily enable it > > > again, at least twice, before going idle is daft. > > > > Hmm, what ends up calling RCU_IDLE() here? Also what about > > cpu_do_idle()? > > I've ammended patches 5-12 with a comment like: > > Notably both cpu_pm_enter() and cpu_cluster_pm_enter() implicity > re-enable RCU. > > (each noting the specific sites for the relevant patch). Thanks!
--- a/arch/arm/mach-imx/cpuidle-imx6sx.c +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c @@ -47,7 +47,9 @@ static int imx6sx_enter_wait(struct cpui cpu_pm_enter(); cpu_cluster_pm_enter(); + ct_idle_enter(); cpu_suspend(0, imx6sx_idle_finish); + ct_idle_exit(); cpu_cluster_pm_exit(); cpu_pm_exit(); @@ -87,7 +89,8 @@ static struct cpuidle_driver imx6sx_cpui */ .exit_latency = 300, .target_residency = 500, - .flags = CPUIDLE_FLAG_TIMER_STOP, + .flags = CPUIDLE_FLAG_TIMER_STOP | + CPUIDLE_FLAG_RCU_IDLE, .enter = imx6sx_enter_wait, .name = "LOW-POWER-IDLE", .desc = "ARM power off",
Doing RCU-idle outside the driver, only to then temporarily enable it again, at least twice, before going idle is daft. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)