[PATCH 0/6] xen: simplify suspend/resume handling

[PATCH 0/6] xen: simplify suspend/resume handling

[Juergen Gross]

Especially in the scheduler area (schedule.c, cpupool.c) there is a
rather complex handling involved when doing suspend and resume.

This can be simplified a lot by not performing a complete cpu down and
up cycle for the non-boot cpus, but keeping the pure software related
state and freeing it only in case a cpu didn't come up again during
resume.

In summary not only the complexity can be reduced, but the failure
tolerance will be even better with this series: With a dedicated hook
for failing cpus when resuming it is now possible to survive e.g. a
cpupool being left without any cpu after resume by moving its domains
to cpupool0.

Juergen Gross (6):
  xen/sched: call cpu_disable_scheduler() via cpu notifier
  xen: add helper for calling notifier_call_chain() to common/cpu.c
  xen: add new cpu notifier action CPU_RESUME_FAILED
  xen: don't free percpu areas during suspend
  xen/cpupool: simplify suspend/resume handling
  xen/sched: don't disable scheduler on cpus during suspend

 xen/arch/x86/percpu.c      |   3 +-
 xen/arch/x86/smpboot.c     |   3 -
 xen/common/cpu.c           |  55 +++++++-------
 xen/common/cpupool.c       | 130 +++++++++++++---------------------
 xen/common/schedule.c      | 173 +++++++++++++++------------------------------
 xen/include/xen/cpu.h      |  20 +++---
 xen/include/xen/sched-if.h |   1 -
 7 files changed, 147 insertions(+), 238 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Juergen Gross]

cpu_disable_scheduler() is being called from __cpu_disable() today.
There is no need to call it on the cpu just being disabled, so use
the CPU_DEAD case of the cpu notifier chain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/smpboot.c |  3 ---
 xen/common/schedule.c  | 12 +++++-------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7d1226d7bc..b7a0a4a419 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1221,9 +1221,6 @@ void __cpu_disable(void)
     cpumask_clear_cpu(cpu, &cpu_online_map);
     fixup_irqs(&cpu_online_map, 1);
     fixup_eoi();
-
-    if ( cpu_disable_scheduler(cpu) )
-        BUG();
 }
 
 void __cpu_die(unsigned int cpu)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 60755a631e..665747f247 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -773,8 +773,9 @@ void restore_vcpu_affinity(struct domain *d)
 }
 
 /*
- * This function is used by cpu_hotplug code from stop_machine context
+ * This function is used by cpu_hotplug code via cpu notifier chain
  * and from cpupools to switch schedulers on a cpu.
+ * Caller must get domlist_read_lock.
  */
 int cpu_disable_scheduler(unsigned int cpu)
 {
@@ -789,12 +790,6 @@ int cpu_disable_scheduler(unsigned int cpu)
     if ( c == NULL )
         return ret;
 
-    /*
-     * We'd need the domain RCU lock, but:
-     *  - when we are called from cpupool code, it's acquired there already;
-     *  - when we are called for CPU teardown, we're in stop-machine context,
-     *    so that's not be a problem.
-     */
     for_each_domain_in_cpupool ( d, c )
     {
         for_each_vcpu ( d, v )
@@ -1738,6 +1733,9 @@ static int cpu_schedule_callback(
         rc = cpu_schedule_up(cpu);
         break;
     case CPU_DEAD:
+        rcu_read_lock(&domlist_read_lock);
+        cpu_disable_scheduler(cpu);
+        rcu_read_unlock(&domlist_read_lock);
         SCHED_OP(sched, deinit_pdata, sd->sched_priv, cpu);
         /* Fallthrough */
     case CPU_UP_CANCELED:
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

[Juergen Gross]

Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
for a cpu with a specified action, returning an errno value.

This avoids coding the same pattern multiple times.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 836c62f97f..c436c0de7f 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -71,11 +71,18 @@ void __init register_cpu_notifier(struct notifier_block *nb)
     spin_unlock(&cpu_add_remove_lock);
 }
 
+static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
+                                   struct notifier_block **nb)
+{
+    void *hcpu = (void *)(long)cpu;
+    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
+
+    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
+}
+
 static void _take_cpu_down(void *unused)
 {
-    void *hcpu = (void *)(long)smp_processor_id();
-    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));
     __cpu_disable();
 }
 
@@ -87,8 +94,7 @@ static int take_cpu_down(void *arg)
 
 int cpu_down(unsigned int cpu)
 {
-    int err, notifier_rc;
-    void *hcpu = (void *)(long)cpu;
+    int err;
     struct notifier_block *nb = NULL;
 
     if ( !cpu_hotplug_begin() )
@@ -100,12 +106,9 @@ int cpu_down(unsigned int cpu)
         return -EINVAL;
     }
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE, hcpu, &nb);
-    if ( notifier_rc != NOTIFY_DONE )
-    {
-        err = notifier_to_errno(notifier_rc);
+    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb);
+    if ( err )
         goto fail;
-    }
 
     if ( unlikely(system_state < SYS_STATE_active) )
         on_selected_cpus(cpumask_of(cpu), _take_cpu_down, NULL, true);
@@ -115,24 +118,21 @@ int cpu_down(unsigned int cpu)
     __cpu_die(cpu);
     BUG_ON(cpu_online(cpu));
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DEAD, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_DEAD, NULL));
 
     send_global_virq(VIRQ_PCPU_STATE);
     cpu_hotplug_done();
     return 0;
 
  fail:
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED, hcpu, &nb);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, &nb));
     cpu_hotplug_done();
     return err;
 }
 
 int cpu_up(unsigned int cpu)
 {
-    int notifier_rc, err = 0;
-    void *hcpu = (void *)(long)cpu;
+    int err;
     struct notifier_block *nb = NULL;
 
     if ( !cpu_hotplug_begin() )
@@ -144,19 +144,15 @@ int cpu_up(unsigned int cpu)
         return -EINVAL;
     }
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu, &nb);
-    if ( notifier_rc != NOTIFY_DONE )
-    {
-        err = notifier_to_errno(notifier_rc);
+    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb);
+    if ( err )
         goto fail;
-    }
 
     err = __cpu_up(cpu);
     if ( err < 0 )
         goto fail;
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL));
 
     send_global_virq(VIRQ_PCPU_STATE);
 
@@ -164,18 +160,14 @@ int cpu_up(unsigned int cpu)
     return 0;
 
  fail:
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_UP_CANCELED, hcpu, &nb);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, &nb));
     cpu_hotplug_done();
     return err;
 }
 
 void notify_cpu_starting(unsigned int cpu)
 {
-    void *hcpu = (void *)(long)cpu;
-    int notifier_rc = notifier_call_chain(
-        &cpu_chain, CPU_STARTING, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_STARTING, NULL));
 }
 
 static cpumask_t frozen_cpus;
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Juergen Gross]

Add a new cpu notifier action CPU_RESUME_FAILED which is called for all
cpus which failed to come up at resume. The calls will be done after
all other cpus are already up.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpu.c      |  5 +++++
 xen/include/xen/cpu.h | 20 +++++++++++---------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index c436c0de7f..f3cf9463b4 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -214,7 +214,12 @@ void enable_nonboot_cpus(void)
             printk("Error bringing CPU%d up: %d\n", cpu, error);
             BUG_ON(error == -EBUSY);
         }
+        else
+            __cpumask_clear_cpu(cpu, &frozen_cpus);
     }
 
+    for_each_cpu ( cpu, &frozen_cpus )
+        BUG_ON(cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL));
+
     cpumask_clear(&frozen_cpus);
 }
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index 2fe3ec05d8..2fc0cb1bb5 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -32,23 +32,25 @@ void register_cpu_notifier(struct notifier_block *nb);
  *  (a) A CPU is going down; or (b) CPU_UP_CANCELED
  */
 /* CPU_UP_PREPARE: Preparing to bring CPU online. */
-#define CPU_UP_PREPARE   (0x0001 | NOTIFY_FORWARD)
+#define CPU_UP_PREPARE    (0x0001 | NOTIFY_FORWARD)
 /* CPU_UP_CANCELED: CPU is no longer being brought online. */
-#define CPU_UP_CANCELED  (0x0002 | NOTIFY_REVERSE)
+#define CPU_UP_CANCELED   (0x0002 | NOTIFY_REVERSE)
 /* CPU_STARTING: CPU nearly online. Runs on new CPU, irqs still disabled. */
-#define CPU_STARTING     (0x0003 | NOTIFY_FORWARD)
+#define CPU_STARTING      (0x0003 | NOTIFY_FORWARD)
 /* CPU_ONLINE: CPU is up. */
-#define CPU_ONLINE       (0x0004 | NOTIFY_FORWARD)
+#define CPU_ONLINE        (0x0004 | NOTIFY_FORWARD)
 /* CPU_DOWN_PREPARE: CPU is going down. */
-#define CPU_DOWN_PREPARE (0x0005 | NOTIFY_REVERSE)
+#define CPU_DOWN_PREPARE  (0x0005 | NOTIFY_REVERSE)
 /* CPU_DOWN_FAILED: CPU is no longer going down. */
-#define CPU_DOWN_FAILED  (0x0006 | NOTIFY_FORWARD)
+#define CPU_DOWN_FAILED   (0x0006 | NOTIFY_FORWARD)
 /* CPU_DYING: CPU is nearly dead (in stop_machine context). */
-#define CPU_DYING        (0x0007 | NOTIFY_REVERSE)
+#define CPU_DYING         (0x0007 | NOTIFY_REVERSE)
 /* CPU_DEAD: CPU is dead. */
-#define CPU_DEAD         (0x0008 | NOTIFY_REVERSE)
+#define CPU_DEAD          (0x0008 | NOTIFY_REVERSE)
 /* CPU_REMOVE: CPU was removed. */
-#define CPU_REMOVE       (0x0009 | NOTIFY_REVERSE)
+#define CPU_REMOVE        (0x0009 | NOTIFY_REVERSE)
+/* CPU_RESUME_FAILED: CPU failed to come up in resume, all other CPUs up. */
+#define CPU_RESUME_FAILED (0x000a | NOTIFY_REVERSE)
 
 /* Perform CPU hotplug. May return -EAGAIN. */
 int cpu_down(unsigned int cpu);
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

Instead of freeing percpu areas during suspend and allocating them
again when resuming keep them. Only free an area in case a cpu didn't
come up again when resuming.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/percpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c
index 8be4ebddf4..5ea14b6ec3 100644
--- a/xen/arch/x86/percpu.c
+++ b/xen/arch/x86/percpu.c
@@ -76,7 +76,8 @@ static int cpu_percpu_callback(
         break;
     case CPU_UP_CANCELED:
     case CPU_DEAD:
-        if ( !park_offline_cpus )
+    case CPU_RESUME_FAILED:
+        if ( !park_offline_cpus && system_state != SYS_STATE_suspend )
             free_percpu_area(cpu);
         break;
     case CPU_REMOVE:
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/6] xen/cpupool: simplify suspend/resume handling

[Juergen Gross]

Instead of removing cpus temporarily from cpupools during
suspend/resume only remove cpus finally which didn't come up when
resuming.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c       | 130 ++++++++++++++++++---------------------------
 xen/include/xen/sched-if.h |   1 -
 2 files changed, 51 insertions(+), 80 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index e89bb67e71..ed689fd290 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -47,12 +47,6 @@ static struct cpupool *alloc_cpupool_struct(void)
         xfree(c);
         c = NULL;
     }
-    else if ( !zalloc_cpumask_var(&c->cpu_suspended) )
-    {
-        free_cpumask_var(c->cpu_valid);
-        xfree(c);
-        c = NULL;
-    }
 
     return c;
 }
@@ -60,10 +54,7 @@ static struct cpupool *alloc_cpupool_struct(void)
 static void free_cpupool_struct(struct cpupool *c)
 {
     if ( c )
-    {
-        free_cpumask_var(c->cpu_suspended);
         free_cpumask_var(c->cpu_valid);
-    }
     xfree(c);
 }
 
@@ -477,10 +468,6 @@ void cpupool_rm_domain(struct domain *d)
 /*
  * 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)
 {
@@ -490,42 +477,15 @@ static int cpupool_cpu_add(unsigned int cpu)
     cpumask_clear_cpu(cpu, &cpupool_locked_cpus);
     cpumask_set_cpu(cpu, &cpupool_free_cpus);
 
-    if ( system_state == SYS_STATE_suspend || system_state == SYS_STATE_resume )
-    {
-        struct cpupool **c;
-
-        for_each_cpupool(c)
-        {
-            if ( cpumask_test_cpu(cpu, (*c)->cpu_suspended ) )
-            {
-                ret = cpupool_assign_cpu_locked(*c, cpu);
-                if ( ret )
-                    goto out;
-                cpumask_clear_cpu(cpu, (*c)->cpu_suspended);
-                break;
-            }
-        }
+    /*
+     * 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.
+     */
+    per_cpu(cpupool, cpu) = NULL;
+    ret = cpupool_assign_cpu_locked(cpupool0, cpu);
 
-        /*
-         * 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.
-         */
-        per_cpu(cpupool, cpu) = NULL;
-        ret = cpupool_assign_cpu_locked(cpupool0, cpu);
-    }
- out:
     spin_unlock(&cpupool_lock);
 
     return ret;
@@ -535,42 +495,14 @@ static int cpupool_cpu_add(unsigned int cpu)
  * 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 = -ENODEV;
 
     spin_lock(&cpupool_lock);
-    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);
-                cpumask_clear_cpu(cpu, (*c)->cpu_valid);
-                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 ( cpumask_test_cpu(cpu, cpupool0->cpu_valid) )
     {
         /*
          * If we are not suspending, we are hot-unplugging cpu, and that is
@@ -587,6 +519,41 @@ static int cpupool_cpu_remove(unsigned int cpu)
     return ret;
 }
 
+/*
+ * Called during resume for all cpus which didn't come up again. The cpu must
+ * be removed from the cpupool it is assigned to. In case a cpupool will be
+ * left without cpu we move all domains of that cpupool to cpupool0.
+ */
+static void cpupool_cpu_remove_forced(unsigned int cpu)
+{
+    struct cpupool **c;
+    struct domain *d;
+
+    spin_lock(&cpupool_lock);
+
+    if ( cpumask_test_cpu(cpu, &cpupool_free_cpus) )
+        cpumask_clear_cpu(cpu, &cpupool_free_cpus);
+    else
+    {
+        for_each_cpupool(c)
+        {
+            if ( cpumask_test_cpu(cpu, (*c)->cpu_valid) )
+            {
+                cpumask_clear_cpu(cpu, (*c)->cpu_valid);
+                if ( cpumask_weight((*c)->cpu_valid) == 0 )
+                {
+                    if ( *c == cpupool0 )
+                        panic("No cpu left in cpupool0\n");
+                    for_each_domain_in_cpupool(d, *c)
+                        cpupool_move_domain_locked(d, cpupool0);
+                }
+            }
+        }
+    }
+
+    spin_unlock(&cpupool_lock);
+}
+
 /*
  * do cpupool related sysctl operations
  */
@@ -774,10 +741,15 @@ static int cpu_callback(
     {
     case CPU_DOWN_FAILED:
     case CPU_ONLINE:
-        rc = cpupool_cpu_add(cpu);
+        if ( system_state <= SYS_STATE_active )
+            rc = cpupool_cpu_add(cpu);
         break;
     case CPU_DOWN_PREPARE:
-        rc = cpupool_cpu_remove(cpu);
+        if ( system_state <= SYS_STATE_active )
+            rc = cpupool_cpu_remove(cpu);
+        break;
+    case CPU_RESUME_FAILED:
+        cpupool_cpu_remove_forced(cpu);
         break;
     default:
         break;
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 9596eae1e2..92bc7a0365 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -214,7 +214,6 @@ struct cpupool
 {
     int              cpupool_id;
     cpumask_var_t    cpu_valid;      /* all cpus assigned to pool */
-    cpumask_var_t    cpu_suspended;  /* cpus in S3 that should be in this pool */
     struct cpupool   *next;
     unsigned int     n_dom;
     struct scheduler *sched;
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/6] xen/sched: don't disable scheduler on cpus during suspend

[Juergen Gross]

Today there is special handling in cpu_disable_scheduler() for suspend
by forcing all vcpus to the boot cpu. In fact there is no need for that
as during resume the vcpus are put on the correct cpus again.

So we can just omit the call of cpu_disable_scheduler() when offlining
a cpu due to suspend and on resuming we can omit taking the schedule
lock for selecting the new processor.

In restore_vcpu_affinity() we should be careful when applying affinity
as the cpu might not have come back to life. This in turn enables us
to even support affinity_broken across suspend/resume.

Avoid all other scheduler dealloc - alloc dance when doing suspend and
resume, too. It is enough to react on cpus failing to come up on resume
again.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/schedule.c | 161 ++++++++++++++++----------------------------------
 1 file changed, 52 insertions(+), 109 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 665747f247..8a8598c7ad 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -560,33 +560,6 @@ static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu)
         v->processor = new_cpu;
 }
 
-/*
- * Move a vcpu from its current processor to a target new processor,
- * without asking the scheduler to do any placement. This is intended
- * for being called from special contexts, where things are quiet
- * enough that no contention is supposed to happen (i.e., during
- * shutdown or software suspend, like ACPI S3).
- */
-static void vcpu_move_nosched(struct vcpu *v, unsigned int new_cpu)
-{
-    unsigned long flags;
-    spinlock_t *lock, *new_lock;
-
-    ASSERT(system_state == SYS_STATE_suspend);
-    ASSERT(!vcpu_runnable(v) && (atomic_read(&v->pause_count) ||
-                                 atomic_read(&v->domain->pause_count)));
-
-    lock = per_cpu(schedule_data, v->processor).schedule_lock;
-    new_lock = per_cpu(schedule_data, new_cpu).schedule_lock;
-
-    sched_spin_lock_double(lock, new_lock, &flags);
-    ASSERT(new_cpu != v->processor);
-    vcpu_move_locked(v, new_cpu);
-    sched_spin_unlock_double(lock, new_lock, flags);
-
-    sched_move_irqs(v);
-}
-
 /*
  * Initiating migration
  *
@@ -735,31 +708,36 @@ void restore_vcpu_affinity(struct domain *d)
 
         ASSERT(!vcpu_runnable(v));
 
-        lock = vcpu_schedule_lock_irq(v);
-
-        if ( v->affinity_broken )
-        {
-            sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
-            v->affinity_broken = 0;
-
-        }
-
         /*
-         * During suspend (in cpu_disable_scheduler()), we moved every vCPU
-         * to BSP (which, as of now, is pCPU 0), as a temporary measure to
-         * allow the nonboot processors to have their data structure freed
-         * and go to sleep. But nothing guardantees that the BSP is a valid
-         * pCPU for a particular domain.
+         * Re-assign the initial processor as after resume we have no
+         * guarantee the old processor has come back to life again.
          *
          * Therefore, here, before actually unpausing the domains, we should
          * set v->processor of each of their vCPUs to something that will
          * make sense for the scheduler of the cpupool in which they are in.
          */
         cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
-                    cpupool_domain_cpumask(v->domain));
-        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
+                    cpupool_domain_cpumask(d));
+        if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
+        {
+            if ( v->affinity_broken )
+            {
+                sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
+                v->affinity_broken = 0;
+                cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
+                            cpupool_domain_cpumask(d));
+            }
 
-        spin_unlock_irq(lock);
+            if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
+            {
+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
+                sched_set_affinity(v, &cpumask_all, NULL);
+                cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
+                            cpupool_domain_cpumask(d));
+            }
+        }
+
+        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
 
         lock = vcpu_schedule_lock_irq(v);
         v->processor = SCHED_OP(vcpu_scheduler(v), pick_cpu, v);
@@ -783,7 +761,6 @@ int cpu_disable_scheduler(unsigned int cpu)
     struct vcpu *v;
     struct cpupool *c;
     cpumask_t online_affinity;
-    unsigned int new_cpu;
     int ret = 0;
 
     c = per_cpu(cpupool, cpu);
@@ -809,14 +786,7 @@ int cpu_disable_scheduler(unsigned int cpu)
                     break;
                 }
 
-                if (system_state == SYS_STATE_suspend)
-                {
-                    cpumask_copy(v->cpu_hard_affinity_saved,
-                                 v->cpu_hard_affinity);
-                    v->affinity_broken = 1;
-                }
-                else
-                    printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
 
                 sched_set_affinity(v, &cpumask_all, NULL);
             }
@@ -828,60 +798,26 @@ int cpu_disable_scheduler(unsigned int cpu)
                 continue;
             }
 
-            /* If it is on this cpu, we must send it away. */
-            if ( unlikely(system_state == SYS_STATE_suspend) )
-            {
-                vcpu_schedule_unlock_irqrestore(lock, flags, v);
-
-                /*
-                 * If we are doing a shutdown/suspend, it is not necessary to
-                 * ask the scheduler to chime in. In fact:
-                 *  * there is no reason for it: the end result we are after
-                 *    is just 'all the vcpus on the boot pcpu, and no vcpu
-                 *    anywhere else', so let's just go for it;
-                 *  * it's wrong, for cpupools with only non-boot pcpus, as
-                 *    the scheduler would always fail to send the vcpus away
-                 *    from the last online (non boot) pcpu!
-                 *
-                 * Therefore, in the shutdown/suspend case, we just pick up
-                 * one (still) online pcpu. Note that, at this stage, all
-                 * domains (including dom0) have been paused already, so we
-                 * do not expect any vcpu activity at all.
-                 */
-                cpumask_andnot(&online_affinity, &cpu_online_map,
-                               cpumask_of(cpu));
-                BUG_ON(cpumask_empty(&online_affinity));
-                /*
-                 * As boot cpu is, usually, pcpu #0, using cpumask_first()
-                 * will make us converge quicker.
-                 */
-                new_cpu = cpumask_first(&online_affinity);
-                vcpu_move_nosched(v, new_cpu);
-            }
-            else
-            {
-                /*
-                 * OTOH, if the system is still live, and we are here because
-                 * we are doing some cpupool manipulations:
-                 *  * we want to call the scheduler, and let it re-evaluation
-                 *    the placement of the vcpu, taking into account the new
-                 *    cpupool configuration;
-                 *  * the scheduler will always fine a suitable solution, or
-                 *    things would have failed before getting in here.
-                 */
-                vcpu_migrate_start(v);
-                vcpu_schedule_unlock_irqrestore(lock, flags, v);
+            /* If it is on this cpu, we must send it away.
+             * We are doing some cpupool manipulations:
+             *  * we want to call the scheduler, and let it re-evaluation
+             *    the placement of the vcpu, taking into account the new
+             *    cpupool configuration;
+             *  * the scheduler will always find a suitable solution, or
+             *    things would have failed before getting in here.
+             */
+            vcpu_migrate_start(v);
+            vcpu_schedule_unlock_irqrestore(lock, flags, v);
 
-                vcpu_migrate_finish(v);
+            vcpu_migrate_finish(v);
 
-                /*
-                 * The only caveat, in this case, is that if a vcpu active in
-                 * the hypervisor isn't migratable. In this case, the caller
-                 * should try again after releasing and reaquiring all locks.
-                 */
-                if ( v->processor == cpu )
-                    ret = -EAGAIN;
-            }
+            /*
+             * The only caveat, in this case, is that if a vcpu active in
+             * the hypervisor isn't migratable. In this case, the caller
+             * should try again after releasing and reaquiring all locks.
+             */
+            if ( v->processor == cpu )
+                ret = -EAGAIN;
         }
     }
 
@@ -1727,20 +1663,27 @@ static int cpu_schedule_callback(
     switch ( action )
     {
     case CPU_STARTING:
-        SCHED_OP(sched, init_pdata, sd->sched_priv, cpu);
+        if ( system_state != SYS_STATE_resume )
+            SCHED_OP(sched, init_pdata, sd->sched_priv, cpu);
         break;
     case CPU_UP_PREPARE:
-        rc = cpu_schedule_up(cpu);
+        if ( system_state != SYS_STATE_resume )
+            rc = cpu_schedule_up(cpu);
         break;
+    case CPU_RESUME_FAILED:
     case CPU_DEAD:
+        if ( system_state == SYS_STATE_suspend )
+            break;
         rcu_read_lock(&domlist_read_lock);
         cpu_disable_scheduler(cpu);
         rcu_read_unlock(&domlist_read_lock);
         SCHED_OP(sched, deinit_pdata, sd->sched_priv, cpu);
-        /* Fallthrough */
-    case CPU_UP_CANCELED:
         cpu_schedule_down(cpu);
         break;
+    case CPU_UP_CANCELED:
+        if ( system_state != SYS_STATE_resume )
+            cpu_schedule_down(cpu);
+        break;
     default:
         break;
     }
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
> 
> This avoids coding the same pattern multiple times.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> --- a/xen/include/xen/cpu.h
> +++ b/xen/include/xen/cpu.h
> @@ -32,23 +32,25 @@ void register_cpu_notifier(struct notifier_block
> *nb);
>   *  (a) A CPU is going down; or (b) CPU_UP_CANCELED
>   */
>  /* CPU_UP_PREPARE: Preparing to bring CPU online. */
> -#define CPU_UP_PREPARE   (0x0001 | NOTIFY_FORWARD)
> +#define CPU_UP_PREPARE    (0x0001 | NOTIFY_FORWARD)
>
In the comment block before these definitions, there's this:

 * Possible event sequences for a given CPU:
 *  CPU_UP_PREPARE -> CPU_UP_CANCELLED           -- failed CPU up
 *  CPU_UP_PREPARE -> CPU_STARTING -> CPU_ONLINE -- successful CPU up
 *  CPU_DOWN_PREPARE -> CPU_DOWN_FAILED          -- failed CPU down
 *  CPU_DOWN_PREPARE -> CPU_DYING -> CPU_DEAD    -- successful CPU down

Shouldn't we add a line for this new hook? Something, IIUIC, like:

 CPU_UP_PREPARE -> CPU_UP_CANCELLED -> CPU_RESUME_FAILED --CPU not resuming

With this, FWIW,

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

One more (minor) thing...

>  /* CPU_REMOVE: CPU was removed. */
> -#define CPU_REMOVE       (0x0009 | NOTIFY_REVERSE)
> +#define CPU_REMOVE        (0x0009 | NOTIFY_REVERSE)
> +/* CPU_RESUME_FAILED: CPU failed to come up in resume, all other CPUs up. */
> +#define CPU_RESUME_FAILED (0x000a | NOTIFY_REVERSE)
>  
... technically, when we're dealing with CPU_RESUME_FAILED on one CPU,
we don't know if _all_ others really went up, so I think I'd remove
what follows the ','.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Juergen Gross]

On 25/03/2019 13:21, Dario Faggioli wrote:
> On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
>> --- a/xen/include/xen/cpu.h
>> +++ b/xen/include/xen/cpu.h
>> @@ -32,23 +32,25 @@ void register_cpu_notifier(struct notifier_block
>> *nb);
>>   *  (a) A CPU is going down; or (b) CPU_UP_CANCELED
>>   */
>>  /* CPU_UP_PREPARE: Preparing to bring CPU online. */
>> -#define CPU_UP_PREPARE   (0x0001 | NOTIFY_FORWARD)
>> +#define CPU_UP_PREPARE    (0x0001 | NOTIFY_FORWARD)
>>
> In the comment block before these definitions, there's this:
> 
>  * Possible event sequences for a given CPU:
>  *  CPU_UP_PREPARE -> CPU_UP_CANCELLED           -- failed CPU up
>  *  CPU_UP_PREPARE -> CPU_STARTING -> CPU_ONLINE -- successful CPU up
>  *  CPU_DOWN_PREPARE -> CPU_DOWN_FAILED          -- failed CPU down
>  *  CPU_DOWN_PREPARE -> CPU_DYING -> CPU_DEAD    -- successful CPU down
> 
> Shouldn't we add a line for this new hook? Something, IIUIC, like:
> 
>  CPU_UP_PREPARE -> CPU_UP_CANCELLED -> CPU_RESUME_FAILED --CPU not resuming

Fine with me.

> 
> With this, FWIW,
> 
> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> 
> One more (minor) thing...
> 
>>  /* CPU_REMOVE: CPU was removed. */
>> -#define CPU_REMOVE       (0x0009 | NOTIFY_REVERSE)
>> +#define CPU_REMOVE        (0x0009 | NOTIFY_REVERSE)
>> +/* CPU_RESUME_FAILED: CPU failed to come up in resume, all other CPUs up. */
>> +#define CPU_RESUME_FAILED (0x000a | NOTIFY_REVERSE)
>>  
> ... technically, when we're dealing with CPU_RESUME_FAILED on one CPU,
> we don't know if _all_ others really went up, so I think I'd remove
> what follows the ','.

The point is that for the CPU_RESUME_FAILED case we can be sure that no
cpu will come up due to resume just a little bit later. So we can test
for e.g. a cpupool suddenly having no more cpus available. This is in
contrast to CPU_UP_CANCELLED being signalled just after the one cpu
failing to come up, but before the next cpu is triggered to come up.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> Instead of freeing percpu areas during suspend and allocating them
> again when resuming keep them. Only free an area in case a cpu didn't
> come up again when resuming.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

[George Dunlap]

On 3/18/19 1:11 PM, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
> 
> This avoids coding the same pattern multiple times.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Andrew Cooper]

On 18/03/2019 13:11, Juergen Gross wrote:
> cpu_disable_scheduler() is being called from __cpu_disable() today.
> There is no need to call it on the cpu just being disabled, so use
> the CPU_DEAD case of the cpu notifier chain.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/arch/x86/smpboot.c |  3 ---
>  xen/common/schedule.c  | 12 +++++-------
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 7d1226d7bc..b7a0a4a419 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -1221,9 +1221,6 @@ void __cpu_disable(void)
>      cpumask_clear_cpu(cpu, &cpu_online_map);
>      fixup_irqs(&cpu_online_map, 1);
>      fixup_eoi();
> -
> -    if ( cpu_disable_scheduler(cpu) )
> -        BUG();
>  }

It looks like ARM needs an equivalent adjustment.

>  
>  void __cpu_die(unsigned int cpu)
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 60755a631e..665747f247 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -773,8 +773,9 @@ void restore_vcpu_affinity(struct domain *d)
>  }
>  
>  /*
> - * This function is used by cpu_hotplug code from stop_machine context
> + * This function is used by cpu_hotplug code via cpu notifier chain
>   * and from cpupools to switch schedulers on a cpu.
> + * Caller must get domlist_read_lock.

s/get/hold/ ?

With at least the ARM side adjusted, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

>   */
>  int cpu_disable_scheduler(unsigned int cpu)
>  {
> @@ -789,12 +790,6 @@ int cpu_disable_scheduler(unsigned int cpu)
>      if ( c == NULL )
>          return ret;
>  
> -    /*
> -     * We'd need the domain RCU lock, but:
> -     *  - when we are called from cpupool code, it's acquired there already;
> -     *  - when we are called for CPU teardown, we're in stop-machine context,
> -     *    so that's not be a problem.
> -     */
>      for_each_domain_in_cpupool ( d, c )
>      {
>          for_each_vcpu ( d, v )
> @@ -1738,6 +1733,9 @@ static int cpu_schedule_callback(
>          rc = cpu_schedule_up(cpu);
>          break;
>      case CPU_DEAD:
> +        rcu_read_lock(&domlist_read_lock);
> +        cpu_disable_scheduler(cpu);
> +        rcu_read_unlock(&domlist_read_lock);
>          SCHED_OP(sched, deinit_pdata, sd->sched_priv, cpu);
>          /* Fallthrough */
>      case CPU_UP_CANCELED:


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[George Dunlap]

On 3/18/19 1:11 PM, Juergen Gross wrote:
> cpu_disable_scheduler() is being called from __cpu_disable() today.
> There is no need to call it on the cpu just being disabled, so use
> the CPU_DEAD case of the cpu notifier chain.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Scheduler change:

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

[Andrew Cooper]

On 18/03/2019 13:11, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
>
> This avoids coding the same pattern multiple times.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 836c62f97f..c436c0de7f 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -71,11 +71,18 @@ void __init register_cpu_notifier(struct notifier_block *nb)
>      spin_unlock(&cpu_add_remove_lock);
>  }
>  
> +static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
> +                                   struct notifier_block **nb)
> +{
> +    void *hcpu = (void *)(long)cpu;
> +    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
> +
> +    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
> +}
> +
>  static void _take_cpu_down(void *unused)
>  {
> -    void *hcpu = (void *)(long)smp_processor_id();
> -    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
> -    BUG_ON(notifier_rc != NOTIFY_DONE);
> +    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));

Where possible, we're trying to remove side effects from macros.

Could I please talk you into writing this as:

int rc = cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL);

BUG_ON(rc);

An alternative might be to have a:

static void cpu_notifier_call_chain_nofail(...)

wrapper as this seems to be a common pattern.  (Ideally longterm, it
might be better to pass the nofail intention into the notifier chain
itself so we can identify which callback had a problem, but thats
definitely not something for here.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[George Dunlap]

On 3/18/19 1:11 PM, Juergen Gross wrote:
> Add a new cpu notifier action CPU_RESUME_FAILED which is called for all
> cpus which failed to come up at resume. The calls will be done after
> all other cpus are already up.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Dario Faggioli]

On Mon, 2019-03-25 at 13:29 +0100, Juergen Gross wrote:
> On 25/03/2019 13:21, Dario Faggioli wrote:
>
> > Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> > 
> > One more (minor) thing...
> > 
> > >  /* CPU_REMOVE: CPU was removed. */
> > > -#define CPU_REMOVE       (0x0009 | NOTIFY_REVERSE)
> > > +#define CPU_REMOVE        (0x0009 | NOTIFY_REVERSE)
> > > +/* CPU_RESUME_FAILED: CPU failed to come up in resume, all other
> > > CPUs up. */
> > > +#define CPU_RESUME_FAILED (0x000a | NOTIFY_REVERSE)
> > >  
> > ... technically, when we're dealing with CPU_RESUME_FAILED on one
> > CPU,
> > we don't know if _all_ others really went up, so I think I'd remove
> > what follows the ','.
> 
> The point is that for the CPU_RESUME_FAILED case we can be sure that
> no
> cpu will come up due to resume just a little bit later.
>
Ah, I see what you mean... that's the fact that this notifier is
invoked from another loop, and although we don't know which CPU did
manage to come up and which don't, we do know that all the ones that
could come up, are up already.

>  So we can test
> for e.g. a cpupool suddenly having no more cpus available. This is in
> contrast to CPU_UP_CANCELLED being signalled just after the one cpu
> failing to come up, but before the next cpu is triggered to come up.
> 
Right.

Well, it still looks to me that "all other CPUs up" is not entirely
accurate. But I can't propose a better alternative (unless we write
something very long, which is probably not worth it).

Perhaps you can explain this a little in another comment, like in
enable_nonboot_cpus(), before the for_each_cpu() loop itself.

But I don't feel too strong about that, and the RoB stands, whatever
you decide to do.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Andrew Cooper]

On 18/03/2019 13:11, Juergen Gross wrote:
> Instead of freeing percpu areas during suspend and allocating them
> again when resuming keep them. Only free an area in case a cpu didn't
> come up again when resuming.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Hmm - this is slightly problematic, given the dual nature of this code.

I agree that it this change is beneficial for the suspend case, but it
is a problem when we are parking an individual CPU for smt=0 or
xen-hptool reasons.

Do we have any hint we can use when taking the CPU down as to whether
we're expecting it to come straight back up again?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen/cpupool: simplify suspend/resume handling

[George Dunlap]

On 3/18/19 1:11 PM, Juergen Gross wrote:
> Instead of removing cpus temporarily from cpupools during
> suspend/resume only remove cpus finally which didn't come up when
> resuming.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Looks good overall -- one comment...

> @@ -774,10 +741,15 @@ static int cpu_callback(
>      {
>      case CPU_DOWN_FAILED:
>      case CPU_ONLINE:
> -        rc = cpupool_cpu_add(cpu);
> +        if ( system_state <= SYS_STATE_active )
> +            rc = cpupool_cpu_add(cpu);
>          break;
>      case CPU_DOWN_PREPARE:
> -        rc = cpupool_cpu_remove(cpu);
> +        if ( system_state <= SYS_STATE_active )
> +            rc = cpupool_cpu_remove(cpu);
> +        break;
> +    case CPU_RESUME_FAILED:
> +        cpupool_cpu_remove_forced(cpu);
>          break;
>      default:

It would be good to have some comments here just explaining this; maybe
just to the effect of, "Suspend/resume operations don't affect cpupool
placement".

With that:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

[Juergen Gross]

On 27/03/2019 16:39, Andrew Cooper wrote:
> On 18/03/2019 13:11, Juergen Gross wrote:
>> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
>> for a cpu with a specified action, returning an errno value.
>>
>> This avoids coding the same pattern multiple times.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
>>  1 file changed, 21 insertions(+), 29 deletions(-)
>>
>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>> index 836c62f97f..c436c0de7f 100644
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -71,11 +71,18 @@ void __init register_cpu_notifier(struct notifier_block *nb)
>>      spin_unlock(&cpu_add_remove_lock);
>>  }
>>  
>> +static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
>> +                                   struct notifier_block **nb)
>> +{
>> +    void *hcpu = (void *)(long)cpu;
>> +    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
>> +
>> +    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
>> +}
>> +
>>  static void _take_cpu_down(void *unused)
>>  {
>> -    void *hcpu = (void *)(long)smp_processor_id();
>> -    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
>> -    BUG_ON(notifier_rc != NOTIFY_DONE);
>> +    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));
> 
> Where possible, we're trying to remove side effects from macros.
> 
> Could I please talk you into writing this as:
> 
> int rc = cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL);
> 
> BUG_ON(rc);

Fine with me.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 27/03/2019 16:55, Andrew Cooper wrote:
> On 18/03/2019 13:11, Juergen Gross wrote:
>> Instead of freeing percpu areas during suspend and allocating them
>> again when resuming keep them. Only free an area in case a cpu didn't
>> come up again when resuming.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Hmm - this is slightly problematic, given the dual nature of this code.
> 
> I agree that it this change is beneficial for the suspend case, but it
> is a problem when we are parking an individual CPU for smt=0 or
> xen-hptool reasons.
> 
> Do we have any hint we can use when taking the CPU down as to whether
> we're expecting it to come straight back up again?

Did you look into the patch? I did this by testing system_state.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Jan Beulich]

>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
> @@ -1738,6 +1733,9 @@ static int cpu_schedule_callback(
>          rc = cpu_schedule_up(cpu);
>          break;
>      case CPU_DEAD:
> +        rcu_read_lock(&domlist_read_lock);
> +        cpu_disable_scheduler(cpu);
> +        rcu_read_unlock(&domlist_read_lock);
>          SCHED_OP(sched, deinit_pdata, sd->sched_priv, cpu);
>          /* Fallthrough */
>      case CPU_UP_CANCELED:

cpu_disable_scheduler() has a return value (and hence means to
fail) - is ignoring this here really appropriate?

Also while indeed (as the description says) there's no need to
run the function on the CPU itself, it's not obvious to me that
it's safe to run it outside of stop_machine() context. Or to be
more precise, it's not clear to me that leaving stop_machine()
context with the adjustments not done yet is not going to
lead to problems (due to the gap between leaving that context
and acquiring the RCU lock). Could you clarify this in the
description, please (if it indeed is fine this way)?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> cpu_disable_scheduler() is being called from __cpu_disable() today.
> There is no need to call it on the cpu just being disabled, so use
> the CPU_DEAD case of the cpu notifier chain.
> 
So, what do you mean with "There is no need to call it on the cpu just
being disabled"?

Because we still (even after this patch, I mean) call
cpu_disable_scheduler() on all non-boot CPUs, aren't we? It's just that
right now we call it from __cpu_disable(), with the patch we call it
slightly later.

And another difference looks to me to be that right now we call
cpu_disable_scheduler() from stop-machine context, which I think is no
longer true with this patch. Perhaps the changelog could tell why that
is ok?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Jan Beulich]

>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -214,7 +214,12 @@ void enable_nonboot_cpus(void)
>              printk("Error bringing CPU%d up: %d\n", cpu, error);
>              BUG_ON(error == -EBUSY);
>          }
> +        else
> +            __cpumask_clear_cpu(cpu, &frozen_cpus);
>      }
>  
> +    for_each_cpu ( cpu, &frozen_cpus )
> +        BUG_ON(cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL));
> +
>      cpumask_clear(&frozen_cpus);
>  }

Is there a particular reason you add a second loop here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Juergen Gross]

On 27/03/2019 17:24, Dario Faggioli wrote:
> On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
>> cpu_disable_scheduler() is being called from __cpu_disable() today.
>> There is no need to call it on the cpu just being disabled, so use
>> the CPU_DEAD case of the cpu notifier chain.
>>
> So, what do you mean with "There is no need to call it on the cpu just
> being disabled"?
> 
> Because we still (even after this patch, I mean) call
> cpu_disable_scheduler() on all non-boot CPUs, aren't we? It's just that
> right now we call it from __cpu_disable(), with the patch we call it
> slightly later.

The CPU_DEAD notifier chain is called on the CPU requesting the other
one to go down (so on the boot CPU in suspend case). So we call it _for_
all non-boot CPUs in the boot CPU.

> And another difference looks to me to be that right now we call
> cpu_disable_scheduler() from stop-machine context, which I think is no
> longer true with this patch. Perhaps the changelog could tell why that
> is ok?

Okay, I'll add something.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen/cpupool: simplify suspend/resume handling

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> Instead of removing cpus temporarily from cpupools during
> suspend/resume only remove cpus finally which didn't come up when
> resuming.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/cpupool.c       | 130 ++++++++++++++++++-----------------
> ----------
>  xen/include/xen/sched-if.h |   1 -
>  2 files changed, 51 insertions(+), 80 deletions(-)
>
Cool diffstat! :-)

And I'm particularly happy to see 'c->cpu_suspended' go away. ;-D

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: add new cpu notifier action CPU_RESUME_FAILED

[Juergen Gross]

On 27/03/2019 17:29, Jan Beulich wrote:
>>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -214,7 +214,12 @@ void enable_nonboot_cpus(void)
>>              printk("Error bringing CPU%d up: %d\n", cpu, error);
>>              BUG_ON(error == -EBUSY);
>>          }
>> +        else
>> +            __cpumask_clear_cpu(cpu, &frozen_cpus);
>>      }
>>  
>> +    for_each_cpu ( cpu, &frozen_cpus )
>> +        BUG_ON(cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL));
>> +
>>      cpumask_clear(&frozen_cpus);
>>  }
> 
> Is there a particular reason you add a second loop here?

Yes, I want to know which cpus did come up again.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
> On 27/03/2019 16:55, Andrew Cooper wrote:
>> On 18/03/2019 13:11, Juergen Gross wrote:
>>> Instead of freeing percpu areas during suspend and allocating them
>>> again when resuming keep them. Only free an area in case a cpu didn't
>>> come up again when resuming.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> 
>> Hmm - this is slightly problematic, given the dual nature of this code.
>> 
>> I agree that it this change is beneficial for the suspend case, but it
>> is a problem when we are parking an individual CPU for smt=0 or
>> xen-hptool reasons.
>> 
>> Do we have any hint we can use when taking the CPU down as to whether
>> we're expecting it to come straight back up again?
> 
> Did you look into the patch? I did this by testing system_state.

I think there's a wider problem here: enable_nonboot_cpus()
only brings back up the CPUs that were previously online.
Parked ones would be left alone, yet after resume they'd
need to be put back into parked state.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Juergen Gross]

On 27/03/2019 17:22, Jan Beulich wrote:
>>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
>> @@ -1738,6 +1733,9 @@ static int cpu_schedule_callback(
>>          rc = cpu_schedule_up(cpu);
>>          break;
>>      case CPU_DEAD:
>> +        rcu_read_lock(&domlist_read_lock);
>> +        cpu_disable_scheduler(cpu);
>> +        rcu_read_unlock(&domlist_read_lock);
>>          SCHED_OP(sched, deinit_pdata, sd->sched_priv, cpu);
>>          /* Fallthrough */
>>      case CPU_UP_CANCELED:
> 
> cpu_disable_scheduler() has a return value (and hence means to
> fail) - is ignoring this here really appropriate?

You are right, I should handle those cases in cpu_disable_scheduler().
Without the complete series committed this will result in a case not
handled correctly: dom0 trying to pin a vcpu to a physical cpu other
than cpu 0 via SCHEDOP_pin_override and suspending in that state. I'm
not aware of any dom0 trying to do that.

> Also while indeed (as the description says) there's no need to
> run the function on the CPU itself, it's not obvious to me that
> it's safe to run it outside of stop_machine() context. Or to be
> more precise, it's not clear to me that leaving stop_machine()
> context with the adjustments not done yet is not going to
> lead to problems (due to the gap between leaving that context
> and acquiring the RCU lock). Could you clarify this in the
> description, please (if it indeed is fine this way)?

It is fine, as the chances are zero that any code will run on the cpu
just taken down and that cpu is not holding any locks we might need.

I'll add that to the commit message.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Dario Faggioli]

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

On Wed, 2019-03-27 at 17:31 +0100, Juergen Gross wrote:
> On 27/03/2019 17:24, Dario Faggioli wrote:
> > On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> > > cpu_disable_scheduler() is being called from __cpu_disable()
> > > today.
> > > There is no need to call it on the cpu just being disabled, so
> > > use
> > > the CPU_DEAD case of the cpu notifier chain.
> > > 
> > So, what do you mean with "There is no need to call it on the cpu
> > just
> > being disabled"?
> > 
> > Because we still (even after this patch, I mean) call
> > cpu_disable_scheduler() on all non-boot CPUs, aren't we? It's just
> > that
> > right now we call it from __cpu_disable(), with the patch we call
> > it
> > slightly later.
> 
> The CPU_DEAD notifier chain is called on the CPU requesting the other
> one to go down (so on the boot CPU in suspend case). So we call it
> _for_
> all non-boot CPUs in the boot CPU.
> 
Mmm... ok, I see what you mean now.

I guess part of "the problem" is that "call func on cpu A" reads, at
least to me, as both 1) call func so that it acts on and change the
state of cpu A, and 2) call func in such a way that it executes on cpu
A.

But I'm no native speaker, so it may very well be that the confusion is
all and only mine.

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 27/03/2019 17:38, Jan Beulich wrote:
>>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
>> On 27/03/2019 16:55, Andrew Cooper wrote:
>>> On 18/03/2019 13:11, Juergen Gross wrote:
>>>> Instead of freeing percpu areas during suspend and allocating them
>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>> come up again when resuming.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Hmm - this is slightly problematic, given the dual nature of this code.
>>>
>>> I agree that it this change is beneficial for the suspend case, but it
>>> is a problem when we are parking an individual CPU for smt=0 or
>>> xen-hptool reasons.
>>>
>>> Do we have any hint we can use when taking the CPU down as to whether
>>> we're expecting it to come straight back up again?
>>
>> Did you look into the patch? I did this by testing system_state.
> 
> I think there's a wider problem here: enable_nonboot_cpus()
> only brings back up the CPUs that were previously online.
> Parked ones would be left alone, yet after resume they'd
> need to be put back into parked state.

I can add that handling in the respin of the series.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Juergen Gross]

On 27/03/2019 17:51, Dario Faggioli wrote:
> On Wed, 2019-03-27 at 17:31 +0100, Juergen Gross wrote:
>> On 27/03/2019 17:24, Dario Faggioli wrote:
>>> On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
>>>> cpu_disable_scheduler() is being called from __cpu_disable()
>>>> today.
>>>> There is no need to call it on the cpu just being disabled, so
>>>> use
>>>> the CPU_DEAD case of the cpu notifier chain.
>>>>
>>> So, what do you mean with "There is no need to call it on the cpu
>>> just
>>> being disabled"?
>>>
>>> Because we still (even after this patch, I mean) call
>>> cpu_disable_scheduler() on all non-boot CPUs, aren't we? It's just
>>> that
>>> right now we call it from __cpu_disable(), with the patch we call
>>> it
>>> slightly later.
>>
>> The CPU_DEAD notifier chain is called on the CPU requesting the other
>> one to go down (so on the boot CPU in suspend case). So we call it
>> _for_
>> all non-boot CPUs in the boot CPU.
>>
> Mmm... ok, I see what you mean now.
> 
> I guess part of "the problem" is that "call func on cpu A" reads, at
> least to me, as both 1) call func so that it acts on and change the
> state of cpu A, and 2) call func in such a way that it executes on cpu
> A.

I'll rephrase to "execute func on cpu...".


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Jan Beulich]

>>> On 27.03.19 at 17:45, <jgross@suse.com> wrote:
> On 27/03/2019 17:22, Jan Beulich wrote:
>> Also while indeed (as the description says) there's no need to
>> run the function on the CPU itself, it's not obvious to me that
>> it's safe to run it outside of stop_machine() context. Or to be
>> more precise, it's not clear to me that leaving stop_machine()
>> context with the adjustments not done yet is not going to
>> lead to problems (due to the gap between leaving that context
>> and acquiring the RCU lock). Could you clarify this in the
>> description, please (if it indeed is fine this way)?
> 
> It is fine, as the chances are zero that any code will run on the cpu
> just taken down and that cpu is not holding any locks we might need.

Well, of course nothing's going to run on that CPU anymore.
But vCPU-s may still have associations with it, so what I'm
worried about is e.g. something finding v->processor pointing
at an offline CPU and getting confused. Another, more exotic
(or should I say contrived) scenario might be a soft-online
request coming very quickly after a prior soft-offline one, with
this function not having got around to run yet. Or basically
anything else that accesses the same state the function
means to update (or use).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen/sched: don't disable scheduler on cpus during suspend

[Dario Faggioli]

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

On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> Today there is special handling in cpu_disable_scheduler() for
> suspend
> by forcing all vcpus to the boot cpu. In fact there is no need for
> that
> as during resume the vcpus are put on the correct cpus again.
> 
> So we can just omit the call of cpu_disable_scheduler() when
> offlining
> a cpu due to suspend and on resuming we can omit taking the schedule
> lock for selecting the new processor.
> 
> In restore_vcpu_affinity() we should be careful when applying
> affinity
> as the cpu might not have come back to life. This in turn enables us
> to even support affinity_broken across suspend/resume.
> 
> Avoid all other scheduler dealloc - alloc dance when doing suspend
> and
> resume, too. It is enough to react on cpus failing to come up on
> resume
> again.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Ok, I think this patch is fine.

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

I guess you have tested both cpu off/on-lining and suspend/resume, or
do you need help with that? (One of my testboxes that I have here,
should be able to do suspend)

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen/sched: don't disable scheduler on cpus during suspend

[Juergen Gross]

On 28/03/2019 00:10, Dario Faggioli wrote:
> On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
>> Today there is special handling in cpu_disable_scheduler() for
>> suspend
>> by forcing all vcpus to the boot cpu. In fact there is no need for
>> that
>> as during resume the vcpus are put on the correct cpus again.
>>
>> So we can just omit the call of cpu_disable_scheduler() when
>> offlining
>> a cpu due to suspend and on resuming we can omit taking the schedule
>> lock for selecting the new processor.
>>
>> In restore_vcpu_affinity() we should be careful when applying
>> affinity
>> as the cpu might not have come back to life. This in turn enables us
>> to even support affinity_broken across suspend/resume.
>>
>> Avoid all other scheduler dealloc - alloc dance when doing suspend
>> and
>> resume, too. It is enough to react on cpus failing to come up on
>> resume
>> again.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> Ok, I think this patch is fine.
> 
> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> 
> I guess you have tested both cpu off/on-lining and suspend/resume, or
> do you need help with that? (One of my testboxes that I have here,
> should be able to do suspend)

I've tested suspend/resume.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 27/03/2019 17:52, Juergen Gross wrote:
> On 27/03/2019 17:38, Jan Beulich wrote:
>>>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
>>> On 27/03/2019 16:55, Andrew Cooper wrote:
>>>> On 18/03/2019 13:11, Juergen Gross wrote:
>>>>> Instead of freeing percpu areas during suspend and allocating them
>>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>>> come up again when resuming.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> Hmm - this is slightly problematic, given the dual nature of this code.
>>>>
>>>> I agree that it this change is beneficial for the suspend case, but it
>>>> is a problem when we are parking an individual CPU for smt=0 or
>>>> xen-hptool reasons.
>>>>
>>>> Do we have any hint we can use when taking the CPU down as to whether
>>>> we're expecting it to come straight back up again?
>>>
>>> Did you look into the patch? I did this by testing system_state.
>>
>> I think there's a wider problem here: enable_nonboot_cpus()
>> only brings back up the CPUs that were previously online.
>> Parked ones would be left alone, yet after resume they'd
>> need to be put back into parked state.
> 
> I can add that handling in the respin of the series.

Looking deeper into that mess I believe that should be a series of its
own. Cpu parking needs to be handled for cpu hotplug and core parking
(XENPF_core_parking), too.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
> Instead of freeing percpu areas during suspend and allocating them
> again when resuming keep them. Only free an area in case a cpu didn't
> come up again when resuming.

There's another aspect here which needs at least mentioning:
Right now, CPUs coming back up have their per-CPU data
cleared. Not doing so may indeed be an improvement in
some cases (like statistics collected), but may also get in the
way in other cases.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 28/03/2019 08:46, Jan Beulich wrote:
>>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
>> Instead of freeing percpu areas during suspend and allocating them
>> again when resuming keep them. Only free an area in case a cpu didn't
>> come up again when resuming.
> 
> There's another aspect here which needs at least mentioning:
> Right now, CPUs coming back up have their per-CPU data
> cleared. Not doing so may indeed be an improvement in
> some cases (like statistics collected), but may also get in the
> way in other cases.

I have checked the respective cpu notifier hooks and I think this is no
problem. I hope I didn't overlook something.

I'll add a note to the commit message.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 28.03.19 at 07:59, <jgross@suse.com> wrote:
> On 27/03/2019 17:52, Juergen Gross wrote:
>> On 27/03/2019 17:38, Jan Beulich wrote:
>>>>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
>>>> On 27/03/2019 16:55, Andrew Cooper wrote:
>>>>> On 18/03/2019 13:11, Juergen Gross wrote:
>>>>>> Instead of freeing percpu areas during suspend and allocating them
>>>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>>>> come up again when resuming.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Hmm - this is slightly problematic, given the dual nature of this code.
>>>>>
>>>>> I agree that it this change is beneficial for the suspend case, but it
>>>>> is a problem when we are parking an individual CPU for smt=0 or
>>>>> xen-hptool reasons.
>>>>>
>>>>> Do we have any hint we can use when taking the CPU down as to whether
>>>>> we're expecting it to come straight back up again?
>>>>
>>>> Did you look into the patch? I did this by testing system_state.
>>>
>>> I think there's a wider problem here: enable_nonboot_cpus()
>>> only brings back up the CPUs that were previously online.
>>> Parked ones would be left alone, yet after resume they'd
>>> need to be put back into parked state.
>> 
>> I can add that handling in the respin of the series.
> 
> Looking deeper into that mess I believe that should be a series of its
> own. Cpu parking needs to be handled for cpu hotplug and core parking
> (XENPF_core_parking), too.

What issue do you see for CPU hotplug? cpu_up_helper() has
been modified by the parking series.

For core parking I wonder whether core_parking_helper()
shouldn't, first of all, invoke cpu_{up,down}_helper(). This
wouldn't be enough, though - the policy hooks need to honor
opt_smt as well.

As to this wanting to be a patch / series of its own - I don't
mind, but preferably it would come ahead of your changes
here, so that it can be backported independently and
(sufficiently) easily (unless of course there's really no
collision between the two).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 28.03.19 at 08:53, <jgross@suse.com> wrote:
> On 28/03/2019 08:46, Jan Beulich wrote:
>>>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
>>> Instead of freeing percpu areas during suspend and allocating them
>>> again when resuming keep them. Only free an area in case a cpu didn't
>>> come up again when resuming.
>> 
>> There's another aspect here which needs at least mentioning:
>> Right now, CPUs coming back up have their per-CPU data
>> cleared. Not doing so may indeed be an improvement in
>> some cases (like statistics collected), but may also get in the
>> way in other cases.
> 
> I have checked the respective cpu notifier hooks and I think this is no
> problem. I hope I didn't overlook something.

Is checking the hooks sufficient? I'd rather expect we need to
check all per-CPU data objects.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 28/03/2019 09:03, Jan Beulich wrote:
>>>> On 28.03.19 at 07:59, <jgross@suse.com> wrote:
>> On 27/03/2019 17:52, Juergen Gross wrote:
>>> On 27/03/2019 17:38, Jan Beulich wrote:
>>>>>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
>>>>> On 27/03/2019 16:55, Andrew Cooper wrote:
>>>>>> On 18/03/2019 13:11, Juergen Gross wrote:
>>>>>>> Instead of freeing percpu areas during suspend and allocating them
>>>>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>>>>> come up again when resuming.
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>
>>>>>> Hmm - this is slightly problematic, given the dual nature of this code.
>>>>>>
>>>>>> I agree that it this change is beneficial for the suspend case, but it
>>>>>> is a problem when we are parking an individual CPU for smt=0 or
>>>>>> xen-hptool reasons.
>>>>>>
>>>>>> Do we have any hint we can use when taking the CPU down as to whether
>>>>>> we're expecting it to come straight back up again?
>>>>>
>>>>> Did you look into the patch? I did this by testing system_state.
>>>>
>>>> I think there's a wider problem here: enable_nonboot_cpus()
>>>> only brings back up the CPUs that were previously online.
>>>> Parked ones would be left alone, yet after resume they'd
>>>> need to be put back into parked state.
>>>
>>> I can add that handling in the respin of the series.
>>
>> Looking deeper into that mess I believe that should be a series of its
>> own. Cpu parking needs to be handled for cpu hotplug and core parking
>> (XENPF_core_parking), too.
> 
> What issue do you see for CPU hotplug? cpu_up_helper() has
> been modified by the parking series.

I was thinking of hot unplug. cpu_down() won't do the job for a parked
cpu.

> For core parking I wonder whether core_parking_helper()
> shouldn't, first of all, invoke cpu_{up,down}_helper(). This
> wouldn't be enough, though - the policy hooks need to honor
> opt_smt as well.

Right.

> As to this wanting to be a patch / series of its own - I don't
> mind, but preferably it would come ahead of your changes
> here, so that it can be backported independently and
> (sufficiently) easily (unless of course there's really no
> collision between the two).

In case there is a collision it should be fairly minimal.

I'd prefer not to block my series as it is a prerequisite for my core
scheduling series, which I believe should go in rather sooner than later
as it probably should see lot of testing before the next release.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 28.03.19 at 09:35, <jgross@suse.com> wrote:
> On 28/03/2019 09:03, Jan Beulich wrote:
>>>>> On 28.03.19 at 07:59, <jgross@suse.com> wrote:
>>> On 27/03/2019 17:52, Juergen Gross wrote:
>>>> On 27/03/2019 17:38, Jan Beulich wrote:
>>>>>>>> On 27.03.19 at 17:18, <jgross@suse.com> wrote:
>>>>>> On 27/03/2019 16:55, Andrew Cooper wrote:
>>>>>>> On 18/03/2019 13:11, Juergen Gross wrote:
>>>>>>>> Instead of freeing percpu areas during suspend and allocating them
>>>>>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>>>>>> come up again when resuming.
>>>>>>>>
>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>
>>>>>>> Hmm - this is slightly problematic, given the dual nature of this code.
>>>>>>>
>>>>>>> I agree that it this change is beneficial for the suspend case, but it
>>>>>>> is a problem when we are parking an individual CPU for smt=0 or
>>>>>>> xen-hptool reasons.
>>>>>>>
>>>>>>> Do we have any hint we can use when taking the CPU down as to whether
>>>>>>> we're expecting it to come straight back up again?
>>>>>>
>>>>>> Did you look into the patch? I did this by testing system_state.
>>>>>
>>>>> I think there's a wider problem here: enable_nonboot_cpus()
>>>>> only brings back up the CPUs that were previously online.
>>>>> Parked ones would be left alone, yet after resume they'd
>>>>> need to be put back into parked state.
>>>>
>>>> I can add that handling in the respin of the series.
>>>
>>> Looking deeper into that mess I believe that should be a series of its
>>> own. Cpu parking needs to be handled for cpu hotplug and core parking
>>> (XENPF_core_parking), too.
>> 
>> What issue do you see for CPU hotplug? cpu_up_helper() has
>> been modified by the parking series.
> 
> I was thinking of hot unplug. cpu_down() won't do the job for a parked
> cpu.

There's nothing to be done when soft-offlining a CPU. And I'm
not convinced physical CPU unplug was actually ever tested to
work.

>> As to this wanting to be a patch / series of its own - I don't
>> mind, but preferably it would come ahead of your changes
>> here, so that it can be backported independently and
>> (sufficiently) easily (unless of course there's really no
>> collision between the two).
> 
> In case there is a collision it should be fairly minimal.
> 
> I'd prefer not to block my series as it is a prerequisite for my core
> scheduling series, which I believe should go in rather sooner than later
> as it probably should see lot of testing before the next release.

Understood.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 28.03.19 at 09:03, <JBeulich@suse.com> wrote:
> For core parking I wonder whether core_parking_helper()
> shouldn't, first of all, invoke cpu_{up,down}_helper(). This
> wouldn't be enough, though - the policy hooks need to honor
> opt_smt as well.

Actually no, there was no problem at the time there: With
opt_smt set to false, no secondary thread would ever have
made it into core_parking_cpunum[] (that's where CPU numbers
to be passed to cpu_up() get taken from). A problem here was
introduced only by Andrew's 2bed1bc241, making it possible for
opt_smt to change at runtime. I think I'll make a patch to have
smt_up_down_helper() call into core-parking code to purge
CPUs from core_parking_cpunum[] as needed.

The interaction of core-parking and xen-hptool activities is up
for discussion anyway, I think. At least to me it's not
immediately clear which of the two should take priority.
Allowing admins to shoot themselves in the foot (as we appear
to do now) is a reasonable possibility, but not the only one, the
more that the platform is liable to notice higher power
consumption and to subsequently request offlining of CPUs
again anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/6] xen: don't free percpu areas during suspend

[Jan Beulich]

>>> On 28.03.19 at 09:03, <JBeulich@suse.com> wrote:
> For core parking I wonder whether core_parking_helper()
> shouldn't, first of all, invoke cpu_{up,down}_helper(). This
> wouldn't be enough, though - the policy hooks need to honor
> opt_smt as well.

Actually no, there was no problem at the time there: With
opt_smt set to false, no secondary thread would ever have
made it into core_parking_cpunum[] (that's where CPU numbers
to be passed to cpu_up() get taken from). A problem here was
introduced only by Andrew's 2bed1bc241, making it possible for
opt_smt to change at runtime. I think I'll make a patch to have
smt_up_down_helper() call into core-parking code to purge
CPUs from core_parking_cpunum[] as needed.

The interaction of core-parking and xen-hptool activities is up
for discussion anyway, I think. At least to me it's not
immediately clear which of the two should take priority.
Allowing admins to shoot themselves in the foot (as we appear
to do now) is a reasonable possibility, but not the only one, the
more that the platform is liable to notice higher power
consumption and to subsequently request offlining of CPUs
again anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen: don't free percpu areas during suspend

[Juergen Gross]

On 28/03/2019 09:04, Jan Beulich wrote:
>>>> On 28.03.19 at 08:53, <jgross@suse.com> wrote:
>> On 28/03/2019 08:46, Jan Beulich wrote:
>>>>>> On 18.03.19 at 14:11, <jgross@suse.com> wrote:
>>>> Instead of freeing percpu areas during suspend and allocating them
>>>> again when resuming keep them. Only free an area in case a cpu didn't
>>>> come up again when resuming.
>>>
>>> There's another aspect here which needs at least mentioning:
>>> Right now, CPUs coming back up have their per-CPU data
>>> cleared. Not doing so may indeed be an improvement in
>>> some cases (like statistics collected), but may also get in the
>>> way in other cases.
>>
>> I have checked the respective cpu notifier hooks and I think this is no
>> problem. I hope I didn't overlook something.
> 
> Is checking the hooks sufficient? I'd rather expect we need to
> check all per-CPU data objects.

Why? The main concern would be a hook not doing its job due to some
data not being zeroed.

In case there is a dependency somewhere else I'd rather expect subtle
negative effects with today's solution as suddenly percpu data will
be zero after suspend/resume without a component having been involved
with suspending.

Sure, there could be negative effects with my series, but I'd asset the
chances lower as without my series. Suspend/resume tends to be not
tested very often, it seems. I guess I'm the only one having it tried
recently (it was broken for 3 months at least before my patch repairing
an issue with IOMMU).


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel