[PATCH 0/3] xen: cpupools: avoid crashing when shutting down/suspending with free CPUs

[PATCH 0/3] xen: cpupools: avoid crashing when shutting down/suspending with free CPUs

[Dario Faggioli]

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

[Dario Faggioli]

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.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/cpu.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 47e8b5b..8f5eb4b 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -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);
         }
     }

[PATCH 2/3] xen: cpupool: assigning a CPU to a pool can fail

[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 <dario.faggioli@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/cpupool.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index a947c24..cabaccd 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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

[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, -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.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/cpupool.c |   80 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index cabaccd..07829c6 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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 2/3] xen: cpupool: assigning a CPU to a pool can fail

[Juergen Gross]

On 05/06/2015 05:10 PM, Dario Faggioli wrote:
> 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 <dario.faggioli@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Keir Fraser <keir@xen.org>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
>   xen/common/cpupool.c |   15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index a947c24..cabaccd 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -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 1/3] xen: always print offending CPU on bringup/teardown failure

[Jan Beulich]

>>> On 06.05.15 at 17:10, <dario.faggioli@citrix.com> wrote:
> 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.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> ---
>  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

Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs

[Juergen Gross]

On 05/06/2015 05:10 PM, Dario Faggioli wrote:
> 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, -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.

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

[Jan Beulich]

>>> On 08.05.15 at 12:20, <JGross@suse.com> wrote:
> On 05/06/2015 05:10 PM, Dario Faggioli wrote:
>> 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, -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.
> 
> 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) 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) ****************************************

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

Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs

[Juergen Gross]

On 05/08/2015 12:34 PM, Jan Beulich wrote:
>>>> On 08.05.15 at 12:20, <JGross@suse.com> wrote:
>> On 05/06/2015 05:10 PM, Dario Faggioli wrote:
>>> 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, -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.
>>
>> 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) 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) ****************************************
>
> 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.

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:

Reviewed-by: Juergen Gross <jgross@suse.com>
Tested-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3699 bytes --]

On Fri, 2015-05-08 at 12:47 +0200, Juergen Gross wrote:
> On 05/08/2015 12:34 PM, Jan Beulich wrote:

> >> (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) ****************************************
> >
> > 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.
> 
> 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?

> 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.

> 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. :-/

> Dario, I suggest you write another patch to correct patch 2.
> 
> For patch 3 with patch 2 corrected:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> Tested-by: Juergen Gross <jgross@suse.com>
> 
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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs

[Juergen Gross]

On 05/08/2015 03:12 PM, Dario Faggioli wrote:
> On Fri, 2015-05-08 at 12:47 +0200, Juergen Gross wrote:
>> On 05/08/2015 12:34 PM, Jan Beulich wrote:
>
>>>> (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) ****************************************
>>>
>>> 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.
>>
>> 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?
>
>> 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.
>
>> 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. :-/
>
>> Dario, I suggest you write another patch to correct patch 2.
>>
>> For patch 3 with patch 2 corrected:
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
>> Tested-by: Juergen Gross <jgross@suse.com>
>>
> 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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 685 bytes --]

On Fri, 2015-05-08 at 15:18 +0200, Juergen Gross wrote:
> On 05/08/2015 03:12 PM, Dario Faggioli 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", 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! :-)

> You can still apply my 2 *.by: tags, of course.
> 
Ok, thanks.

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] xen: cpupools: avoid crashing if shutting down with free CPUs

[Jan Beulich]

>>> On 08.05.15 at 15:12, <dario.faggioli@citrix.com> 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