[PATCH] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[PATCH] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Pavankumar Kondeti]

When "deep" suspend is enabled, all CPUs except the primary CPU
are hotplugged out. Since CPU hotplug is a costly operation,
check if we have to abort the suspend in between each CPU
hotplug. This would improve the system suspend abort latency
upon detecting a wakeup condition.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 kernel/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index f2ef104..784b33d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1221,6 +1221,13 @@ int freeze_secondary_cpus(int primary)
 	for_each_online_cpu(cpu) {
 		if (cpu == primary)
 			continue;
+
+		if (pm_wakeup_pending()) {
+			pr_info("Aborting disabling non-boot CPUs..\n");
+			error = -EBUSY;
+			break;
+		}
+
 		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
 		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
 		trace_suspend_resume(TPS("CPU_OFF"), cpu, false);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Pavan Kondeti]

Hi Rafael/Thomas,

On Mon, Jun 3, 2019 at 10:03 AM Pavankumar Kondeti
<pkondeti@codeaurora.org> wrote:
>
> When "deep" suspend is enabled, all CPUs except the primary CPU
> are hotplugged out. Since CPU hotplug is a costly operation,
> check if we have to abort the suspend in between each CPU
> hotplug. This would improve the system suspend abort latency
> upon detecting a wakeup condition.
>

Please let me know if you have any comments on this patch.

Thanks,
Pavan

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

[tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[tip-bot for Pavankumar Kondeti]

Commit-ID:  a66d955e910ab0e598d7a7450cbe6139f52befe7
Gitweb:     https://git.kernel.org/tip/a66d955e910ab0e598d7a7450cbe6139f52befe7
Author:     Pavankumar Kondeti <pkondeti@codeaurora.org>
AuthorDate: Mon, 3 Jun 2019 10:01:03 +0530
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 12 Jun 2019 11:03:05 +0200

cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

When "deep" suspend is enabled, all CPUs except the primary CPU are frozen
via CPU hotplug one by one. After all secondary CPUs are unplugged the
wakeup pending condition is evaluated and if pending the suspend operation
is aborted and the secondary CPUs are brought up again.

CPU hotplug is a slow operation, so it makes sense to check for wakeup
pending in the freezer loop before bringing down the next CPU. This
improves the system suspend abort latency significantly.

[ tglx: Massaged changelog and improved printk message ]

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: iri Kosina <jkosina@suse.cz>
Cc: Mukesh Ojha <mojha@codeaurora.org>
Cc: linux-pm@vger.kernel.org
Link: https://lkml.kernel.org/r/1559536263-16472-1-git-send-email-pkondeti@codeaurora.org

---
 kernel/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index be82cbc11a8a..0778249cd49d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1221,6 +1221,13 @@ int freeze_secondary_cpus(int primary)
 	for_each_online_cpu(cpu) {
 		if (cpu == primary)
 			continue;
+
+		if (pm_wakeup_pending()) {
+			pr_info("Wakeup pending. Abort CPU freeze\n");
+			error = -EBUSY;
+			break;
+		}
+
 		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
 		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
 		trace_suspend_resume(TPS("CPU_OFF"), cpu, false);

Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Boqun Feng]

Hi Thomas and Pavankumar,

I have a question about this patch, please see below:

On Wed, Jun 12, 2019 at 05:34:08AM -0700, tip-bot for Pavankumar Kondeti wrote:
> Commit-ID:  a66d955e910ab0e598d7a7450cbe6139f52befe7
> Gitweb:     https://git.kernel.org/tip/a66d955e910ab0e598d7a7450cbe6139f52befe7
> Author:     Pavankumar Kondeti <pkondeti@codeaurora.org>
> AuthorDate: Mon, 3 Jun 2019 10:01:03 +0530
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Wed, 12 Jun 2019 11:03:05 +0200
> 
> cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending
> 
> When "deep" suspend is enabled, all CPUs except the primary CPU are frozen
> via CPU hotplug one by one. After all secondary CPUs are unplugged the
> wakeup pending condition is evaluated and if pending the suspend operation
> is aborted and the secondary CPUs are brought up again.
> 
> CPU hotplug is a slow operation, so it makes sense to check for wakeup
> pending in the freezer loop before bringing down the next CPU. This
> improves the system suspend abort latency significantly.
> 

From the commit message, it makes sense to add the pm_wakeup_pending()
check if freeze_secondary_cpus() is used for system suspend. However,
freeze_secondary_cpus() is also used in kexec path on arm64:

	kernel_kexec():
	  machine_shutdown():
	    disable_nonboot_cpus():
	      freeze_secondary_cpus()

, so I wonder whether the pm_wakeup_pending() makes sense in this
situation? Because IIUC, in this case we want to reboot the system
regardlessly, the pm_wakeup_pending() checking seems to be inappropriate
then.

I'm asking this because I'm debugging a kexec failure on ARM64 guest on
Hyper-V, and I got the BUG_ON() triggered:

[  108.378016] kexec_core: Starting new kernel
[  108.378018] Disabling non-boot CPUs ...
[  108.378019] Wakeup pending. Abort CPU freeze
[  108.378020] Non-boot CPUs are not disabled
[  108.378049] ------------[ cut here ]------------
[  108.378050] kernel BUG at arch/arm64/kernel/machine_kexec.c:154!

Thanks!

Regards,
Boqun

> [ tglx: Massaged changelog and improved printk message ]
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: iri Kosina <jkosina@suse.cz>
> Cc: Mukesh Ojha <mojha@codeaurora.org>
> Cc: linux-pm@vger.kernel.org
> Link: https://lkml.kernel.org/r/1559536263-16472-1-git-send-email-pkondeti@codeaurora.org
> 
> ---
>  kernel/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index be82cbc11a8a..0778249cd49d 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1221,6 +1221,13 @@ int freeze_secondary_cpus(int primary)
>  	for_each_online_cpu(cpu) {
>  		if (cpu == primary)
>  			continue;
> +
> +		if (pm_wakeup_pending()) {
> +			pr_info("Wakeup pending. Abort CPU freeze\n");
> +			error = -EBUSY;
> +			break;
> +		}
> +
>  		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
>  		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
>  		trace_suspend_resume(TPS("CPU_OFF"), cpu, false);

Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Thomas Gleixner]

Boqun Feng <boqun.feng@gmail.com> writes:
> From the commit message, it makes sense to add the pm_wakeup_pending()
> check if freeze_secondary_cpus() is used for system suspend. However,
> freeze_secondary_cpus() is also used in kexec path on arm64:

Bah!

> 	kernel_kexec():
> 	  machine_shutdown():
> 	    disable_nonboot_cpus():
> 	      freeze_secondary_cpus()
>
> , so I wonder whether the pm_wakeup_pending() makes sense in this
> situation? Because IIUC, in this case we want to reboot the system
> regardlessly, the pm_wakeup_pending() checking seems to be inappropriate
> then.

Fix below.

Thanks,

        tglx

8<------------

--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -133,12 +133,18 @@ static inline void get_online_cpus(void)
 static inline void put_online_cpus(void) { cpus_read_unlock(); }
 
 #ifdef CONFIG_PM_SLEEP_SMP
-extern int freeze_secondary_cpus(int primary);
+int __freeze_secondary_cpus(int primary, bool suspend);
+static inline int freeze_secondary_cpus(int primary)
+{
+	return __freeze_secondary_cpus(primary, true);
+}
+
 static inline int disable_nonboot_cpus(void)
 {
-	return freeze_secondary_cpus(0);
+	return __freeze_secondary_cpus(0, false);
 }
-extern void enable_nonboot_cpus(void);
+
+void enable_nonboot_cpus(void);
 
 static inline int suspend_disable_secondary_cpus(void)
 {
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1200,7 +1200,7 @@ EXPORT_SYMBOL_GPL(cpu_up);
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
-int freeze_secondary_cpus(int primary)
+int __freeze_secondary_cpus(int primary, bool suspend)
 {
 	int cpu, error = 0;
 
@@ -1225,7 +1225,7 @@ int freeze_secondary_cpus(int primary)
 		if (cpu == primary)
 			continue;
 
-		if (pm_wakeup_pending()) {
+		if (suspend && pm_wakeup_pending()) {
 			pr_info("Wakeup pending. Abort CPU freeze\n");
 			error = -EBUSY;
 			break;

[tip: smp/core] cpu/hotplug: Ignore pm_wakeup_pending() for disable_nonboot_cpus()

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the smp/core branch of tip:

Commit-ID:     e98eac6ff1b45e4e73f2e6031b37c256ccb5d36b
Gitweb:        https://git.kernel.org/tip/e98eac6ff1b45e4e73f2e6031b37c256ccb5d36b
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Fri, 27 Mar 2020 12:06:44 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 28 Mar 2020 11:42:55 +01:00

cpu/hotplug: Ignore pm_wakeup_pending() for disable_nonboot_cpus()

A recent change to freeze_secondary_cpus() which added an early abort if a
wakeup is pending missed the fact that the function is also invoked for
shutdown, reboot and kexec via disable_nonboot_cpus().

In case of disable_nonboot_cpus() the wakeup event needs to be ignored as
the purpose is to terminate the currently running kernel.

Add a 'suspend' argument which is only set when the freeze is in context of
a suspend operation. If not set then an eventually pending wakeup event is
ignored.

Fixes: a66d955e910a ("cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending")
Reported-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Pavankumar Kondeti <pkondeti@codeaurora.org>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/874kuaxdiz.fsf@nanos.tec.linutronix.de


---
 include/linux/cpu.h | 12 +++++++++---
 kernel/cpu.c        |  4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 9ead281..beaed2d 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -144,12 +144,18 @@ static inline void get_online_cpus(void) { cpus_read_lock(); }
 static inline void put_online_cpus(void) { cpus_read_unlock(); }
 
 #ifdef CONFIG_PM_SLEEP_SMP
-extern int freeze_secondary_cpus(int primary);
+int __freeze_secondary_cpus(int primary, bool suspend);
+static inline int freeze_secondary_cpus(int primary)
+{
+	return __freeze_secondary_cpus(primary, true);
+}
+
 static inline int disable_nonboot_cpus(void)
 {
-	return freeze_secondary_cpus(0);
+	return __freeze_secondary_cpus(0, false);
 }
-extern void enable_nonboot_cpus(void);
+
+void enable_nonboot_cpus(void);
 
 static inline int suspend_disable_secondary_cpus(void)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 3084849..12ae636 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1327,7 +1327,7 @@ void bringup_nonboot_cpus(unsigned int setup_max_cpus)
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
-int freeze_secondary_cpus(int primary)
+int __freeze_secondary_cpus(int primary, bool suspend)
 {
 	int cpu, error = 0;
 
@@ -1352,7 +1352,7 @@ int freeze_secondary_cpus(int primary)
 		if (cpu == primary)
 			continue;
 
-		if (pm_wakeup_pending()) {
+		if (suspend && pm_wakeup_pending()) {
 			pr_info("Wakeup pending. Abort CPU freeze\n");
 			error = -EBUSY;
 			break;

Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Qais Yousef]

On 03/27/20 10:53, Boqun Feng wrote:
> Hi Thomas and Pavankumar,
> 
> I have a question about this patch, please see below:
> 
> On Wed, Jun 12, 2019 at 05:34:08AM -0700, tip-bot for Pavankumar Kondeti wrote:
> > Commit-ID:  a66d955e910ab0e598d7a7450cbe6139f52befe7
> > Gitweb:     https://git.kernel.org/tip/a66d955e910ab0e598d7a7450cbe6139f52befe7
> > Author:     Pavankumar Kondeti <pkondeti@codeaurora.org>
> > AuthorDate: Mon, 3 Jun 2019 10:01:03 +0530
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Wed, 12 Jun 2019 11:03:05 +0200
> > 
> > cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending
> > 
> > When "deep" suspend is enabled, all CPUs except the primary CPU are frozen
> > via CPU hotplug one by one. After all secondary CPUs are unplugged the
> > wakeup pending condition is evaluated and if pending the suspend operation
> > is aborted and the secondary CPUs are brought up again.
> > 
> > CPU hotplug is a slow operation, so it makes sense to check for wakeup
> > pending in the freezer loop before bringing down the next CPU. This
> > improves the system suspend abort latency significantly.
> > 
> 
> From the commit message, it makes sense to add the pm_wakeup_pending()
> check if freeze_secondary_cpus() is used for system suspend. However,
> freeze_secondary_cpus() is also used in kexec path on arm64:
> 
> 	kernel_kexec():
> 	  machine_shutdown():
> 	    disable_nonboot_cpus():
> 	      freeze_secondary_cpus()

FWIW, I fixed this already and the change was picked up:

	https://lore.kernel.org/lkml/20200323135110.30522-7-qais.yousef@arm.com/

Only x86 now uses disable_nonboot_cpus().

# tip/smp/core

$ git grep disable_nonboot_cpus
Documentation/power/suspend-and-cpuhotplug.rst:                              disable_nonboot_cpus()
Documentation/power/suspend-and-cpuhotplug.rst:                       /* disable_nonboot_cpus() complete */
arch/x86/power/cpu.c:   ret = disable_nonboot_cpus();
include/linux/cpu.h:static inline int disable_nonboot_cpus(void)
include/linux/cpu.h:static inline int disable_nonboot_cpus(void) { return 0; }
kernel/cpu.c:    * this even in case of failure as all disable_nonboot_cpus() users are

Thanks

--
Qais Yousef

> 
> , so I wonder whether the pm_wakeup_pending() makes sense in this
> situation? Because IIUC, in this case we want to reboot the system
> regardlessly, the pm_wakeup_pending() checking seems to be inappropriate
> then.
> 
> I'm asking this because I'm debugging a kexec failure on ARM64 guest on
> Hyper-V, and I got the BUG_ON() triggered:
> 
> [  108.378016] kexec_core: Starting new kernel
> [  108.378018] Disabling non-boot CPUs ...
> [  108.378019] Wakeup pending. Abort CPU freeze
> [  108.378020] Non-boot CPUs are not disabled
> [  108.378049] ------------[ cut here ]------------
> [  108.378050] kernel BUG at arch/arm64/kernel/machine_kexec.c:154!
> 
> Thanks!
> 
> Regards,
> Boqun
> 
> > [ tglx: Massaged changelog and improved printk message ]
> > 
> > Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: iri Kosina <jkosina@suse.cz>
> > Cc: Mukesh Ojha <mojha@codeaurora.org>
> > Cc: linux-pm@vger.kernel.org
> > Link: https://lkml.kernel.org/r/1559536263-16472-1-git-send-email-pkondeti@codeaurora.org
> > 
> > ---
> >  kernel/cpu.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index be82cbc11a8a..0778249cd49d 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1221,6 +1221,13 @@ int freeze_secondary_cpus(int primary)
> >  	for_each_online_cpu(cpu) {
> >  		if (cpu == primary)
> >  			continue;
> > +
> > +		if (pm_wakeup_pending()) {
> > +			pr_info("Wakeup pending. Abort CPU freeze\n");
> > +			error = -EBUSY;
> > +			break;
> > +		}
> > +
> >  		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
> >  		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
> >  		trace_suspend_resume(TPS("CPU_OFF"), cpu, false);

Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if wakeup is pending

[Qais Yousef]

On 03/27/20 12:06, Thomas Gleixner wrote:
> Boqun Feng <boqun.feng@gmail.com> writes:
> > From the commit message, it makes sense to add the pm_wakeup_pending()
> > check if freeze_secondary_cpus() is used for system suspend. However,
> > freeze_secondary_cpus() is also used in kexec path on arm64:
> 
> Bah!
> 
> > 	kernel_kexec():
> > 	  machine_shutdown():
> > 	    disable_nonboot_cpus():
> > 	      freeze_secondary_cpus()
> >
> > , so I wonder whether the pm_wakeup_pending() makes sense in this
> > situation? Because IIUC, in this case we want to reboot the system
> > regardlessly, the pm_wakeup_pending() checking seems to be inappropriate
> > then.
> 
> Fix below.
> 
> Thanks,
> 
>         tglx
> 
> 8<------------
> 
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -133,12 +133,18 @@ static inline void get_online_cpus(void)
>  static inline void put_online_cpus(void) { cpus_read_unlock(); }
>  
>  #ifdef CONFIG_PM_SLEEP_SMP
> -extern int freeze_secondary_cpus(int primary);
> +int __freeze_secondary_cpus(int primary, bool suspend);
> +static inline int freeze_secondary_cpus(int primary)
> +{
> +	return __freeze_secondary_cpus(primary, true);
> +}
> +
>  static inline int disable_nonboot_cpus(void)
>  {
> -	return freeze_secondary_cpus(0);
> +	return __freeze_secondary_cpus(0, false);
>  }

If I read the code correctly, arch/x86/power/cpu.c is calling
disable_nonboot_cpus() from suspend resume, which is the only user in
tip/smp/core after my series.

This means you won't abort a suspend/hibernate if a late wakeup source happens?
Or it might just mean that we'll wakeup slightly later than we would have done.

Anyways. I think it would be better to kill off disable_nonboot_cpus() and
directly call freeze_nonboot_cpus() in x86/power/cpu.c.

I'd be happy to send a patch for this.

Assuming that x86 is okay with the late(r) abort, this patch could stay as-is
for stable trees. Otherwise, maybe we need to revert this and look for another
option for stable trees?

Thanks

--
Qais Yousef

> -extern void enable_nonboot_cpus(void);
> +
> +void enable_nonboot_cpus(void);
>  
>  static inline int suspend_disable_secondary_cpus(void)
>  {
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1200,7 +1200,7 @@ EXPORT_SYMBOL_GPL(cpu_up);
>  #ifdef CONFIG_PM_SLEEP_SMP
>  static cpumask_var_t frozen_cpus;
>  
> -int freeze_secondary_cpus(int primary)
> +int __freeze_secondary_cpus(int primary, bool suspend)
>  {
>  	int cpu, error = 0;
>  
> @@ -1225,7 +1225,7 @@ int freeze_secondary_cpus(int primary)
>  		if (cpu == primary)
>  			continue;
>  
> -		if (pm_wakeup_pending()) {
> +		if (suspend && pm_wakeup_pending()) {
>  			pr_info("Wakeup pending. Abort CPU freeze\n");
>  			error = -EBUSY;
>  			break;