xen: cpupools: avoid crashing when shutting down/suspending with free CPUs
cover xen: cpupools: avoid crashing when shutting down/suspending with free CPUs
In fact, in the following situation:
root@Zhaman:~# xl cpupool-cpu-remove Pool-0 node:1,^9-11 root@Zhaman:~# xl cpupool-list -c Name CPU list Pool-0 0,1,2,3,4,5,6,7,9,10,11
Trying to power off the system would results in this splat:
(XEN) Preparing system for ACPI S5 state.
(XEN) Disabling non-boot CPUs ...
(XEN) Error taking CPU8 down: -16
(XEN) Xen BUG at cpu.c:191
(XEN) ----[ Xen-4.6-unstable x86_64 debug=y Not tainted ]----
(XEN) CPU: 0
(XEN) RIP: e008:[<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138
(XEN) RFLAGS: 0000000000010246 CONTEXT: hypervisor
(XEN) rax: 0000000000000000 rbx: 0000000000000008 rcx: 0000000000000000 (XEN) rdx: ffff82d0802e8000 rsi: 000000000000000a rdi: ffff82d08029b69c (XEN) rbp: ffff82d0802efe30 rsp: ffff82d0802efe10 r8: ffff830320700000 (XEN) r9: 0000000000000002 r10: 0000000000000010 r11: 0000000000000002 (XEN) r12: 0000000000000008 r13: ffff82d080288b80 r14: 00000000fffffff0 (XEN) r15: 0000000000000003 cr0: 000000008005003b cr4: 00000000000026f0 (XEN) cr3: 00000000dba99000 cr2: ffff88000e8f2cb0 (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: e010 cs: e008 (XEN) Xen stack trace from rsp=ffff82d0802efe10: (XEN) 0000000000000000 0000000000000005 ffff82d080334ae0 ffff82d080334a08 (XEN) ffff82d0802efe80 ffff82d0801a8824 ffff82d0802efe60 ffff82d080167e6e (XEN) ffff82d080334a08 ffff8303191c0680 ffff8300dbb3b000 ffff82d080334ae0 (XEN) ffff82d080334a08 0000000000000003 ffff82d0802efea0 ffff82d08010614a (XEN) ffff8300dbb3b1c8 0000000000000000 ffff82d0802efec0 ffff82d0801320bd (XEN) ffff82d08012fef1 ffff82d080334af0 ffff82d0802efef0 ffff82d0801323f3 (XEN) ffff8300dbb3b000 ffff82d0802e8000 ffff8300dbb3b000 00000000ffffff01 (XEN) ffff82d0802eff10 ffff82d080163cb6 ffff82d08012f67d ffff8300dbdf4000 (XEN) ffff82d0802efde8 00000000fee1dead 0000000000000000 0000000000002803 (XEN) 0000000000000005 ffff880012a53d48 00000000ffffffff 0000000000000246 (XEN) 0000000000000010 0000000000002803 0000000000002803 0000000000000000 (XEN) ffffffff810010ea 00000000ffff0000 0000000000002803 00000000deadbeef (XEN) 0000010000000000 ffffffff810010ea 000000000000e033 0000000000000246 (XEN) ffff880012a53c90 000000000000e02b 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 ffff8300dbdf4000 (XEN) 0000000000000000 0000000000000000 (XEN) Xen call trace: (XEN) [<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138 (XEN) [<ffff82d0801a8824>] enter_state_helper+0xbd/0x369 (XEN) [<ffff82d08010614a>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d0801320bd>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d0801323f3>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080163cb6>] idle_loop+0x56/0x6b
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:191
(XEN) ****************************************
(NB. Output is with patch #1 applied, so we can see that the problem arises when tearing down CPU #8.)
That is because, for CPUs that are not assigned to any cpupool, -EBUSY is returned, in cpupool_cpu_remove(), and that makes cpu_down() (invoked by disable_nonboot_cpus() ) really, really angry.
The main focus of this series is hence to fix this, by just avoiding returning -EBUSY for free CPUs, during system shutdown or suspend, while still not allowing to hot-unplug a CPU that does not belong to cpupool0 (i.e., either free or assigned to another pool), as it was by design. When we are shutting down, we really don't care much. For the suspend/resume case, I think I managed to put things in such a way that free CPUs at suspend time stay free after resuming.
This happens in patch #3, together with a few other improvements to the involved code, in the form of better (IMO, of course :-D) comments, and a couple of ASSERT()-s. Patch 1 and 2 are mostly about improving debugging (patch #1) and code cleanup (patch #2).
The only problem I have is that I don't have a box that suspends and then resumes nicely, so I couldn't really test that path (I think I got it right, though).
This is available as a git branch here:
git://xenbits.xen.org/people/dariof/xen.git rel/cpupools/shutdown-fix-v1
http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/cpupools/shutdown-fix-v1
Thanks and Regards,
Dario
---
Dario Faggioli (3):
xen: always print offending CPU on bringup/teardown failure
xen: cpupool: assigning a CPU to a pool can fail
xen: cpupools: avoid crashing if shutting down with free CPUs
xen/common/cpu.c | 4 +- xen/common/cpupool.c | 95 +++++++++++++++++++++++++++++++++++--------------- 2 files changed, 69 insertions(+), 30 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PATCH 1/3 xen: always print offending CPU on bringup/teardown failure
In fact, before this change, if bringing up or tearing down a CPU fails with -EBUSY, we BUG_ON() and never get to see what CPU caused the problem.
--- xen/common/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
xen/common/cpu.c +2 -2
@@ -187,8 +187,8 @@ int disable_nonboot_cpus(void)if ( (error =cpu_down (cpu)) ) { -BUG_ON (error == -EBUSY );printk ("Error taking CPU%d down: %d\n" , cpu, error); +BUG_ON (error == -EBUSY );break ; }
@@ -209,8 +209,8 @@ void enable_nonboot_cpus(void) {if ( (error =cpu_up (cpu)) ) { -BUG_ON (error == -EBUSY );printk ("Error taking CPU%d up: %d\n" , cpu, error); +BUG_ON (error == -EBUSY ); } }
Re: [PATCH 1/3] xen: always print offending CPU on bringup/teardown failure
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 06.05.15 at 17:10, <[email protected]> wrote:
In fact, before this change, if bringing up or tearing down a CPU fails with
-EBUSY, weBUG_ON()and never get to see what CPU caused the problem.
Quoted 7 lines
Signed-off-by Dario Faggioli <[email protected]>Cc Jan Beulich <[email protected]>Cc Andrew Cooper <[email protected]>Cc Keir Fraser <[email protected]>--- xen/common/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I committed this, but please note that your Cc list looks like you considered this an x86-specific change.
Jan
PATCH 2/3 xen: cpupool: assigning a CPU to a pool can fail
In reply to Dario Faggioliwhich means such an event must be handled at the call sites of cpupool_assign_cpu_locked(), and the error, if occurring, properly propagated.
--- xen/common/cpupool.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
xen/common/cpupool.c +11 -4
@@ -458,8 +458,10 @@ void cpupool_rm_domain(struct domain *d)* unless we are resuming from S3, in which case we put the cpu back * in the cpupool it was in prior to suspend. */ -static void cpupool_cpu_add (unsigned int cpu) +static int cpupool_cpu_add (unsigned int cpu) { +int ret = -EINVAL ; +spin_lock (&cpupool_lock);cpumask_clear_cpu (cpu, &cpupool_locked_cpus);cpumask_set_cpu (cpu, &cpupool_free_cpus);
@@ -472,15 +474,20 @@ static void cpupool_cpu_add(unsigned int cpu) {if (cpumask_test_cpu (cpu, (*c)->cpu_suspended ) ) { -cpupool_assign_cpu_locked (*c, cpu); + ret =cpupool_assign_cpu_locked (*c, cpu); +if ( ret ) + goto out;cpumask_clear_cpu (cpu, (*c)->cpu_suspended); } } }if (cpumask_test_cpu (cpu, &cpupool_free_cpus) ) -cpupool_assign_cpu_locked (cpupool0, cpu); + ret =cpupool_assign_cpu_locked (cpupool0, cpu); + out:spin_unlock (&cpupool_lock); + +return ret; }/*
@@ -715,7 +722,7 @@ static int cpu_callback( {case CPU_DOWN_FAILED :case CPU_ONLINE : -cpupool_cpu_add (cpu); + rc =cpupool_cpu_add (cpu);break ;case CPU_DOWN_PREPARE : rc =cpupool_cpu_remove (cpu);
Re: [PATCH 2/3] xen: cpupool: assigning a CPU to a pool can fail
Quoted 10 lines · Dario Faggioli
which means such an event must be handled at the call sites of
cpupool_assign_cpu_locked(), and the error, if occurring, properly propagated.Signed-off-by Dario Faggioli <[email protected]>Cc Juergen Gross <[email protected]>Cc George Dunlap <[email protected]>Cc Jan Beulich <[email protected]>Cc Andrew Cooper <[email protected]>Cc Keir Fraser <[email protected]>
Quoted 54 lines
--- xen/common/cpupool.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)xen/common/cpupool.c +11
-4@@ -458,8 +458,10 @@ void cpupool_rm_domain(struct domain *d)* unless we are resuming from S3, in which case we put the cpu back * in the cpupool it was in prior to suspend. */ -static void cpupool_cpu_add (unsigned int cpu) +static int cpupool_cpu_add (unsigned int cpu) { +int ret = -EINVAL ; +spin_lock (&cpupool_lock);cpumask_clear_cpu (cpu, &cpupool_locked_cpus);cpumask_set_cpu (cpu, &cpupool_free_cpus);@@ -472,15 +474,20 @@ static void cpupool_cpu_add(unsigned int cpu) {if (cpumask_test_cpu (cpu, (*c)->cpu_suspended ) ) { -cpupool_assign_cpu_locked (*c, cpu); + ret =cpupool_assign_cpu_locked (*c, cpu); +if ( ret ) + goto out;cpumask_clear_cpu (cpu, (*c)->cpu_suspended); } } }if (cpumask_test_cpu (cpu, &cpupool_free_cpus) ) -cpupool_assign_cpu_locked (cpupool0, cpu); + ret =cpupool_assign_cpu_locked (cpupool0, cpu); + out:spin_unlock (&cpupool_lock); + +return ret; }/* @@ -715,7 +722,7 @@ static int cpu_callback( {case CPU_DOWN_FAILED :case CPU_ONLINE : -cpupool_cpu_add (cpu); + rc =cpupool_cpu_add (cpu);break ;case CPU_DOWN_PREPARE : rc =cpupool_cpu_remove (cpu);
PATCH 3/3 xen: cpupools: avoid crashing if shutting down with free CPUs
In reply to Dario Faggioliin fact, before this change, shutting down or suspending the system with some CPUs not assigned to any cpupool, would crash as follows:
(XEN) Xen call trace:
(XEN) [<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138 (XEN) [<ffff82d0801a8824>] enter_state_helper+0xbd/0x369 (XEN) [<ffff82d08010614a>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d0801320bd>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d0801323f3>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080163cb6>] idle_loop+0x56/0x6b
(XEN)
(XEN)
(XEN) **************************************** (XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:191
(XEN) ****************************************
This is because, for free CPUs, -EBUSY were being returned when trying to tear them down, making cpu_down() unhappy.
It is certainly unpractical to forbid shutting down or suspenging if there are unassigned CPUs, so this change fixes the above by just avoiding returning -EBUSY for those CPUs. If shutting off, that does not matter much anyway. If suspending, we make sure that the CPUs remain unassigned when resuming.
While there, take the chance to:
- fix the doc comment of cpupool_cpu_remove() (it was wrong);
- improve comments in general around and in cpupool_cpu_remove() and cpupool_cpu_add();
- add a couple of ASSERT()-s for checking consistency.
--- xen/common/cpupool.c | 80 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 24 deletions(-)
xen/common/cpupool.c +56 -24
@@ -453,10 +453,12 @@ void cpupool_rm_domain(struct domain *d) }/* -* called to add a new cpu to pool admin -* we add a hotplugged cpu to the cpupool0 to be able to add it to dom0, -* unless we are resuming from S3, in which case we put the cpu back -* in the cpupool it was in prior to suspend. +* Called to add a cpu to a pool. CPUs being hot-plugged are added to pool0, +* as they must have been in there when unplugged. +* +* If, on the other hand, we are adding CPUs because we are resuming (e.g., +* after ACPI S3) we put the cpu back in the pool where it was in prior when +* we suspended. */ static int cpupool_cpu_add (unsigned int cpu) {
@@ -478,12 +480,28 @@ static int cpupool_cpu_add(unsigned int cpu)if ( ret ) goto out;cpumask_clear_cpu (cpu, (*c)->cpu_suspended); +break ; } } - } -if (cpumask_test_cpu (cpu, &cpupool_free_cpus) ) +/* +* Either cpu has been found as suspended in a pool, and added back +* there, or it stayed free (if it did not belong to any pool when +* suspending), and we don't want to do anything. +*/ +ASSERT (cpumask_test_cpu (cpu, &cpupool_free_cpus) || +cpumask_test_cpu (cpu, (*c)->cpu_valid)); + } +else + { +/* +* If we are not resuming, we are hot-plugging cpu, and in which case +* we add it to pool0, as it certainly was there when hot-unplagged +* (or unplugging would have failed) and that is the default behavior +* anyway. +*/ ret =cpupool_assign_cpu_locked (cpupool0, cpu); + } out:spin_unlock (&cpupool_lock);
@@ -491,29 +509,52 @@ static int cpupool_cpu_add(unsigned int cpu) }/* -* called to remove a cpu from pool admin -* the cpu to be removed is locked to avoid removing it from dom0 -* returns failure if not in pool0 +* Called to remove a CPU from a pool. The CPU is locked, to forbid removing +* it from pool0. In fact, if we want to hot-unplug a CPU, it must belong to +* pool0, or we fail. +* +* However, if we are suspending (e.g., to ACPI S3), we mark the CPU in such +* a way that it can be put back in its pool when resuming. */ static int cpupool_cpu_remove (unsigned int cpu) {int ret = -EBUSY ; -struct cpupool **c;spin_lock (&cpupool_lock); -if (cpumask_test_cpu (cpu, cpupool0->cpu_valid) ) - ret =0 ; -else +if ( system_state == SYS_STATE_suspend ) { +struct cpupool **c; +for_each_cpupool (c) { -if (cpumask_test_cpu (cpu, (*c)->cpu_suspended ) ) +if (cpumask_test_cpu (cpu, (*c)->cpu_valid ) ) { - ret =0 ; +cpumask_set_cpu (cpu, (*c)->cpu_suspended);break ; } } + +/* +* Either we found cpu in a pool, or it must be free (if it has been +* hot-unplagged, then we must have found it in pool0). It is, of +* course, fine to suspend or shutdown with CPUs not assigned to a +* pool, and (in case of suspend) they will stay free when resuming. +*/ +ASSERT (cpumask_test_cpu (cpu, &cpupool_free_cpus) || +cpumask_test_cpu (cpu, (*c)->cpu_suspended)); +ASSERT (cpumask_test_cpu (cpu, &cpu_online_map) || +cpumask_test_cpu (cpu, cpupool0->cpu_suspended)); + ret =0 ; + } +else if (cpumask_test_cpu (cpu, cpupool0->cpu_valid) ) + { +/* +* If we are not suspending, we are hot-unplugging cpu, and that is +* allowed only for CPUs in pool0. +*/ + ret =0 ; } +if ( !ret )cpumask_set_cpu (cpu, &cpupool_locked_cpus);spin_unlock (&cpupool_lock);
@@ -709,15 +750,6 @@ static int cpu_callback(unsigned int cpu = (unsigned long )hcpu;int rc =0 ; -if ( system_state == SYS_STATE_suspend ) - { -struct cpupool **c; - -for_each_cpupool (c) -if (cpumask_test_cpu (cpu, (*c)->cpu_valid ) ) -cpumask_set_cpu (cpu, (*c)->cpu_suspended); - } -switch ( action ) {case CPU_DOWN_FAILED :
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 28 lines · Dario Faggioli
in fact, before this change, shutting down or suspending the system with some CPUs not assigned to any cpupool, would crash as follows:
(XEN) Xen call trace: (XEN) [<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138 (XEN) [<ffff82d0801a8824>] enter_state_helper+0xbd/0x369 (XEN) [<ffff82d08010614a>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d0801320bd>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d0801323f3>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080163cb6>] idle_loop+0x56/0x6b (XEN) (XEN) (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) Xen BUG at cpu.c:191 (XEN) ****************************************This is because, for free CPUs,
-EBUSYwere being returned when trying to tear them down, makingcpu_down()unhappy.It is certainly unpractical to forbid shutting down or suspenging if there are unassigned CPUs, so this change fixes the above by just avoiding returning
-EBUSYfor those CPUs. If shutting off, that does not matter much anyway. If suspending, we make sure that the CPUs remain unassigned when resuming.
Quoted 6 lines
While there, take the chance to:
- fix the doc comment ofcpupool_cpu_remove()(it waswrong); - improve comments in general around and in cpupool_cpu_remove() and cpupool_cpu_add();- add a couple of ASSERT()-s for checking consistency.
I did a test with the patches applied.
# xl cpupool-cpu-remove Pool-0 2 # echo mem >/sys/power/state
When resuming this resulted in:
(XEN) mce_intel.c:735: MCA Capability: BCAST 1 SER 0 CMCI 1 firstbank 0 extended MCE MSR 0 (XEN) CPU0 CMCI LVT vector (0xf2) already installed
(XEN) Finishing wakeup from ACPI S3 state.
(XEN) Enabling non-boot CPUs ...
(XEN) Xen BUG at cpu.c:149
(XEN) ----[ Xen-4.6-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 0
(XEN) RIP: e008:[<ffff82d080101531>] cpu_up+0xaf/0xfe
(XEN) RFLAGS: 0000000000010202 CONTEXT: hypervisor
(XEN) rax: 0000000000008016 rbx: 0000000000000000 rcx: 0000000000000000 (XEN) rdx: ffff82d0802e0000 rsi: 0000000000000004 rdi: ffff82d080296488 (XEN) rbp: ffff82d0802e7e00 rsp: ffff82d0802e7de0 r8: 0000000000081000 (XEN) r9: ffff82d15ba99600 r10: 00000000dba99600 r11: ffff82d080299600 (XEN) r12: 0000000000000002 r13: ffff82d08025cf6c r14: 0000000000000002 (XEN) r15: ffff82d0802e0000 cr0: 000000008005003b cr4: 00000000001526f0 (XEN) cr3: 00000000dba94000 cr2: 0000000000000000 (XEN) ds: 0018 es: 0010 fs: 8100 gs: 0010 ss: e010 cs: e008 (XEN) Xen stack trace from rsp=ffff82d0802e7de0: (XEN) ffff82d0802e0000 ffff82d080297360 0000000000000002 0000000000000004 (XEN) ffff82d0802e7e30 ffff82d080101733 0000000000000003 0000000000000000 (XEN) ffff82d080329130 00000000001526f0 ffff82d0802e7e80 ffff82d0801a6a8d (XEN) ffff82d0802e7e60 0000000000000282 ffff82d080328de8 ffff8301351c4fd0 (XEN) ffff8300d8758000 ffff82d080328ec0 ffff82d080328de8 ffffffffffffffff (XEN) ffff82d0802e7ea0 ffff82d08010615f ffff8300d87581c8 0000000000000000 (XEN) ffff82d0802e7ec0 ffff82d08013101d ffff82d08012ebc4 ffff82d080328ed0 (XEN) ffff82d0802e7ef0 ffff82d08013134c 00000021dc728d2c ffff82d0802e0000 (XEN) 00000021dc728d2c ffff8300dbcf0000 ffff82d0802e7f10 ffff82d080161bcb (XEN) ffff82d08012e37a ffff8300dbcf0000 ffff82d0802e7e10 ffff8801fa5e8000 (XEN) ffff8801fa5e8000 ffff8801fa5e8000 0000000000000000 0000000000000000 (XEN) ffffffff818cb700 0000000000000246 0000000000000017 0000000000000007 (XEN) 000000000000e3b8 0000000000000000 ffffffff8100130a 00000000deadbeef (XEN) 00000000deadbeef 00000000deadbeef 0000010000000000 ffffffff8100130a (XEN) 000000000000e033 0000000000000246 ffff8801fa5ebed8 000000000000e02b (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 ffff8300dbcf0000 0000000000000000 0000000000000000 (XEN) Xen call trace: (XEN) [<ffff82d080101531>] cpu_up+0xaf/0xfe (XEN) [<ffff82d080101733>] enable_nonboot_cpus+0x4f/0xfc (XEN) [<ffff82d0801a6a8d>] enter_state_helper+0x2cb/0x370 (XEN) [<ffff82d08010615f>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d08013101d>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d08013134c>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080161bcb>] idle_loop+0x56/0x70
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:149
(XEN) ****************************************
Juergen
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 55 lines
Quoted 1 line
Quoted 1 line · On 08.05.15 at 12:20, <[email protected]> wrote:
Quoted 28 lines · Dario Faggioli
in fact, before this change, shutting down or suspending the system with some CPUs not assigned to any cpupool, would crash as follows:
(XEN) Xen call trace: (XEN) [<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138 (XEN) [<ffff82d0801a8824>] enter_state_helper+0xbd/0x369 (XEN) [<ffff82d08010614a>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d0801320bd>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d0801323f3>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080163cb6>] idle_loop+0x56/0x6b (XEN) (XEN) (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) Xen BUG at cpu.c:191 (XEN) ****************************************This is because, for free CPUs,
-EBUSYwere being returned when trying to tear them down, makingcpu_down()unhappy.It is certainly unpractical to forbid shutting down or suspenging if there are unassigned CPUs, so this change fixes the above by just avoiding returning
-EBUSYfor those CPUs. If shutting off, that does not matter much anyway. If suspending, we make sure that the CPUs remain unassigned when resuming.Quoted 6 lines
While there, take the chance to:
- fix the doc comment ofcpupool_cpu_remove()(it waswrong); - improve comments in general around and in cpupool_cpu_remove() and cpupool_cpu_add();- add a couple of ASSERT()-s for checking consistency.
I did a test with the patches applied.
# xl cpupool-cpu-remove Pool-0 2 # echo mem >/sys/power/stateWhen resuming this resulted in:
(XEN) mce_intel.c:735: MCA Capability: BCAST 1 SER 0 CMCI 1 firstbank 0 extended MCE MSR 0
(XEN) CPU0 CMCI LVT vector (0xf2) already installed
(XEN) Finishing wakeup from ACPI S3 state.
(XEN) Enabling non-boot CPUs ...
(XEN) Xen BUG at cpu.c:149
(XEN) ----[ Xen-4.6-unstablex86_64debug=y Tainted: C ]----
(XEN) CPU: 0
(XEN) RIP: e008:[<ffff82d080101531>]cpu_up+0xaf/0xfe
(XEN) RFLAGS: 0000000000010202 CONTEXT: hypervisor
(XEN) rax: 0000000000008016 rbx: 0000000000000000 rcx: 0000000000000000
[...]
Quoted 8 lines
(XEN) Xen call trace:
(XEN) [<ffff82d080101531>] cpu_up+0xaf/0xfe (XEN) [<ffff82d080101733>] enable_nonboot_cpus+0x4f/0xfc (XEN) [<ffff82d0801a6a8d>] enter_state_helper+0x2cb/0x370 (XEN) [<ffff82d08010615f>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d08013101d>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d08013134c>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080161bcb>] idle_loop+0x56/0x70
Quoted 6 lines
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:149
(XEN) ****************************************
Which would seem to more likely be a result of patch 2. Having taken a closer look - is setting ret to -EINVAL at the top of cpupool_cpu_add() really correct? I.e. it is guaranteed that at least one of the two places altering ret will always be run into? If it is, then I'd still suspect one of the two cpupool_assign_cpu_locked() invocations to be failing.
In any event, unless confirmed otherwise we may need to revert patch 2 for the time being.
Jan
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 71 lines · Jan Beulich
Quoted 55 lines
Quoted 1 line
Quoted 1 line · On 08.05.15 at 12:20, <[email protected]> wrote:
Quoted 28 lines · Dario Faggioli
in fact, before this change, shutting down or suspending the system with some CPUs not assigned to any cpupool, would crash as follows:
(XEN) Xen call trace: (XEN) [<ffff82d080101757>] disable_nonboot_cpus+0xb5/0x138 (XEN) [<ffff82d0801a8824>] enter_state_helper+0xbd/0x369 (XEN) [<ffff82d08010614a>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d0801320bd>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d0801323f3>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080163cb6>] idle_loop+0x56/0x6b (XEN) (XEN) (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) Xen BUG at cpu.c:191 (XEN) ****************************************This is because, for free CPUs,
-EBUSYwere being returned when trying to tear them down, makingcpu_down()unhappy.It is certainly unpractical to forbid shutting down or suspenging if there are unassigned CPUs, so this change fixes the above by just avoiding returning
-EBUSYfor those CPUs. If shutting off, that does not matter much anyway. If suspending, we make sure that the CPUs remain unassigned when resuming.Quoted 6 lines
While there, take the chance to:
- fix the doc comment ofcpupool_cpu_remove()(it waswrong); - improve comments in general around and in cpupool_cpu_remove() and cpupool_cpu_add();- add a couple of ASSERT()-s for checking consistency.
I did a test with the patches applied.
# xl cpupool-cpu-remove Pool-0 2 # echo mem >/sys/power/stateWhen resuming this resulted in:
(XEN) mce_intel.c:735: MCA Capability: BCAST 1 SER 0 CMCI 1 firstbank 0 extended MCE MSR 0
(XEN) CPU0 CMCI LVT vector (0xf2) already installed
(XEN) Finishing wakeup from ACPI S3 state.
(XEN) Enabling non-boot CPUs ...
(XEN) Xen BUG at cpu.c:149
(XEN) ----[ Xen-4.6-unstablex86_64debug=y Tainted: C ]----
(XEN) CPU: 0
(XEN) RIP: e008:[<ffff82d080101531>]cpu_up+0xaf/0xfe
(XEN) RFLAGS: 0000000000010202 CONTEXT: hypervisor
(XEN) rax: 0000000000008016 rbx: 0000000000000000 rcx: 0000000000000000[...]
Quoted 8 lines
(XEN) Xen call trace:
(XEN) [<ffff82d080101531>] cpu_up+0xaf/0xfe (XEN) [<ffff82d080101733>] enable_nonboot_cpus+0x4f/0xfc (XEN) [<ffff82d0801a6a8d>] enter_state_helper+0x2cb/0x370 (XEN) [<ffff82d08010615f>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d08013101d>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d08013134c>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080161bcb>] idle_loop+0x56/0x70Quoted 6 lines
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:149
(XEN) ****************************************
Quoted 6 lines
Which would seem to more likely be a result of patch 2. Having taken a closer look - is setting ret to
-EINVALat the top ofcpupool_cpu_add()really correct? I.e. it is guaranteed that at least one of the two places altering ret will always be run into? If it is, then I'd still suspect one of the twocpupool_assign_cpu_locked()invocations to be failing.
Indeed. Setting ret to 0 initially does the trick. With this modification suspend/resume and power off are working with cpus not allocated to any cpupool.
Dario, I suggest you write another patch to correct patch 2.
For patch 3 with patch 2 corrected:
Juergen
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 17 lines · Juergen Gross
Quoted 15 lines · Jan Beulich
Quoted 14 lines
(XEN) Xen call trace:
(XEN) [<ffff82d080101531>] cpu_up+0xaf/0xfe (XEN) [<ffff82d080101733>] enable_nonboot_cpus+0x4f/0xfc (XEN) [<ffff82d0801a6a8d>] enter_state_helper+0x2cb/0x370 (XEN) [<ffff82d08010615f>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d08013101d>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d08013134c>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080161bcb>] idle_loop+0x56/0x70(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:149
(XEN) ****************************************
Quoted 9 lines
Quoted 6 lines
Which would seem to more likely be a result of patch 2. Having taken a closer look - is setting ret to
-EINVALat the top ofcpupool_cpu_add()really correct? I.e. it is guaranteed that at least one of the two places altering ret will always be run into? If it is, then I'd still suspect one of the twocpupool_assign_cpu_locked()invocations to be failing.Indeed.
Not really.
Well, the problem is, of course, related, as your test shows, and I now see why this happens, but it's all patch 3 fault (see below).
So what's in tree right now is ok and there is no need to revert. I believe the best thing to do is for me to send a new, fixed, version of patch 3. The fix would probably still be just changing "int ret = -EINVAL" to "int ret = 0" in cpupool_cpu_add(), but that should be done within patch 3, not as a fix to patch 2, which was indeed right.
What do you both think?
Quoted 2 lines
Setting ret to 0 initially does the trick.
Yes. However, as far as patch 2 is concerned, that initialization to -EINVAL is ok, as we are sure and it is guaranted that at least one of the two places altering ret is executed, as Jan was wandering. (Well, because of that, the initialization is not that important, I just added it to be extra-cautious.)
The problem is, in patch 3, when that code becomes:
int ret = -EINVAL;
if ( system_state == SYS_STATE_resume )
{
<look for the cpu>
ret = cpupool_assign_cpu_locked(*c, cpu);
}
else
{
ret = cpupool_assign_cpu_locked(cpupool0, cpu);
}In fact, now, if the cpu was free when suspending, we won't find it anywhere when looking for it in the system_state==SYS_STATE_resume case, and hence we won't call cpupool_assign_cpu_locked(). Then, because of the 'if() else', we don't call it below either (as we did before), and hence no one alters 'ret'.
That is my point, actually: in patch 2, we are sure ret will be altered. In patch 3, it's no longer guaranteed that we alter ret, and the case in which we don't is perfectly fine, so ret should be inited to 0.
Quoted 4 lines
With this
modification suspend/resume and power off are working with cpus not allocated to any cpupool.
Great to know, thanks for testing... and sorry for not having been able to do so myself. My test box allows me to "echo mem >/sys/power/state", and it seems to suspend ok (e.g., power led is blinking)... but then it just does not resume. :-/
Quoted 7 lines
Dario, I suggest you write another patch to correct patch 2.
For patch 3 with patch 2 corrected:
Reviewed-by Juergen Gross <[email protected]>Tested-by Juergen Gross <[email protected]>
If you agree on my plan of sending v2 of patch3, and if that will really be just the same of v1, but with "int ret=0", I'll stick these tags there, unless you tell me not to.
Thanks again and Regards,
Dario
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 84 lines · Dario Faggioli
Quoted 17 lines · Juergen Gross
Quoted 15 lines · Jan Beulich
Quoted 14 lines
(XEN) Xen call trace:
(XEN) [<ffff82d080101531>] cpu_up+0xaf/0xfe (XEN) [<ffff82d080101733>] enable_nonboot_cpus+0x4f/0xfc (XEN) [<ffff82d0801a6a8d>] enter_state_helper+0x2cb/0x370 (XEN) [<ffff82d08010615f>] continue_hypercall_tasklet_handler+0x4a/0xb1 (XEN) [<ffff82d08013101d>] do_tasklet_work+0x78/0xab (XEN) [<ffff82d08013134c>] do_tasklet+0x5e/0x8a (XEN) [<ffff82d080161bcb>] idle_loop+0x56/0x70(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at cpu.c:149
(XEN) ****************************************Quoted 9 lines
Quoted 6 lines
Which would seem to more likely be a result of patch 2. Having taken a closer look - is setting ret to
-EINVALat the top ofcpupool_cpu_add()really correct? I.e. it is guaranteed that at least one of the two places altering ret will always be run into? If it is, then I'd still suspect one of the twocpupool_assign_cpu_locked()invocations to be failing.Indeed.
Not really.
Well, the problem is, of course, related, as your test shows, and I now see why this happens, but it's all patch 3 fault (see below).
So what's in tree right now is ok and there is no need to revert. I believe the best thing to do is for me to send a new, fixed, version of patch 3. The fix would probably still be just changing "int ret =
-EINVAL" to "int ret = 0" incpupool_cpu_add(), but that should be done within patch 3, not as a fix to patch 2, which was indeed right.What do you both think?
Quoted 2 lines
Setting ret to 0 initially does the trick.
Yes. However, as far as patch 2 is concerned, that initialization to
-EINVALis ok, as we are sure and it is guaranted that at least one of the two places altering ret is executed, as Jan was wandering. (Well, because of that, the initialization is not that important, I just added it to be extra-cautious.)The problem is, in patch 3, when that code becomes:
int ret = -EINVAL; if ( system_state == SYS_STATE_resume ) { <look for the cpu> ret = cpupool_assign_cpu_locked(*c, cpu); } else { ret = cpupool_assign_cpu_locked(cpupool0, cpu); }In fact, now, if the cpu was free when suspending, we won't find it anywhere when looking for it in the system_state==SYS_STATE_resume case, and hence we won't call
cpupool_assign_cpu_locked(). Then, because of the 'if()else', we don't call it below either (as we did before), and hence no one alters 'ret'.That is my point, actually: in patch 2, we are sure ret will be altered. In patch 3, it's no longer guaranteed that we alter ret, and the case in which we don't is perfectly fine, so ret should be inited to 0.
Quoted 4 lines
With this
modification suspend/resume and power off are working with cpus not allocated to any cpupool.Great to know, thanks for testing... and sorry for not having been able to do so myself. My test box allows me to "echo mem >/sys/power/state", and it seems to suspend ok (e.g., power led is blinking)... but then it just does not resume. :-/
Quoted 2 lines
Dario, I suggest you write another patch to correct patch 2.
Quoted 8 lines
Quoted 5 lines
For patch 3 with patch 2 corrected:
Reviewed-by Juergen Gross <[email protected]>Tested-by Juergen Gross <[email protected]>If you agree on my plan of sending v2 of patch3, and if that will really be just the same of v1, but with "int ret=0", I'll stick these tags there, unless you tell me not to.
I don't mind how you are doing it. The machine crashed even without patch 2 when suspending with at least one free cpu, so this patch isn't making anything worse.
You can still apply my 2 *.by: tags, of course.
Juergen
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
Quoted 10 lines · Juergen Gross
Quoted 3 lines · Dario Faggioli
If you agree on my plan of sending v2 of patch3, and if that will really be just the same of v1, but with "int ret=0", I'll stick these tags there, unless you tell me not to.
I don't mind how you are doing it. The machine crashed even without patch 2 when suspending with at least one free cpu, so this patch isn't making anything worse.
Indeed! I can't tell about suspend, but I've seen it happening on shutdown... that's the purpose of the series! :-)
Quoted 2 lines
You can still apply my 2 *.by: tags, of course.
Ok, thanks.
Regards,
Dario
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs
In reply to Dario FaggioliQuoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.05.15 at 15:12, <[email protected]> wrote:
If you agree on my plan of sending v2 of patch3, and if that will really be just the same of v1, but with "int ret=0",
Sounds fine to me too.
Jan