[RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[Ikjoon Jang]

cpuidle is paused only during dpm_suspend_noirq() ~ dpm_resume_noirq().
But some device drivers need random sized IOs in dpm_{suspend|resume}()
stage (e.g. re-downloading firmware in resume).
And with such a device, cpuidle's latencies could be critical to
response time of system suspend/resume.

To minimize those latencies, we could apply pm_qos to such device drivers,
but simply undelaying cpuidle from dpm_suspend_noirq() to dpm suspend()
seems no harm.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---
 drivers/base/power/main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 134a8af51511..5928dd2139e8 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -773,8 +773,6 @@ void dpm_resume_noirq(pm_message_t state)
 
 	resume_device_irqs();
 	device_wakeup_disarm_wake_irqs();
-
-	cpuidle_resume();
 }
 
 static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
@@ -1069,6 +1067,7 @@ void dpm_resume(pm_message_t state)
 
 	cpufreq_resume();
 	devfreq_resume();
+	cpuidle_resume();
 	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
 }
 
@@ -1411,8 +1410,6 @@ int dpm_suspend_noirq(pm_message_t state)
 {
 	int ret;
 
-	cpuidle_pause();
-
 	device_wakeup_arm_wake_irqs();
 	suspend_device_irqs();
 
@@ -1830,6 +1827,7 @@ int dpm_suspend(pm_message_t state)
 	trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
 	might_sleep();
 
+	cpuidle_pause();
 	devfreq_suspend();
 	cpufreq_suspend();
 
-- 
2.24.0.rc0.303.g954a862665-goog

Re: [RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[Rafael J. Wysocki]

On Wednesday, October 30, 2019 3:21:05 AM CET Ikjoon Jang wrote:
> cpuidle is paused only during dpm_suspend_noirq() ~ dpm_resume_noirq().
> But some device drivers need random sized IOs in dpm_{suspend|resume}()
> stage (e.g. re-downloading firmware in resume).
> And with such a device, cpuidle's latencies could be critical to
> response time of system suspend/resume.
> 
> To minimize those latencies, we could apply pm_qos to such device drivers,
> but simply undelaying cpuidle from dpm_suspend_noirq() to dpm suspend()
> seems no harm.

While the patch is generally acceptable, the changelog is not.

First, what does "undelying" mean?

Second, you seem to be talking about the cases in which exit latencies of
idle states are not small relative to the system suspend/resume time, so
without any specific examples this is not really convincing.

Also, potentially, there is another reason to make this change, which is
that on some systems i2c (or similar) controllers may be requisite for
idle state entry and exit, so it may make sense in general to prevent
cpuidle from being used over the entire suspend and resume of the
system.  However, without any example of a system in which that matters
it still is not convincing enough IMO.

> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
>  drivers/base/power/main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 134a8af51511..5928dd2139e8 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -773,8 +773,6 @@ void dpm_resume_noirq(pm_message_t state)
>  
>  	resume_device_irqs();
>  	device_wakeup_disarm_wake_irqs();
> -
> -	cpuidle_resume();
>  }
>  
>  static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
> @@ -1069,6 +1067,7 @@ void dpm_resume(pm_message_t state)
>  
>  	cpufreq_resume();
>  	devfreq_resume();
> +	cpuidle_resume();
>  	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
>  }
>  
> @@ -1411,8 +1410,6 @@ int dpm_suspend_noirq(pm_message_t state)
>  {
>  	int ret;
>  
> -	cpuidle_pause();
> -
>  	device_wakeup_arm_wake_irqs();
>  	suspend_device_irqs();
>  
> @@ -1830,6 +1827,7 @@ int dpm_suspend(pm_message_t state)
>  	trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
>  	might_sleep();
>  
> +	cpuidle_pause();
>  	devfreq_suspend();
>  	cpufreq_suspend();
>  
> 

Re: [RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[Ikjoon Jang]

On Fri, Nov 8, 2019 at 7:22 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, October 30, 2019 3:21:05 AM CET Ikjoon Jang wrote:
> > cpuidle is paused only during dpm_suspend_noirq() ~ dpm_resume_noirq().
> > But some device drivers need random sized IOs in dpm_{suspend|resume}()
> > stage (e.g. re-downloading firmware in resume).
> > And with such a device, cpuidle's latencies could be critical to
> > response time of system suspend/resume.
> >
> > To minimize those latencies, we could apply pm_qos to such device drivers,
> > but simply undelaying cpuidle from dpm_suspend_noirq() to dpm suspend()
> > seems no harm.
>
> While the patch is generally acceptable, the changelog is not.
>
> First, what does "undelying" mean?

You're right, that should be fixed,
actually I used 'undelaying' from commit: 8651f97bd951d
(PM / cpuidle: System resume hang fix with cpuidle),
when the first time cpuidle_{pause|resume} is introduced:

"Since we are dealing with drivers it seems best to call this function
during dpm_suspend(). Delaying the call till dpm_suspend_noirq() does
no harm, as long as it is before cpu_hotplug_begin() to avoid race
conditions with cpu hotpulg operations."

Delaying does no harm, but I think that there had been no specific
reason of this
delay from the beginning. Undelaying does no harm too.

>
> Second, you seem to be talking about the cases in which exit latencies of
> idle states are not small relative to the system suspend/resume time, so
> without any specific examples this is not really convincing.
>
> Also, potentially, there is another reason to make this change, which is
> that on some systems i2c (or similar) controllers may be requisite for
> idle state entry and exit, so it may make sense in general to prevent
> cpuidle from being used over the entire suspend and resume of the
> system.  However, without any example of a system in which that matters
> it still is not convincing enough IMO.
>

Currently I've got only one specific device for examples.
Maybe this patch needs more generalized examples for applying to all
other machines.

Thanks!

> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > ---
> >  drivers/base/power/main.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 134a8af51511..5928dd2139e8 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -773,8 +773,6 @@ void dpm_resume_noirq(pm_message_t state)
> >
> >       resume_device_irqs();
> >       device_wakeup_disarm_wake_irqs();
> > -
> > -     cpuidle_resume();
> >  }
> >
> >  static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
> > @@ -1069,6 +1067,7 @@ void dpm_resume(pm_message_t state)
> >
> >       cpufreq_resume();
> >       devfreq_resume();
> > +     cpuidle_resume();
> >       trace_suspend_resume(TPS("dpm_resume"), state.event, false);
> >  }
> >
> > @@ -1411,8 +1410,6 @@ int dpm_suspend_noirq(pm_message_t state)
> >  {
> >       int ret;
> >
> > -     cpuidle_pause();
> > -
> >       device_wakeup_arm_wake_irqs();
> >       suspend_device_irqs();
> >
> > @@ -1830,6 +1827,7 @@ int dpm_suspend(pm_message_t state)
> >       trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
> >       might_sleep();
> >
> > +     cpuidle_pause();
> >       devfreq_suspend();
> >       cpufreq_suspend();
> >
> >
>
>
>
>

Re: [RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[Rafael J. Wysocki]

On Tue, Nov 12, 2019 at 6:10 AM Ikjoon Jang <ikjn@chromium.org> wrote:
>
> On Fri, Nov 8, 2019 at 7:22 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Wednesday, October 30, 2019 3:21:05 AM CET Ikjoon Jang wrote:
> > > cpuidle is paused only during dpm_suspend_noirq() ~ dpm_resume_noirq().
> > > But some device drivers need random sized IOs in dpm_{suspend|resume}()
> > > stage (e.g. re-downloading firmware in resume).
> > > And with such a device, cpuidle's latencies could be critical to
> > > response time of system suspend/resume.
> > >
> > > To minimize those latencies, we could apply pm_qos to such device drivers,
> > > but simply undelaying cpuidle from dpm_suspend_noirq() to dpm suspend()
> > > seems no harm.
> >
> > While the patch is generally acceptable, the changelog is not.
> >
> > First, what does "undelying" mean?
>
> You're right, that should be fixed,
> actually I used 'undelaying' from commit: 8651f97bd951d
> (PM / cpuidle: System resume hang fix with cpuidle),
> when the first time cpuidle_{pause|resume} is introduced:
>
> "Since we are dealing with drivers it seems best to call this function
> during dpm_suspend(). Delaying the call till dpm_suspend_noirq() does
> no harm, as long as it is before cpu_hotplug_begin() to avoid race
> conditions with cpu hotpulg operations."
>
> Delaying does no harm, but I think that there had been no specific
> reason of this
> delay from the beginning. Undelaying does no harm too.

I see.

It would be good to mention commit 8651f97bd951d in the changelog.
And while "delaying" is a proper word in English, "undelaying" isn't
AFAICS, so maybe say "avoid delaying" or something to that effect
instead.

> >
> > Second, you seem to be talking about the cases in which exit latencies of
> > idle states are not small relative to the system suspend/resume time, so
> > without any specific examples this is not really convincing.
> >
> > Also, potentially, there is another reason to make this change, which is
> > that on some systems i2c (or similar) controllers may be requisite for
> > idle state entry and exit, so it may make sense in general to prevent
> > cpuidle from being used over the entire suspend and resume of the
> > system.  However, without any example of a system in which that matters
> > it still is not convincing enough IMO.
> >
>
> Currently I've got only one specific device for examples.
> Maybe this patch needs more generalized examples for applying to all
> other machines.

One example would be enough, but please provide it in the changelog of
the patch.