Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 27 Mar 2020 10:53:11 +0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     tglx@linutronix.de, peterz@infradead.org,
        linux-kernel@vger.kernel.org, len.brown@intel.com,
        pkondeti@codeaurora.org, jpoimboe@redhat.com, pavel@ucw.cz,
        konrad.wilk@oracle.com, mojha@codeaurora.org, jkosina@suse.cz,
        mingo@kernel.org, hpa@zytor.com, rjw@rjwysocki.net
Cc:     linux-tip-commits@vger.kernel.org
Subject: Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if
 wakeup is pending
Message-ID: <20200327025311.GA58760@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
References: <1559536263-16472-1-git-send-email-pkondeti@codeaurora.org>
 <tip-a66d955e910ab0e598d7a7450cbe6139f52befe7@git.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <tip-a66d955e910ab0e598d7a7450cbe6139f52befe7@git.kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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