[Xen-devel] [PATCH 2/7] xen: sched: fix locking for insert_vcpu() in credit1 and RTDS

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

The insert_vcpu scheduler hook is called with an inconsistent
locking strategy. In fact, it is sometimes invoked while
holding the runqueue lock and sometimes when that is not the
case.

For instance, in case of schedule_cpu_switch() the lock is
acquired in generic code. On the other hand, in case of
sched_move_domain(), locking is left as a responsibility
of the schedulers implementing the hook.

This results in Credit1 and RTDS schedulers ending up (in
case of sched_move_domain()) doing runqueue manipulation
without holding any runqueue lock, which is a bug. (Credit2
was doing the locking by itself already.)

The right thing is to defer locking to the specific schedulers,
as it's them that know what, how and when it is best to lock
(as in: runqueue locks, vs. private scheduler locks, vs. both,
etc.).

This patch, therefore:
 - removes any locking around insert_vcpu() from generic
   code;
 - add proper locking in the hook implementations, for
   both Credit1 and RTDS.

Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
---
Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
Cc: Meng Xu <mengxu@xxxxxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: Jan Beulich <JBeulich@xxxxxxxx>
---
Changes from v1 (within the other series):
 * split the patch (wrt the original patch, in the original
   series), and take care, in this one, only of insert_vcpu();
---
 xen/common/sched_credit.c |    5 +++++
 xen/common/sched_rt.c     |    3 +++
 xen/common/schedule.c     |    6 ------
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 6f71e0d..fccb368 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -903,10 +903,15 @@ static void
 csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct csched_vcpu *svc = vc->sched_priv;
+    spinlock_t *lock;
+
+    lock = vcpu_schedule_lock_irq(vc);
 
     if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
         __runq_insert(vc->processor, svc);
 
+    vcpu_schedule_unlock_irq(lock, vc);
+
     SCHED_STAT_CRANK(vcpu_insert);
 }
 
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 6a341b1..1086399 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -622,16 +622,19 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu 
*vc)
 {
     struct rt_vcpu *svc = rt_vcpu(vc);
     s_time_t now = NOW();
+    spinlock_t *lock;
 
     /* not addlocate idle vcpu to dom vcpu list */
     if ( is_idle_vcpu(vc) )
         return;
 
+    lock = vcpu_schedule_lock_irq(vc);
     if ( now >= svc->cur_deadline )
         rt_update_deadline(now, svc);
 
     if ( !__vcpu_on_q(svc) && vcpu_runnable(vc) && !vc->is_running )
         __runq_insert(ops, svc);
+    vcpu_schedule_unlock_irq(lock, vc);
 
     /* add rt_vcpu svc to scheduler-specific vcpu list of the dom */
     list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c5f640f..9aa209d 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1488,9 +1488,7 @@ void __init scheduler_init(void)
 
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
-    unsigned long flags;
     struct vcpu *idle;
-    spinlock_t *lock;
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
     struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
@@ -1509,8 +1507,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool 
*c)
         return -ENOMEM;
     }
 
-    lock = pcpu_schedule_lock_irqsave(cpu, &flags);
-
     SCHED_OP(old_ops, tick_suspend, cpu);
     vpriv_old = idle->sched_priv;
     idle->sched_priv = vpriv;
@@ -1520,8 +1516,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool 
*c)
     SCHED_OP(new_ops, tick_resume, cpu);
     SCHED_OP(new_ops, insert_vcpu, idle);
 
-    pcpu_schedule_unlock_irqrestore(lock, flags, cpu);
-
     SCHED_OP(old_ops, free_vdata, vpriv_old);
     SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel