ACPI state change corrections

xen/common/cpupool.c +7 -0

+            cpupool_assign_cpu_locked(*c, cpu);
+            goto out;
+        }
+    }
    cpupool_assign_cpu_locked(cpupool0, cpu);
...
+void cpupool_disable_cpu(unsigned int cpu)
+{
+    struct cpupool *c;

Currently offlining a cpu will migrate all vcpus which were active on that cpu to another one, possibly breaking existing cpu affinities.

In case of an ACPI state change the cpus are taken offline and online later (if not poweroff) while all domains are paused. There is no reason to migrate the vcpus during offlining the cpus, as the cpus will be available again when the domains are being unpaused.

This patch defers the migration check in case of paused vcpus or domains by adding vcpu_arouse() to wake up a vcpu and check whether it must be migrated to another cpu.

Signed-off-by Juergen Gross <[email protected]>

3 files changed, 64 insertions(+), 24 deletions(-)
xen/common/domain.c     |    4 +-
xen/common/schedule.c   |   83 ++++++++++++++++++++++++++++++++++-------------
xen/include/xen/sched.h |    1

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Your original patch didn't touch this code. Was that an omission in the original version? On reflection I prefer your original patch to this new approach. I'll apply it if you still believe your original patch is complete and correct as it stands.

-- Keir

Quoted 21 lines · Juergen Gross

Currently offlining a cpu will migrate all vcpus which were active on that cpu to another one, possibly breaking existing cpu affinities.

In case of an ACPI state change the cpus are taken offline and online later (if not poweroff) while all domains are paused. There is no reason to migrate the vcpus during offlining the cpus, as the cpus will be available again when the domains are being unpaused.

This patch defers the migration check in case of paused vcpus or domains by adding vcpu_arouse() to wake up a vcpu and check whether it must be migrated to another cpu.

Signed-off-by Juergen Gross<[email protected]>

3 files changed, 64 insertions(+), 24 deletions(-)
xen/common/domain.c     |    4 +-
xen/common/schedule.c   |   83 ++++++++++++++++++++++++++++++++++-------------
xen/include/xen/sched.h |    1

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 4 lines · Keir Fraser

Your original patch didn't touch this code. Was that an omission in the original version? On reflection I prefer your original patch to this new approach. I'll apply it if you still believe your original patch is complete and correct as it stands.

I like my second patch more :-)

It covers more cases, not just poweroff. In hibernate case no vcpu pinnings will be lost. Today all vcpus pinned to a cpu other than 0 will lose their pinnings at cpu offlining. At reactivation those pinnings will not be restored automatically. My patch will cover that by checking availability of the cpus after reactivation.

Poweroff (which was my primary concern) works with both versions. I did not test other ACPI state changes with either version, but would expect better results in hibernate case with my second approach.

Juergen

Quoted 38 lines

-- Keir

Quoted 22 lines · Juergen Gross

Currently offlining a cpu will migrate all vcpus which were active on that cpu to another one, possibly breaking existing cpu affinities.

In case of an ACPI state change the cpus are taken offline and online later (if not poweroff) while all domains are paused. There is no reason to migrate the vcpus during offlining the cpus, as the cpus will be available again when the domains are being unpaused.

This patch defers the migration check in case of paused vcpus or domains by adding vcpu_arouse() to wake up a vcpu and check whether it must be migrated to another cpu.

Signed-off-by Juergen Gross<[email protected]>

3 files changed, 64 insertions(+), 24 deletions(-)

xen/common/domain.c | 4 +-
xen/common/schedule.c | 83
++++++++++++++++++++++++++++++++++-------------
xen/include/xen/sched.h | 1

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 13 lines · Juergen Gross

Quoted 4 lines · Keir Fraser

Your original patch didn't touch this code. Was that an omission in the original version? On reflection I prefer your original patch to this new approach. I'll apply it if you still believe your original patch is complete and correct as it stands.

I like my second patch more :-)

It covers more cases, not just poweroff. In hibernate case no vcpu pinnings will be lost. Today all vcpus pinned to a cpu other than 0 will lose their pinnings at cpu offlining. At reactivation those pinnings will not be restored automatically. My patch will cover that by checking availability of the cpus after reactivation.

Quoted 3 lines

Poweroff (which was my primary concern) works with both versions. I did not test other ACPI state changes with either version, but would expect better results in hibernate case with my second approach.

How about the attached patch? Which is similar to your original patch except I added the global state variable, and I added a check for it to cpu_disable_scheduler(). It's nice and small. :-)

-- Keir

Quoted 39 lines

Juergen

Quoted 37 lines

   -- Keir

Quoted 22 lines · Juergen Gross

Currently offlining a cpu will migrate all vcpus which were active on that cpu to another one, possibly breaking existing cpu affinities.

In case of an ACPI state change the cpus are taken offline and online later (if not poweroff) while all domains are paused. There is no reason to migrate the vcpus during offlining the cpus, as the cpus will be available again when the domains are being unpaused.

This patch defers the migration check in case of paused vcpus or domains by adding vcpu_arouse() to wake up a vcpu and check whether it must be migrated to another cpu.

Signed-off-by Juergen Gross<[email protected]>

3 files changed, 64 insertions(+), 24 deletions(-)

xen/common/domain.c | 4 +-
xen/common/schedule.c | 83
++++++++++++++++++++++++++++++++++-------------
xen/include/xen/sched.h | 1

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 13 lines · Juergen Gross

Quoted 4 lines · Keir Fraser

Your original patch didn't touch this code. Was that an omission in the original version? On reflection I prefer your original patch to this new approach. I'll apply it if you still believe your original patch is complete and correct as it stands.

I like my second patch more :-)

It covers more cases, not just poweroff. In hibernate case no vcpu pinnings will be lost. Today all vcpus pinned to a cpu other than 0 will lose their pinnings at cpu offlining. At reactivation those pinnings will not be restored automatically. My patch will cover that by checking availability of the cpus after reactivation.

Quoted 3 lines

Poweroff (which was my primary concern) works with both versions. I did not test other ACPI state changes with either version, but would expect better results in hibernate case with my second approach.

How about the attached patch? Which is similar to your original patch except I added the global state variable, and I added a check for it to cpu_disable_scheduler(). It's nice and small. :-)

Quoted 1 line

Quoted 1 line · Juergen Gross

xen/common/schedule.c +0 -0

@@ -525,6 +525,47 @@ void vcpu_force_reschedule(struct vcpu *
    }

xen/common/schedule.c +12 -1

+    {
+        set_bit(_VPF_migrating, &v->pause_flags);
+        vcpu_schedule_unlock_irqrestore(v, flags);
+        vcpu_migrate(v);
+        return;
+    }
+
+    vcpu_schedule_unlock_irqrestore(v, flags);
+
+    vcpu_wake(v);
+}
+
 /*
 * This function is used by cpu_hotplug code from stop_machine context
 * and from cpupools to switch schedulers on a cpu.

@@ -534,7 +575,6 @@ int cpu_disable_scheduler(unsigned int c
    struct domain *d;
    struct vcpu *v;
    struct cpupool *c;
-    cpumask_t online_affinity;
    int    ret = 0;
 
    c = per_cpu(cpupool, cpu);

xen/common/schedule.c +1 -4

            {
-                printk("Breaking vcpu affinity for domain %d vcpu %d\n",
-                        v->domain->domain_id, v->vcpu_id);
-                cpumask_setall(v->cpu_affinity);
-            }
+                if ( vcpu_chk_affinity(v, cpu) )

xen/common/schedule.c +7 -14

-            if ( v->processor == cpu )
-            {
-                set_bit(_VPF_migrating, &v->pause_flags);
-                vcpu_schedule_unlock_irq(v);
-                vcpu_sleep_nosync(v);
-                vcpu_migrate(v);
+                /*
+                 * A vcpu active in the hypervisor will not be migratable.
+                 * The caller should try again after releasing and reaquiring
+                 * all locks.
+                 */
+                if ( v->processor == cpu )
+                    ret = -EAGAIN;
            }
            else
            {
                vcpu_schedule_unlock_irq(v);
            }
-
-            /*
-             * A vcpu active in the hypervisor will not be migratable.
-             * The caller should try again after releasing and reaquiring
-             * all locks.
-             */
-            if ( v->processor == cpu )
-                ret = -EAGAIN;
        }
 
        domain_update_node_affinity(d);

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Juergen Gross

xen/common/schedule.c +0 -0

@@ -525,6 +525,47 @@ void vcpu_force_reschedule(struct vcpu *
     }
 }
 

xen/common/schedule.c void vcpu_force_reschedule(struct vcpu *

Quoted 8 lines

xen/common/schedule.c +8 -0

+static int vcpu_chk_affinity(struct vcpu *v, unsigned int cpu)
+{
+    cpumask_t online_affinity;
+    struct cpupool *c;
+
+    c = v->domain->cpupool;
+    cpumask_and(&online_affinity, v->cpu_affinity, c->cpu_valid);
+    if ( cpumask_empty(&online_affinity)&&

There's no need for a local cpumask_t variable here - please use !cpumask_intersects(v->cpu_affinity, c->cpu_valid) instead.

xen/common/schedule.c void vcpu_force_reschedule(struct vcpu *

Quoted 8 lines

xen/common/schedule.c +8 -0

+         cpumask_test_cpu(cpu, v->cpu_affinity) )
+    {
+        printk("Breaking vcpu affinity for domain %d vcpu %d\n",
+                v->domain->domain_id, v->vcpu_id);
+        cpumask_setall(v->cpu_affinity);
+    }
+
+    return ( !cpumask_test_cpu(cpu, c->cpu_valid)&&  (v->processor == cpu) );

Please drop the extra outermost parentheses.

xen/common/schedule.c void vcpu_force_reschedule(struct vcpu * · line 551

Quoted 8 lines

xen/common/schedule.c +8 -0

+}
+
+void vcpu_arouse(struct vcpu *v)
+{
+    unsigned long flags;
+
+    if ( atomic_read(&v->pause_count) ||
+         atomic_read(&v->domain->pause_count) )

Is it not possible (or even more correct) to use vcpu_runnable() here?

xen/common/schedule.c void vcpu_force_reschedule(struct vcpu *

Quoted 5 lines

xen/common/schedule.c +5 -0

+        return;
+
+    vcpu_schedule_lock_irqsave(v, flags);
+
+    if ( unlikely(vcpu_chk_affinity(v, v->processor)&&  (v != current)) )

unlikely() is generally useful only around individual conditions (i.e. not around&& or || expressions).

Quoted 27 lines

xen/common/schedule.c +12 -1

+    {
+        set_bit(_VPF_migrating,&v->pause_flags);
+        vcpu_schedule_unlock_irqrestore(v, flags);
+        vcpu_migrate(v);
+        return;
+    }
+
+    vcpu_schedule_unlock_irqrestore(v, flags);
+
+    vcpu_wake(v);
+}
+
 /*
  * This function is used by cpu_hotplug code from stop_machine context
  * and from cpupools to switch schedulers on a cpu.

@@ -534,7 +575,6 @@ int cpu_disable_scheduler(unsigned int c
     struct domain *d;
     struct vcpu *v;
     struct cpupool *c;
-    cpumask_t online_affinity;
     int    ret = 0;
 
     c = per_cpu(cpupool, cpu);

@@ -547,34 +587,33 @@ int cpu_disable_scheduler(unsigned int c
         {
             vcpu_schedule_lock_irq(v);
 

xen/common/schedule.c int cpu_disable_scheduler(unsigned int c · line 591

Quoted 5 lines

xen/common/schedule.c +2 -3

-            cpumask_and(&online_affinity, v->cpu_affinity, c->cpu_valid);
-            if ( cpumask_empty(&online_affinity)&&
-                 cpumask_test_cpu(cpu, v->cpu_affinity) )
+            if ( likely(!atomic_read(&v->pause_count)&&
+                        !atomic_read(&d->pause_count)) )

Same question as above regarding vcpu_runnable().

Quoted 6 lines

xen/common/schedule.c +1 -4

             {
-                printk("Breaking vcpu affinity for domain %d vcpu %d\n",
-                        v->domain->domain_id, v->vcpu_id);
-                cpumask_setall(v->cpu_affinity);
-            }
+                if ( vcpu_chk_affinity(v, cpu) )

xen/common/schedule.c int cpu_disable_scheduler(unsigned int c · line 603

Quoted 10 lines

xen/common/schedule.c +10 -0

+                {
+                    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);
+                }

Please drop the unnecessary braces here, as per the recently posted coding style draft.