Skip to contentSkip to conversation

xen scheduling: Fix credit2 and cpupools

PATCH New George Dunlap 1 participant 4 messages Apr 5, 2012

lore
Share & tools
lore
author
#1 lore raw

PATCH 1/1 xen: Fix schedule's grabbing of the schedule lock

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 <[email protected]>
xen/common/schedule.c +4 -3
diff -r 197e09bf0322 -r fd0d2fd2824b xen/common/schedule.c
@@ -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);
author
#2 lore raw

PATCH 1/1 xen, credit2: Put the per-cpu schedule lock back to the default lock when releasing cpu

In reply to George Dunlap

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

Signed-off-by George Dunlap <[email protected]>
xen/common/sched_credit2.c +6 -0
diff -r fd0d2fd2824b -r 761f0a1209f6 xen/common/sched_credit2.c
@@ -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);
author
#3 lore raw

PATCH 1/1 xen, cpupools: Fix cpupool-move to make more consistent

In reply to George Dunlap

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 <[email protected]>
xen/common/schedule.c +13 -8
diff -r 761f0a1209f6 -r 869ead491d7c xen/common/schedule.c
@@ -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);