[PATCH 0/2][v3] Fix IPI missing issue when woken from suspend to idle

[PATCH 0/2][v3] Fix IPI missing issue when woken from suspend to idle

[Chen Yu]

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

[PATCH 1/2][v3] PM / s2idle: Clear _TIF_POLLING_NRFLAG before suspend to idle

[Chen Yu]

Suspend to idle was found to not work on Goldmont CPU recently.
And the issue was triggered due to:

1. On Goldmont the CPU in idle can only be woken up via IPIs,
   not POLLING mode:
   Commit 08e237fa56a1 ("x86/cpu: Add workaround for MONITOR
   instruction erratum on Goldmont based CPUs")
2. When the CPU is entering suspend to idle process, the
   _TIF_POLLING_NRFLAG is kept on, due to cpuidle_enter_s2idle()
   doesn't properly match call_cpuidle().
3. Commit b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
   makes use of _TIF_POLLING_NRFLAG to avoid sending IPIs to
   idle CPUs.
4. As a result, some IPIs related functions might not work
   well during suspend to idle on Goldmont. For example, one
   suspected victim:
   tick_unfreeze() -> timekeeping_resume() -> hrtimers_resume()
   -> clock_was_set() -> on_each_cpu() might wait forever,
   because the IPIs will not be sent to the CPUs which are
   sleeping with _TIF_POLLING_NRFLAG set, and Goldmont CPU
   could not be woken up by only setting _TIF_NEED_RESCHED
   on the monitor address.

Clear the _TIF_POLLING_NRFLAG flag before entering suspend to idle,
and let the driver's enter_s2idle() to decide whether to set
_TIF_POLLING_NRFLAG or not. So that to avoid the scenario described
above and keep the context consistent with before.

Fixes: b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
Reported-by: kbuild test robot <lkp@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v2: According to Peter's review, v1 is racy, if someone already
    set TIF_NEED_RESCHED this patch just clear POLLING and go to sleep.
    Check TIF_NEED_RESCHED before entering suspend to idle and
    adjust the naming to be consistent with call_cpuidle().

v3: According to Rafael, it would be better to do the simplest fix
   first and then do the cleanup on top of it.
---
 drivers/cpuidle/cpuidle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c149d9e20dfd..e092789187c6 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -13,6 +13,7 @@
 #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>
@@ -186,7 +187,7 @@ 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)
+	if (index > 0 && !current_clr_polling_and_test())
 		enter_s2idle_proper(drv, dev, index);
 
 	return index;
-- 
2.17.1

Re: [PATCH 1/2][v3] PM / s2idle: Clear _TIF_POLLING_NRFLAG before suspend to idle

[Rafael J. Wysocki]

On Tue, Jun 23, 2020 at 8:30 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Suspend to idle was found to not work on Goldmont CPU recently.
> And the issue was triggered due to:
>
> 1. On Goldmont the CPU in idle can only be woken up via IPIs,
>    not POLLING mode:
>    Commit 08e237fa56a1 ("x86/cpu: Add workaround for MONITOR
>    instruction erratum on Goldmont based CPUs")
> 2. When the CPU is entering suspend to idle process, the
>    _TIF_POLLING_NRFLAG is kept on, due to cpuidle_enter_s2idle()
>    doesn't properly match call_cpuidle().
> 3. Commit b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
>    makes use of _TIF_POLLING_NRFLAG to avoid sending IPIs to
>    idle CPUs.
> 4. As a result, some IPIs related functions might not work
>    well during suspend to idle on Goldmont. For example, one
>    suspected victim:
>    tick_unfreeze() -> timekeeping_resume() -> hrtimers_resume()
>    -> clock_was_set() -> on_each_cpu() might wait forever,
>    because the IPIs will not be sent to the CPUs which are
>    sleeping with _TIF_POLLING_NRFLAG set, and Goldmont CPU
>    could not be woken up by only setting _TIF_NEED_RESCHED
>    on the monitor address.
>
> Clear the _TIF_POLLING_NRFLAG flag before entering suspend to idle,
> and let the driver's enter_s2idle() to decide whether to set
> _TIF_POLLING_NRFLAG or not. So that to avoid the scenario described
> above and keep the context consistent with before.
>
> Fixes: b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
> Reported-by: kbuild test robot <lkp@intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>

Applied (based on the previous discussion) with some subject and
changelog edits.

Thanks!

> ---
> v2: According to Peter's review, v1 is racy, if someone already
>     set TIF_NEED_RESCHED this patch just clear POLLING and go to sleep.
>     Check TIF_NEED_RESCHED before entering suspend to idle and
>     adjust the naming to be consistent with call_cpuidle().
>
> v3: According to Rafael, it would be better to do the simplest fix
>    first and then do the cleanup on top of it.
> ---
>  drivers/cpuidle/cpuidle.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index c149d9e20dfd..e092789187c6 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -13,6 +13,7 @@
>  #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>
> @@ -186,7 +187,7 @@ 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)
> +       if (index > 0 && !current_clr_polling_and_test())
>                 enter_s2idle_proper(drv, dev, index);
>
>         return index;
> --
> 2.17.1
>

[PATCH 2/2][v3] PM / s2idle: Code cleanup to make s2idle consistent with normal idle path

[Chen Yu]

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) <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 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

Re: [PATCH 2/2][v3] PM / s2idle: Code cleanup to make s2idle consistent with normal idle path

[Rafael J. Wysocki]

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) <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  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 <rafael.j.wysocki@intel.com>
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 <rafael.j.wysocki@intel.com>
---
 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 */

Re: [PATCH 2/2][v3] PM / s2idle: Code cleanup to make s2idle consistent with normal idle path

[Chen Yu]

Hi Rafael,
On Tue, Jun 23, 2020 at 07:57:59PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 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 <rafael.j.wysocki@intel.com>
> ---
>  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 <yu.c.chen@intel.com>

Thanks,
Chenyu

Re: [PATCH 2/2][v3] PM / s2idle: Code cleanup to make s2idle consistent with normal idle path

[Rafael J. Wysocki]

On Thu, Jun 25, 2020 at 7:14 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Hi Rafael,
> On Tue, Jun 23, 2020 at 07:57:59PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 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 <rafael.j.wysocki@intel.com>
> > ---
> >  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 <yu.c.chen@intel.com>

OK, thanks!

I'll queue up this one too along with the [1/2] for -rc4 then.

Thanks!