Xen devel - Jun 2013 - [PATCH 1/4] xen: disable non-boot VCPUs during suspend

From: David Vrabel <david.vrabel@citrix.com>

syscore_suspend() and syscore_resume() expect there to be only one
online CPU.  e.g., hrtimers_resume() only triggers events for the
current CPU.  Xen''s suspend path was leaving all VCPUs online and then
attempting to fixup problems afterwards (e.g., with an explicit call
to clock_was_set() to trigger pending high resolution timers).

Instead, disable non-boot CPUs before calling stop_machine() and
reenable them afterwards.

This is then similar to what the kexec code does before and after a
kexec jump (see kernel_kexec() in kernel/kexec.c).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/manage.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 412b96c..596e55a 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -148,8 +148,19 @@ static void do_suspend(void)
 		si.post = &xen_post_suspend;
 	}
 
+	/*
+	 * syscore_suspend() and syscore_resume() called in
+	 * xen_suspend() above, assume that only the boot CPU is
+	 * online.
+	 */
+	err = disable_nonboot_cpus();
+	if (err)
+		goto out_resume;
+
 	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
+	enable_nonboot_cpus();
+
 	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	if (err) {
@@ -166,9 +177,6 @@ out_resume:
 
 	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
-	/* Make sure timer events get retriggered on all CPUs */
-	clock_was_set();
-
 out_thaw:
 #ifdef CONFIG_PREEMPT
 	thaw_processes();
-- 
1.7.2.5

On Wed, Jun 19, 2013 at 04:25:20PM +0100, David Vrabel
wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> syscore_suspend() and syscore_resume() expect there to be only one
> online CPU.  e.g., hrtimers_resume() only triggers events for the
> current CPU.  Xen''s suspend path was leaving all VCPUs online and
then
> attempting to fixup problems afterwards (e.g., with an explicit call
> to clock_was_set() to trigger pending high resolution timers).
> 
> Instead, disable non-boot CPUs before calling stop_machine() and
> reenable them afterwards.
> 
> This is then similar to what the kexec code does before and after a
> kexec jump (see kernel_kexec() in kernel/kexec.c).
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Looks like a bug-fix. But considering that the VCPU hotplug code
in PVHVM had bugs in until v3.10 it probably shouldn''t even hit
stable tree.

Looks ok to me so will stick it on the 3.11 train if nobody screams.
> ---
>  drivers/xen/manage.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 412b96c..596e55a 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -148,8 +148,19 @@ static void do_suspend(void)
>  		si.post = &xen_post_suspend;
>  	}
>  
> +	/*
> +	 * syscore_suspend() and syscore_resume() called in
> +	 * xen_suspend() above, assume that only the boot CPU is
> +	 * online.
> +	 */
> +	err = disable_nonboot_cpus();
> +	if (err)
> +		goto out_resume;
> +
>  	err = stop_machine(xen_suspend, &si, cpumask_of(0));
>  
> +	enable_nonboot_cpus();
> +
>  	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>  
>  	if (err) {
> @@ -166,9 +177,6 @@ out_resume:
>  
>  	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>  
> -	/* Make sure timer events get retriggered on all CPUs */
> -	clock_was_set();
> -
>  out_thaw:
>  #ifdef CONFIG_PREEMPT
>  	thaw_processes();
> -- 
> 1.7.2.5
>

On 19/06/13 18:11, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 19, 2013 at 04:25:20PM +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> syscore_suspend() and syscore_resume() expect there to be only one
>> online CPU.  e.g., hrtimers_resume() only triggers events for the
>> current CPU.  Xen''s suspend path was leaving all VCPUs online
and then
>> attempting to fixup problems afterwards (e.g., with an explicit call
>> to clock_was_set() to trigger pending high resolution timers).
>>
>> Instead, disable non-boot CPUs before calling stop_machine() and
>> reenable them afterwards.
>>
>> This is then similar to what the kexec code does before and after a
>> kexec jump (see kernel_kexec() in kernel/kexec.c).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Looks like a bug-fix. But considering that the VCPU hotplug code
> in PVHVM had bugs in until v3.10 it probably shouldn''t even hit
> stable tree.
I don''t think actually fixes any bugs so it doesn''t need to go
to stable.

David

>>> On 19.06.13 at 17:25, David Vrabel <david.vrabel@citrix.com>
wrote:
> syscore_suspend() and syscore_resume() expect there to be only one
> online CPU.  e.g., hrtimers_resume() only triggers events for the
> current CPU.  Xen''s suspend path was leaving all VCPUs online and
then
> attempting to fixup problems afterwards (e.g., with an explicit call
> to clock_was_set() to trigger pending high resolution timers).
> 
> Instead, disable non-boot CPUs before calling stop_machine() and
> reenable them afterwards.
In XenoLinux the so called "fast suspend" mode was specifically
added for performance reasons, and it looks like to date pv-ops
only ever supported that mode. So one question is whether
there''s going to be any bad performance effect from this.

The other question is that about retaining the use of
stop_machine() then - it seems pretty pointless if you already
bring down all other CPUs.

Jan

On 20/06/13 11:38, Jan Beulich wrote:
>>>> On 19.06.13 at 17:25, David Vrabel
<david.vrabel@citrix.com> wrote:
>> syscore_suspend() and syscore_resume() expect there to be only one
>> online CPU.  e.g., hrtimers_resume() only triggers events for the
>> current CPU.  Xen''s suspend path was leaving all VCPUs online
and then
>> attempting to fixup problems afterwards (e.g., with an explicit call
>> to clock_was_set() to trigger pending high resolution timers).
>>
>> Instead, disable non-boot CPUs before calling stop_machine() and
>> reenable them afterwards.
> 
> In XenoLinux the so called "fast suspend" mode was specifically
> added for performance reasons, and it looks like to date pv-ops
> only ever supported that mode. So one question is whether
> there''s going to be any bad performance effect from this.
Yes :(

On a VM with 4 VCPUs, disable_boot_cpus() took > 200 ms.

I''ll have to rethink this.

David