After an recent optimization on IPIs among idle Cores, the Goldmont failed to
resume from suspend to idle due to missing IPIs. This is because
Goldmont could only be woken up from idle via IPIs rather than POLLING.
Clear the POLLING flag before suspend to idle so that Goldmont will always
get IPIs during suspend to idle.
Chen Yu (2):
PM / s2idle: Clear _TIF_POLLING_NRFLAG before suspend to idle
PM / s2idle: Code cleanup to make s2idle consistent with normal idle
path
drivers/cpuidle/cpuidle.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
--
2.17.1
Currently s2idle is a little different from the normal idle path. This
patch makes call_s2idle() consistent with call_cpuidle(), and also
s2idle_enter() is analogous to cpuidle_enter().
No functional change.
Suggested-by: Peter Zijlstra (Intel) <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Chen Yu <[email protected]>
---
drivers/cpuidle/cpuidle.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index e092789187c6..b2e764d1ac99 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -134,8 +134,8 @@ int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
}
#ifdef CONFIG_SUSPEND
-static void enter_s2idle_proper(struct cpuidle_driver *drv,
- struct cpuidle_device *dev, int index)
+static void s2idle_enter(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev, int index)
{
ktime_t time_start, time_end;
@@ -169,6 +169,15 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
dev->states_usage[index].s2idle_usage++;
}
+static int call_s2idle(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev, int index)
+{
+ if (!current_clr_polling_and_test())
+ s2idle_enter(drv, dev, index);
+
+ return index;
+}
+
/**
* cpuidle_enter_s2idle - Enter an idle state suitable for suspend-to-idle.
* @drv: cpuidle driver for the given CPU.
@@ -187,8 +196,8 @@ int cpuidle_enter_s2idle(struct cpuidle_driver *drv, struct cpuidle_device *dev)
* be frozen safely.
*/
index = find_deepest_state(drv, dev, U64_MAX, 0, true);
- if (index > 0 && !current_clr_polling_and_test())
- enter_s2idle_proper(drv, dev, index);
+ if (index > 0)
+ call_s2idle(drv, dev, index);
return index;
}
--
2.17.1
On Tuesday, June 23, 2020 8:31:52 AM CEST Chen Yu wrote:
> Currently s2idle is a little different from the normal idle path. This
> patch makes call_s2idle() consistent with call_cpuidle(), and also
> s2idle_enter() is analogous to cpuidle_enter().
>
> No functional change.
>
> Suggested-by: Peter Zijlstra (Intel) <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Chen Yu <[email protected]>
> ---
> drivers/cpuidle/cpuidle.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index e092789187c6..b2e764d1ac99 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -134,8 +134,8 @@ int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
> }
>
> #ifdef CONFIG_SUSPEND
> -static void enter_s2idle_proper(struct cpuidle_driver *drv,
> - struct cpuidle_device *dev, int index)
> +static void s2idle_enter(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev, int index)
> {
> ktime_t time_start, time_end;
>
> @@ -169,6 +169,15 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
> dev->states_usage[index].s2idle_usage++;
> }
>
> +static int call_s2idle(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev, int index)
> +{
> + if (!current_clr_polling_and_test())
> + s2idle_enter(drv, dev, index);
> +
Well, I'd rather move the definition of this function to idle.c, because it is
better to call current_clr_polling_and_test() from there.
> + return index;
> +}
> +
> /**
> * cpuidle_enter_s2idle - Enter an idle state suitable for suspend-to-idle.
> * @drv: cpuidle driver for the given CPU.
> @@ -187,8 +196,8 @@ int cpuidle_enter_s2idle(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> * be frozen safely.
> */
> index = find_deepest_state(drv, dev, U64_MAX, 0, true);
> - if (index > 0 && !current_clr_polling_and_test())
> - enter_s2idle_proper(drv, dev, index);
> + if (index > 0)
> + call_s2idle(drv, dev, index);
This can be made look more like cpuidle_enter() too.
>
> return index;
> }
>
So overall I'd prefer to apply something like this (on top of the [1/2]):
---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpuidle: Rearrange s2idle-specific idle state entry code
Implement call_cpuidle_s2idle() in analogy with call_cpuidle()
for the s2idle-specific idle state entry and invoke it from
cpuidle_idle_call() to make the s2idle-specific idle entry code
path look more similar to the "regular" idle entry one.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/cpuidle/cpuidle.c | 6 +++---
kernel/sched/idle.c | 15 +++++++++++----
2 files changed, 14 insertions(+), 7 deletions(-)
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -96,6 +96,15 @@ void __cpuidle default_idle_call(void)
}
}
+static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+ if (current_clr_polling_and_test())
+ return -EBUSY;
+
+ return cpuidle_enter_s2idle(drv, dev);
+}
+
static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
int next_state)
{
@@ -171,11 +180,9 @@ static void cpuidle_idle_call(void)
if (idle_should_enter_s2idle()) {
rcu_idle_enter();
- entered_state = cpuidle_enter_s2idle(drv, dev);
- if (entered_state > 0) {
- local_irq_enable();
+ entered_state = call_cpuidle_s2idle(drv, dev);
+ if (entered_state > 0)
goto exit_idle;
- }
rcu_idle_exit();
Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -13,7 +13,6 @@
#include <linux/mutex.h>
#include <linux/sched.h>
#include <linux/sched/clock.h>
-#include <linux/sched/idle.h>
#include <linux/notifier.h>
#include <linux/pm_qos.h>
#include <linux/cpu.h>
@@ -187,9 +186,10 @@ int cpuidle_enter_s2idle(struct cpuidle_
* be frozen safely.
*/
index = find_deepest_state(drv, dev, U64_MAX, 0, true);
- if (index > 0 && !current_clr_polling_and_test())
+ if (index > 0) {
enter_s2idle_proper(drv, dev, index);
-
+ local_irq_enable();
+ }
return index;
}
#endif /* CONFIG_SUSPEND */
Hi Rafael,
On Tue, Jun 23, 2020 at 07:57:59PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> Subject: [PATCH] cpuidle: Rearrange s2idle-specific idle state entry code
>
> Implement call_cpuidle_s2idle() in analogy with call_cpuidle()
> for the s2idle-specific idle state entry and invoke it from
> cpuidle_idle_call() to make the s2idle-specific idle entry code
> path look more similar to the "regular" idle entry one.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/cpuidle/cpuidle.c | 6 +++---
> kernel/sched/idle.c | 15 +++++++++++----
> 2 files changed, 14 insertions(+), 7 deletions(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -96,6 +96,15 @@ void __cpuidle default_idle_call(void)
> }
> }
>
> +static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
> +{
> + if (current_clr_polling_and_test())
> + return -EBUSY;
> +
> + return cpuidle_enter_s2idle(drv, dev);
> +}
> +
> static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> int next_state)
> {
> @@ -171,11 +180,9 @@ static void cpuidle_idle_call(void)
> if (idle_should_enter_s2idle()) {
> rcu_idle_enter();
>
> - entered_state = cpuidle_enter_s2idle(drv, dev);
> - if (entered_state > 0) {
> - local_irq_enable();
> + entered_state = call_cpuidle_s2idle(drv, dev);
I guess this changes the context a little bit that(comparing to [1/2 patch],
after this modification, when we found that TIF_NEED_RESCHED is set we can have
a second chance in the following call_cpuidle to do a second s2idle try. However
in [1/2 patch], it might exit the s2idle phase directly once when we see
TIF_NEED_RESCHED is set(because entered_state is postive we treat it as a successful
s2idle). In summary I think the change (patch [2/2]) is more robust.
Acked-by: Chen Yu <[email protected]>
Thanks,
Chenyu
On Thu, Jun 25, 2020 at 7:14 AM Chen Yu <[email protected]> wrote:
>
> Hi Rafael,
> On Tue, Jun 23, 2020 at 07:57:59PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> > Subject: [PATCH] cpuidle: Rearrange s2idle-specific idle state entry code
> >
> > Implement call_cpuidle_s2idle() in analogy with call_cpuidle()
> > for the s2idle-specific idle state entry and invoke it from
> > cpuidle_idle_call() to make the s2idle-specific idle entry code
> > path look more similar to the "regular" idle entry one.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/cpuidle/cpuidle.c | 6 +++---
> > kernel/sched/idle.c | 15 +++++++++++----
> > 2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > Index: linux-pm/kernel/sched/idle.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/idle.c
> > +++ linux-pm/kernel/sched/idle.c
> > @@ -96,6 +96,15 @@ void __cpuidle default_idle_call(void)
> > }
> > }
> >
> > +static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
> > + struct cpuidle_device *dev)
> > +{
> > + if (current_clr_polling_and_test())
> > + return -EBUSY;
> > +
> > + return cpuidle_enter_s2idle(drv, dev);
> > +}
> > +
> > static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > int next_state)
> > {
> > @@ -171,11 +180,9 @@ static void cpuidle_idle_call(void)
> > if (idle_should_enter_s2idle()) {
> > rcu_idle_enter();
> >
> > - entered_state = cpuidle_enter_s2idle(drv, dev);
> > - if (entered_state > 0) {
> > - local_irq_enable();
> > + entered_state = call_cpuidle_s2idle(drv, dev);
> I guess this changes the context a little bit that(comparing to [1/2 patch],
> after this modification, when we found that TIF_NEED_RESCHED is set we can have
> a second chance in the following call_cpuidle to do a second s2idle try. However
> in [1/2 patch], it might exit the s2idle phase directly once when we see
> TIF_NEED_RESCHED is set(because entered_state is postive we treat it as a successful
> s2idle). In summary I think the change (patch [2/2]) is more robust.
Yeah, good point.
> Acked-by: Chen Yu <[email protected]>
OK, thanks!
I'll queue up this one too along with the [1/2] for -rc4 then.
Thanks!