Xen devel - Mar 2013 - [PATCH] x86/S3: Restore broken vcpu affinity on resume (v4)

When in SYS_STATE_suspend, and going through the cpu_disable_scheduler
path, save a copy of the current cpu affinity, and mark a flag to
restore it later.

Later, in the resume process, when enabling nonboot cpus restore these
affinities.

v2:
Formatting: Fix hard tabs.
remove early return in cpu_disable_scheduler() path.

v3:
Formatting: Fix remaining errant tab.
Move restore_vcpu_affinity() to thaw_domains(), eliminating the need to
promote for_each_cpupool()

v4:
Formatting: Fix if statement spacing.
Eliminate unnecessary if statement in thaw_domains()
Suppress affinity related logging to XENLOG_DEBUG to reduce noise.

Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
---
 xen/arch/x86/acpi/power.c |    3 +++
 xen/common/domain.c       |    2 ++
 xen/common/schedule.c     |   45 ++++++++++++++++++++++++++++++++++++++++++---
 xen/include/xen/sched.h   |    6 ++++++
 4 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 3c2585c..f41f0de 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -96,7 +96,10 @@ static void thaw_domains(void)
 
     rcu_read_lock(&domlist_read_lock);
     for_each_domain ( d )
+    {
+        restore_vcpu_affinity(d);
         domain_unpause(d);
+    }
     rcu_read_unlock(&domlist_read_lock);
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 64ee29d..590548e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -126,6 +126,7 @@ struct vcpu *alloc_vcpu(
 
     if ( !zalloc_cpumask_var(&v->cpu_affinity) ||
          !zalloc_cpumask_var(&v->cpu_affinity_tmp) ||
+         !zalloc_cpumask_var(&v->cpu_affinity_saved) ||
          !zalloc_cpumask_var(&v->vcpu_dirty_cpumask) )
         goto fail_free;
 
@@ -155,6 +156,7 @@ struct vcpu *alloc_vcpu(
  fail_free:
         free_cpumask_var(v->cpu_affinity);
         free_cpumask_var(v->cpu_affinity_tmp);
+        free_cpumask_var(v->cpu_affinity_saved);
         free_cpumask_var(v->vcpu_dirty_cpumask);
         free_vcpu_struct(v);
         return NULL;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 83fae4c..7364ff8 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -541,6 +541,38 @@ void vcpu_force_reschedule(struct vcpu *v)
     }
 }
 
+void restore_vcpu_affinity(struct domain *d)
+{
+    struct vcpu *v;
+
+    for_each_vcpu ( d, v )
+    {
+        vcpu_schedule_lock_irq(v);
+
+        if ( v->affinity_broken )
+        {
+            printk(XENLOG_DEBUG "Restoring affinity for d%dv%d\n",
+                   d->domain_id, v->vcpu_id);
+            cpumask_copy(v->cpu_affinity, v->cpu_affinity_saved);
+            v->affinity_broken = 0;
+        }
+
+        if ( v->processor == smp_processor_id() )
+        {
+            set_bit(_VPF_migrating, &v->pause_flags);
+            vcpu_schedule_unlock_irq(v);
+            vcpu_sleep_nosync(v);
+            vcpu_migrate(v);
+        }
+        else
+        {
+            vcpu_schedule_unlock_irq(v);
+        }
+    }
+
+    domain_update_node_affinity(d);
+}
+
 /*
  * This function is used by cpu_hotplug code from stop_machine context
  * and from cpupools to switch schedulers on a cpu.
@@ -554,7 +586,7 @@ int cpu_disable_scheduler(unsigned int cpu)
     int    ret = 0;
 
     c = per_cpu(cpupool, cpu);
-    if ( (c == NULL) || (system_state == SYS_STATE_suspend) )
+    if ( c == NULL )
         return ret;
 
     for_each_domain_in_cpupool ( d, c )
@@ -567,8 +599,15 @@ int cpu_disable_scheduler(unsigned int cpu)
             if ( cpumask_empty(&online_affinity) &&
                  cpumask_test_cpu(cpu, v->cpu_affinity) )
             {
-                printk("Breaking vcpu affinity for domain %d vcpu
%d\n",
-                        v->domain->domain_id, v->vcpu_id);
+                printk(XENLOG_DEBUG "Breaking affinity for d%dv%d\n",
+                        d->domain_id, v->vcpu_id);
+
+                if (system_state == SYS_STATE_suspend)
+                {
+                    cpumask_copy(v->cpu_affinity_saved, v->cpu_affinity);
+                    v->affinity_broken = 1;
+                }
+
                 cpumask_setall(v->cpu_affinity);
             }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index cabaf27..d15d567 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -153,6 +153,9 @@ struct vcpu
     bool_t           defer_shutdown;
     /* VCPU is paused following shutdown request (d->is_shutting_down)? */
     bool_t           paused_for_shutdown;
+    /* VCPU need affinity restored */
+    bool_t           affinity_broken;
+
 
     /*
      * > 0: a single port is being polled;
@@ -175,6 +178,8 @@ struct vcpu
     cpumask_var_t    cpu_affinity;
     /* Used to change affinity temporarily. */
     cpumask_var_t    cpu_affinity_tmp;
+    /* Used to restore affinity across S3. */
+    cpumask_var_t    cpu_affinity_saved;
 
     /* Bitmask of CPUs which are holding onto this VCPU''s state. */
     cpumask_var_t    vcpu_dirty_cpumask;
@@ -697,6 +702,7 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool
*c);
 void vcpu_force_reschedule(struct vcpu *v);
 int cpu_disable_scheduler(unsigned int cpu);
 int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity);
+void restore_vcpu_affinity(struct domain *d);
 
 void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate);
 uint64_t get_cpu_idle_time(unsigned int cpu);
-- 
1.7.9.5

>>> On 27.03.13 at 14:13, Ben Guthro <benjamin.guthro@citrix.com>
wrote:
> When in SYS_STATE_suspend, and going through the cpu_disable_scheduler
> path, save a copy of the current cpu affinity, and mark a flag to
> restore it later.
> 
> Later, in the resume process, when enabling nonboot cpus restore these
> affinities.
> 
> v2:
> Formatting: Fix hard tabs.
> remove early return in cpu_disable_scheduler() path.
> 
> v3:
> Formatting: Fix remaining errant tab.
> Move restore_vcpu_affinity() to thaw_domains(), eliminating the need to
> promote for_each_cpupool()
> 
> v4:
> Formatting: Fix if statement spacing.
> Eliminate unnecessary if statement in thaw_domains()
> Suppress affinity related logging to XENLOG_DEBUG to reduce noise.
> 
> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
Looks good to me now, but needs an ack by Keir (or George,
considering the minor change to xen/common/domain.c is perhaps
acceptable without formal ack).

Thanks, Jan

On Wed, Mar 27, 2013 at 1:13 PM, Ben Guthro <benjamin.guthro@citrix.com>
wrote:
> When in SYS_STATE_suspend, and going through the cpu_disable_scheduler
> path, save a copy of the current cpu affinity, and mark a flag to
> restore it later.
>
> Later, in the resume process, when enabling nonboot cpus restore these
> affinities.
>
> v2:
> Formatting: Fix hard tabs.
> remove early return in cpu_disable_scheduler() path.
>
> v3:
> Formatting: Fix remaining errant tab.
> Move restore_vcpu_affinity() to thaw_domains(), eliminating the need to
> promote for_each_cpupool()
>
> v4:
> Formatting: Fix if statement spacing.
> Eliminate unnecessary if statement in thaw_domains()
> Suppress affinity related logging to XENLOG_DEBUG to reduce noise.
>
> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
I''m not super-familiar with the save/restore paths; but it looks like
a reasonable change to me:

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

On 03/28/2013 08:18 AM, George Dunlap wrote:
> On Wed, Mar 27, 2013 at 1:13 PM, Ben Guthro
<benjamin.guthro@citrix.com> wrote:
>> When in SYS_STATE_suspend, and going through the cpu_disable_scheduler
>> path, save a copy of the current cpu affinity, and mark a flag to
>> restore it later.
>>
>> Later, in the resume process, when enabling nonboot cpus restore these
>> affinities.
>>
>> v2:
>> Formatting: Fix hard tabs.
>> remove early return in cpu_disable_scheduler() path.
>>
>> v3:
>> Formatting: Fix remaining errant tab.
>> Move restore_vcpu_affinity() to thaw_domains(), eliminating the need to
>> promote for_each_cpupool()
>>
>> v4:
>> Formatting: Fix if statement spacing.
>> Eliminate unnecessary if statement in thaw_domains()
>> Suppress affinity related logging to XENLOG_DEBUG to reduce noise.
>>
>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>
> I''m not super-familiar with the save/restore paths; but it looks
like
> a reasonable change to me:
>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>
Thanks George.

Jan,

Is this a sufficient Ack, or does Kier need to weigh in as well?

Thanks,

Ben

On 01/04/2013 20:47, "Ben Guthro" <Benjamin.Guthro@citrix.com>
wrote:
>>> v4:
>>> Formatting: Fix if statement spacing.
>>> Eliminate unnecessary if statement in thaw_domains()
>>> Suppress affinity related logging to XENLOG_DEBUG to reduce noise.
>>> 
>>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>> 
>> I''m not super-familiar with the save/restore paths; but it
looks like
>> a reasonable change to me:
>> 
>> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>> 
> 
> Thanks George.
> 
> Jan,
> 
> Is this a sufficient Ack, or does Kier need to weigh in as well?
Acked-by: Keir Fraser <keir@xen.org>

I kind of feel this sort of thing should be done in dom0 userspace but,
pragmatically, I know that''s not really going to happen!

 -- Keir

>>> On 01.04.13 at 22:17, Keir Fraser <keir.xen@gmail.com> wrote:
> On 01/04/2013 20:47, "Ben Guthro"
<Benjamin.Guthro@citrix.com> wrote:
> 
>>>> v4:
>>>> Formatting: Fix if statement spacing.
>>>> Eliminate unnecessary if statement in thaw_domains()
>>>> Suppress affinity related logging to XENLOG_DEBUG to reduce
noise.
>>>> 
>>>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>>> 
>>> I''m not super-familiar with the save/restore paths; but it
looks like
>>> a reasonable change to me:
>>> 
>>> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> 
>> 
>> Thanks George.
>> 
>> Jan,
>> 
>> Is this a sufficient Ack, or does Kier need to weigh in as well?
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
> I kind of feel this sort of thing should be done in dom0 userspace but,
> pragmatically, I know that''s not really going to happen!
How would that work, considering the Dom0''s vCPU affinities are
equally affected, and considering that all domains get thawed at
once? Minimally you''d have a burst of (overcommit) load on pCPU0
right after resume, and whether the system would survive that is
impossible to predict.

Jan

On 02/04/2013 08:49, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> Thanks George.
>>> 
>>> Jan,
>>> 
>>> Is this a sufficient Ack, or does Kier need to weigh in as well?
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
>> 
>> I kind of feel this sort of thing should be done in dom0 userspace but,
>> pragmatically, I know that''s not really going to happen!
> 
> How would that work, considering the Dom0''s vCPU affinities are
> equally affected, and considering that all domains get thawed at
> once? Minimally you''d have a burst of (overcommit) load on pCPU0
> right after resume, and whether the system would survive that is
> impossible to predict.
Ha, yes, well you''d put freeze/thaw in dom0 too. Well, it
wasn''t really a
serious suggestion on my part, what we have works, this new patch will also
work, and I''m happy. :)

 -- Keir