LinuxLists.cc - [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

2022-09-19 10:53:52

Subject: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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) <[email protected]>
---
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",

2022-09-19 14:50:14

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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) <[email protected]>
> ---
> 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",
>
>

2022-09-19 14:55:06

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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) <[email protected]>
> ---
> 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",
>
>

2022-09-19 15:04:53

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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) <[email protected]>
> > ---
> > 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!

2022-09-19 15:05:05

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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().

2022-09-19 15:20:57

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

2022-09-19 16:01:14

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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) <[email protected]>
> > > ---
> > > 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 <[email protected]>

Thanks!

2022-09-20 09:05:26

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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).

2022-09-20 09:31:19

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: [PATCH v2 08/44] cpuidle,imx6: Push RCU-idle into driver

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!