Xen devel - Apr 2012 - [PATCH 0 of 3] xen scheduling: Fix credit2 and cpupools

This patch series fixes some long-standing bugs related to credit2
interacting with cpupools.

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

Because the location of the lock can change between the time you read
it and the time you grab it, the per-cpu schedule locks need to check
after lock acquisition that the lock location hasn''t changed, and
release and re-try if so.  This change was effected throughout the
source code, but one very important place was apparently missed: in
schedule() itself.

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

diff -r 197e09bf0322 -r fd0d2fd2824b xen/common/schedule.c
--- a/xen/common/schedule.c	Wed Apr 04 11:29:43 2012 +0100
+++ b/xen/common/schedule.c	Thu Apr 05 12:20:04 2012 +0100
@@ -1075,6 +1075,7 @@ static void schedule(void)
     bool_t                tasklet_work_scheduled = 0;
     struct schedule_data *sd;
     struct task_slice     next_slice;
+    int cpu = smp_processor_id();
 
     ASSERT(!in_atomic());
 
@@ -1099,7 +1100,7 @@ static void schedule(void)
         BUG();
     }
 
-    spin_lock_irq(sd->schedule_lock);
+    pcpu_schedule_lock_irq(cpu);
 
     stop_timer(&sd->s_timer);
     
@@ -1116,7 +1117,7 @@ static void schedule(void)
 
     if ( unlikely(prev == next) )
     {
-        spin_unlock_irq(sd->schedule_lock);
+        pcpu_schedule_unlock_irq(cpu);
         trace_continue_running(next);
         return continue_running(prev);
     }
@@ -1154,7 +1155,7 @@ static void schedule(void)
     ASSERT(!next->is_running);
     next->is_running = 1;
 
-    spin_unlock_irq(sd->schedule_lock);
+    pcpu_schedule_unlock_irq(cpu);
 
     perfc_incr(sched_ctx);

This fixes a bug that happens when you remove cpus from a credit2 cpupool.

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

diff -r fd0d2fd2824b -r 761f0a1209f6 xen/common/sched_credit2.c
--- a/xen/common/sched_credit2.c	Thu Apr 05 12:20:04 2012 +0100
+++ b/xen/common/sched_credit2.c	Thu Apr 05 12:45:14 2012 +0100
@@ -1952,6 +1952,7 @@ csched_free_pdata(const struct scheduler
     unsigned long flags;
     struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_runqueue_data *rqd;
+    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
     int rqi;
 
     spin_lock_irqsave(&prv->lock, flags);
@@ -1979,6 +1980,11 @@ csched_free_pdata(const struct scheduler
         deactivate_runqueue(prv, rqi);
     }
 
+    /* Move spinlock to the original lock.  */
+    ASSERT(sd->schedule_lock == &rqd->lock);
+    ASSERT(!spin_is_locked(&sd->_lock));
+    sd->schedule_lock = &sd->_lock;
+
     spin_unlock(&rqd->lock);
 
     cpumask_clear_cpu(cpu, &prv->initialized);

The full order for creating new private data structures when moving
from one pool to another is now:
* Allocate all new structures
 - Allocate a new private domain structure (but don''t point there yet)
 - Allocate per-vcpu data structures (but don''t point there yet)
* Remove old structures
 - Remove each vcpu, freeing the associated data structure
 - Free the domain data structure
* Switch to the new structures
 - Set the domain to the new cpupool, with the new private domain structure
 - Set each vcpu to the respective new structure, and insert

This is in line with a (fairly reasonable) assumption in credit2 that the
private structure of the domain will be the private structure pointed to
by the per-vcpu private structure.

Also fix a bug, in which insert_vcpu was called with the *old* vcpu ops
rather than the new ones.

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

diff -r 761f0a1209f6 -r 869ead491d7c xen/common/schedule.c
--- a/xen/common/schedule.c	Thu Apr 05 12:45:14 2012 +0100
+++ b/xen/common/schedule.c	Thu Apr 05 12:54:33 2012 +0100
@@ -261,6 +261,18 @@ int sched_move_domain(struct domain *d, 
 
     domain_pause(d);
 
+    for_each_vcpu ( d, v )
+    {
+        SCHED_OP(VCPU2OP(v), remove_vcpu, v);
+        SCHED_OP(VCPU2OP(v), free_vdata, v->sched_priv);
+        v->sched_priv = NULL;
+    }
+
+    SCHED_OP(DOM2OP(d), free_domdata, d->sched_priv);
+
+    d->cpupool = c;
+    d->sched_priv = domdata;
+
     new_p = cpumask_first(c->cpu_valid);
     for_each_vcpu ( d, v )
     {
@@ -268,9 +280,6 @@ int sched_move_domain(struct domain *d, 
         migrate_timer(&v->singleshot_timer, new_p);
         migrate_timer(&v->poll_timer, new_p);
 
-        SCHED_OP(VCPU2OP(v), remove_vcpu, v);
-        SCHED_OP(VCPU2OP(v), free_vdata, v->sched_priv);
-
         cpumask_setall(v->cpu_affinity);
         v->processor = new_p;
         v->sched_priv = vcpu_priv[v->vcpu_id];
@@ -278,13 +287,9 @@ int sched_move_domain(struct domain *d, 
 
         new_p = cpumask_cycle(new_p, c->cpu_valid);
 
-        SCHED_OP(VCPU2OP(v), insert_vcpu, v);
+        SCHED_OP(c->sched, insert_vcpu, v);
     }
 
-    d->cpupool = c;
-    SCHED_OP(DOM2OP(d), free_domdata, d->sched_priv);
-    d->sched_priv = domdata;
-
     domain_update_node_affinity(d);
 
     domain_unpause(d);