sched: rework locking for dump debugkey.

PATCH In review Dario Faggioli 3 participants 6 messages Jan 18, 2012 – Jan 26, 2012

lore

Share & tools

lore

Dario Faggioli

author

2012-01-18 16:13 UTC #0 lore raw

PATCH 1/1 sched: rework locking for dump debugkey.

Hi George, everyone,

I noticed this issue while going through the scheduling code. Basically, for the runq debugkey, locking happens in schedule.c (which we usually don't want) and doesn't seem take credit2's runqs<-->CPUs mapping into account.

I think this patch correctly deals with locking in this specific case, but of course I'm happy to hear what you think! :-)
I tested the patch with all schedulers, but as you can imagine it's quite hard to produce an actual race and see if it is being handled properly...

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

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

Jan Beulich

2012-01-18 16:22 UTC #1 lore raw

Quoted 11 lines

Quoted 1 line
Quoted 1 line · Dario Faggioli
Hi George, everyone,
I noticed this issue while going through the scheduling code. Basically, for the runq debugkey, locking happens in schedule.c (which we usually don't want) and doesn't seem take credit2's runqs<-->CPUs mapping into account.
I think this patch correctly deals with locking in this specific case, but of course I'm happy to hear what you think! :-)

Forgot to include/attach the patch?

Quoted 13 lines

I tested the patch with all schedulers, but as you can imagine it's quite hard to produce an actual race and see if it is being handled properly...
Thanks and Regards,
Dario
Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli 
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

Dario Faggioli

author

2012-01-18 16:28 UTC #2 lore raw

Quoted 5 lines · Jan Beulich

Quoted 2 lines
I think this patch correctly deals with locking in this specific case, but of course I'm happy to hear what you think! :-)
Forgot to include/attach the patch?

No, it's coming but you're right, my "submission machinery" definitely need some refinements! :-)

Will do that ASAP, as I expect to start submitting more complex series in the coming period, and this method I'm using now does not scale.

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

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

Dario Faggioli

author

2012-01-18 16:24 UTC #3 lore raw

PATCH 1/1 sched: rework locking for dump debugkey.

In reply to Dario Faggioli

As in all other paths, locking should be dealt with in the specific schedulers, not in schedule.c.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/sched_credit.c +18 -2

diff -r 15ab61865ecb xen/common/sched_credit.c
@@ -1451,11 +1451,16 @@ static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
     struct list_head *runq, *iter;
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_pcpu *spc;
     struct csched_vcpu *svc;
+    unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
 
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
+
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;

@@ -1464,6 +1469,12 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(cpu);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1482,6 +1493,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    pcpu_schedule_unlock(cpu);
+    spin_unlock_irqrestore(&prv->lock, flags);
 #undef cpustr
 }

@@ -1493,7 +1507,7 @@ csched_dump(const struct scheduler *ops)
     int loop;
     unsigned long flags;
 
-    spin_lock_irqsave(&(prv->lock), flags);
+    spin_lock_irqsave(&prv->lock, flags);
 
 #define idlers_buf keyhandler_scratch

@@ -1537,12 +1551,14 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, active_vcpu_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
 #undef idlers_buf
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static int

xen/common/sched_credit2.c +26 -8

diff -r 15ab61865ecb xen/common/sched_credit2.c
@@ -53,8 +53,6 @@
  * credit2 wiki page:
  *  http://wiki.xen.org/wiki/Credit2_Scheduler_Development
  * TODO:
- * + Immediate bug-fixes
- *  - Do per-runqueue, grab proper lock for dump debugkey
  * + Multiple sockets
  *  - Detect cpu layout and make runqueue map, one per L2 (make_runq_map())
  *  - Simple load balancer / runqueue assignment

@@ -1759,12 +1757,16 @@ csched_dump_vcpu(struct csched_vcpu *svc
 static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct list_head *runq, *iter;
     struct csched_vcpu *svc;
+    unsigned long flags;
+    spinlock_t *lock;
     int loop;
     char cpustr[100];
 
-    /* FIXME: Do locking properly for access to runqueue structures */
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
 
     runq = &RQD(ops, cpu)->runq;

@@ -1773,6 +1775,13 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and here's how we get to it
+     * for the requested cpu.
+     */
+    lock = per_cpu(schedule_data, cpu).schedule_lock;
+    spin_lock(lock);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1791,6 +1800,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    spin_unlock(lock);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

@@ -1798,8 +1810,11 @@ csched_dump(const struct scheduler *ops)
 {
     struct list_head *iter_sdom, *iter_svc;
     struct csched_private *prv = CSCHED_PRIV(ops);
+    unsigned long flags;
     int i, loop;
 
+    spin_lock_irqsave(&prv->lock, flags);
+
     printk("Active queues: %d\n"
            "\tdefault-weight     = %d\n",
            cpumask_weight(&prv->active_queues),

@@ -1822,7 +1837,6 @@ csched_dump(const struct scheduler *ops)
                fraction);
 
     }
-    /* FIXME: Locking! */
 
     printk("Domain info:\n");
     loop = 0;

@@ -1831,10 +1845,10 @@ csched_dump(const struct scheduler *ops)
         struct csched_dom *sdom;
         sdom = list_entry(iter_sdom, struct csched_dom, sdom_elem);
 
-       printk("\tDomain: %d w %d v %d\n\t", 
-              sdom->dom->domain_id, 
-              sdom->weight, 
-              sdom->nr_vcpus);
+        printk("\tDomain: %d w %d v %d\n\t", 
+               sdom->dom->domain_id, 
+               sdom->weight, 
+               sdom->nr_vcpus);
 
         list_for_each( iter_svc, &sdom->vcpu )
         {

@@ -1842,9 +1856,13 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, sdom_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void activate_runqueue(struct csched_private *prv, int rqi)

xen/common/sched_sedf.c +15 -0

diff -r 15ab61865ecb xen/common/sched_sedf.c
@@ -1219,13 +1219,25 @@ static void sedf_dump_domain(struct vcpu
 /* Dumps all domains on the specified cpu */
 static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
     struct list_head      *list, *queue, *tmp;
     struct sedf_vcpu_info *d_inf;
     struct domain         *d;
     struct vcpu    *ed;
+    unsigned long flags;
     int loop = 0;
+
+    /* Domains' parameters are changed under scheduler lock */
+    spin_lock_irqsave(&prv->lock, flags);
  
     printk("now=%"PRIu64"\n",NOW());
+
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(i);
+
     queue = RUNQ(i);
     printk("RUNQ rq %lx   n: %lx, p: %lx\n",  (unsigned long)queue,
            (unsigned long) queue->next, (unsigned long) queue->prev);

@@ -1288,6 +1300,9 @@ static void sedf_dump_cpu_state(const st
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    pcpu_schedule_unlock(i);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

xen/common/schedule.c +1 -2

diff -r 15ab61865ecb xen/common/schedule.c
@@ -1417,6 +1417,7 @@ void schedule_dump(struct cpupool *c)
     struct scheduler *sched;
     cpumask_t        *cpus;
 
+    /* Proper locking is provided by each scheduler */
     sched = (c == NULL) ? &ops : c->sched;
     cpus = (c == NULL) ? &cpupool_free_cpus : c->cpu_valid;
     printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name);

@@ -1424,10 +1425,8 @@ void schedule_dump(struct cpupool *c)
 
     for_each_cpu (i, cpus)
     {
-        pcpu_schedule_lock(i);
         printk("CPU[%02d] ", i);
         SCHED_OP(sched, dump_cpu_state, i);
-        pcpu_schedule_unlock(i);
     }
 }

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

# HG changeset patch
# Parent 15ab61865ecbd146f6ce65fbea5bf49bfd9c6cb1
sched: rework locking for dump debugkey.

As in all other paths, locking should be dealt with in the specific schedulers, not in schedule.c.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/sched_credit.c +18 -2

diff -r 15ab61865ecb xen/common/sched_credit.c
@@ -1451,11 +1451,16 @@ static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
     struct list_head *runq, *iter;
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_pcpu *spc;
     struct csched_vcpu *svc;
+    unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
 
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
+
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;

@@ -1464,6 +1469,12 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(cpu);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1482,6 +1493,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    pcpu_schedule_unlock(cpu);
+    spin_unlock_irqrestore(&prv->lock, flags);
 #undef cpustr
 }

@@ -1493,7 +1507,7 @@ csched_dump(const struct scheduler *ops)
     int loop;
     unsigned long flags;
 
-    spin_lock_irqsave(&(prv->lock), flags);
+    spin_lock_irqsave(&prv->lock, flags);
 
 #define idlers_buf keyhandler_scratch

@@ -1537,12 +1551,14 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, active_vcpu_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
 #undef idlers_buf
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static int

xen/common/sched_credit2.c +26 -8

diff -r 15ab61865ecb xen/common/sched_credit2.c
@@ -53,8 +53,6 @@
  * credit2 wiki page:
  *  http://wiki.xen.org/wiki/Credit2_Scheduler_Development
  * TODO:
- * + Immediate bug-fixes
- *  - Do per-runqueue, grab proper lock for dump debugkey
  * + Multiple sockets
  *  - Detect cpu layout and make runqueue map, one per L2 (make_runq_map())
  *  - Simple load balancer / runqueue assignment

@@ -1759,12 +1757,16 @@ csched_dump_vcpu(struct csched_vcpu *svc
 static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct list_head *runq, *iter;
     struct csched_vcpu *svc;
+    unsigned long flags;
+    spinlock_t *lock;
     int loop;
     char cpustr[100];
 
-    /* FIXME: Do locking properly for access to runqueue structures */
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
 
     runq = &RQD(ops, cpu)->runq;

@@ -1773,6 +1775,13 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and here's how we get to it
+     * for the requested cpu.
+     */
+    lock = per_cpu(schedule_data, cpu).schedule_lock;
+    spin_lock(lock);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1791,6 +1800,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    spin_unlock(lock);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

@@ -1798,8 +1810,11 @@ csched_dump(const struct scheduler *ops)
 {
     struct list_head *iter_sdom, *iter_svc;
     struct csched_private *prv = CSCHED_PRIV(ops);
+    unsigned long flags;
     int i, loop;
 
+    spin_lock_irqsave(&prv->lock, flags);
+
     printk("Active queues: %d\n"
            "\tdefault-weight     = %d\n",
            cpumask_weight(&prv->active_queues),

@@ -1822,7 +1837,6 @@ csched_dump(const struct scheduler *ops)
                fraction);
 
     }
-    /* FIXME: Locking! */
 
     printk("Domain info:\n");
     loop = 0;

@@ -1831,10 +1845,10 @@ csched_dump(const struct scheduler *ops)
         struct csched_dom *sdom;
         sdom = list_entry(iter_sdom, struct csched_dom, sdom_elem);
 
-       printk("\tDomain: %d w %d v %d\n\t", 
-              sdom->dom->domain_id, 
-              sdom->weight, 
-              sdom->nr_vcpus);
+        printk("\tDomain: %d w %d v %d\n\t", 
+               sdom->dom->domain_id, 
+               sdom->weight, 
+               sdom->nr_vcpus);
 
         list_for_each( iter_svc, &sdom->vcpu )
         {

@@ -1842,9 +1856,13 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, sdom_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void activate_runqueue(struct csched_private *prv, int rqi)

xen/common/sched_sedf.c +15 -0

diff -r 15ab61865ecb xen/common/sched_sedf.c
@@ -1219,13 +1219,25 @@ static void sedf_dump_domain(struct vcpu
 /* Dumps all domains on the specified cpu */
 static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
     struct list_head      *list, *queue, *tmp;
     struct sedf_vcpu_info *d_inf;
     struct domain         *d;
     struct vcpu    *ed;
+    unsigned long flags;
     int loop = 0;
+
+    /* Domains' parameters are changed under scheduler lock */
+    spin_lock_irqsave(&prv->lock, flags);
  
     printk("now=%"PRIu64"\n",NOW());
+
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(i);
+
     queue = RUNQ(i);
     printk("RUNQ rq %lx   n: %lx, p: %lx\n",  (unsigned long)queue,
            (unsigned long) queue->next, (unsigned long) queue->prev);

@@ -1288,6 +1300,9 @@ static void sedf_dump_cpu_state(const st
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    pcpu_schedule_unlock(i);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

xen/common/schedule.c +1 -2

diff -r 15ab61865ecb xen/common/schedule.c
@@ -1417,6 +1417,7 @@ void schedule_dump(struct cpupool *c)
     struct scheduler *sched;
     cpumask_t        *cpus;
 
+    /* Proper locking is provided by each scheduler */
     sched = (c == NULL) ? &ops : c->sched;
     cpus = (c == NULL) ? &cpupool_free_cpus : c->cpu_valid;
     printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name);

@@ -1424,10 +1425,8 @@ void schedule_dump(struct cpupool *c)
 
     for_each_cpu (i, cpus)
     {
-        pcpu_schedule_lock(i);
         printk("CPU[%02d] ", i);
         SCHED_OP(sched, dump_cpu_state, i);
-        pcpu_schedule_unlock(i);
     }
 }

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

George Dunlap

2012-01-26 16:37 UTC #4 lore raw

Quoted 2 lines · Dario Faggioli

As in all other paths, locking should be dealt with in the specific schedulers, not in schedule.c.

I think this looks right. But you should probably add a comment at the top of credit1 and sedf to say that now the locking order must be private -> schedule, to avoid deadlock.

(Before I don't think there was an ordering.)

Thanks,
-George

Quoted 236 lines

Signed-off-by Dario Faggioli <[email protected]>

diff -r 15ab61865ecb xen/common/sched_credit.c

xen/common/sched_credit.c +18 -2

@@ -1451,11 +1451,16 @@ static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
     struct list_head *runq, *iter;
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_pcpu *spc;
     struct csched_vcpu *svc;
+    unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
 
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
+
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;

@@ -1464,6 +1469,12 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(cpu);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1482,6 +1493,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    pcpu_schedule_unlock(cpu);
+    spin_unlock_irqrestore(&prv->lock, flags);
 #undef cpustr
 }

@@ -1493,7 +1507,7 @@ csched_dump(const struct scheduler *ops)
     int loop;
     unsigned long flags;
 
-    spin_lock_irqsave(&(prv->lock), flags);
+    spin_lock_irqsave(&prv->lock, flags);
 
 #define idlers_buf keyhandler_scratch

@@ -1537,12 +1551,14 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, active_vcpu_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
 #undef idlers_buf
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static int

diff -r 15ab61865ecb xen/common/sched_credit2.c

xen/common/sched_credit2.c +26 -8

@@ -53,8 +53,6 @@
  * credit2 wiki page:
  *  http://wiki.xen.org/wiki/Credit2_Scheduler_Development
  * TODO:
- * + Immediate bug-fixes
- *  - Do per-runqueue, grab proper lock for dump debugkey
  * + Multiple sockets
  *  - Detect cpu layout and make runqueue map, one per L2 (make_runq_map())
  *  - Simple load balancer / runqueue assignment

@@ -1759,12 +1757,16 @@ csched_dump_vcpu(struct csched_vcpu *svc
 static void
 csched_dump_pcpu(const struct scheduler *ops, int cpu)
 {
+    struct csched_private *prv = CSCHED_PRIV(ops);
     struct list_head *runq, *iter;
     struct csched_vcpu *svc;
+    unsigned long flags;
+    spinlock_t *lock;
     int loop;
     char cpustr[100];
 
-    /* FIXME: Do locking properly for access to runqueue structures */
+    /* Domains' parameters are changed under csched_priv lock */
+    spin_lock_irqsave(&prv->lock, flags);
 
     runq = &RQD(ops, cpu)->runq;

@@ -1773,6 +1775,13 @@ csched_dump_pcpu(const struct scheduler 
     cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
     printk("core=%s\n", cpustr);
 
+    /*
+     * We need runq lock as well, and here's how we get to it
+     * for the requested cpu.
+     */
+    lock = per_cpu(schedule_data, cpu).schedule_lock;
+    spin_lock(lock);
+
     /* current VCPU */
     svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
     if ( svc )

@@ -1791,6 +1800,9 @@ csched_dump_pcpu(const struct scheduler 
             csched_dump_vcpu(svc);
         }
     }
+
+    spin_unlock(lock);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

@@ -1798,8 +1810,11 @@ csched_dump(const struct scheduler *ops)
 {
     struct list_head *iter_sdom, *iter_svc;
     struct csched_private *prv = CSCHED_PRIV(ops);
+    unsigned long flags;
     int i, loop;
 
+    spin_lock_irqsave(&prv->lock, flags);
+
     printk("Active queues: %d\n"
            "\tdefault-weight     = %d\n",
            cpumask_weight(&prv->active_queues),

@@ -1822,7 +1837,6 @@ csched_dump(const struct scheduler *ops)
                fraction);
 
     }
-    /* FIXME: Locking! */
 
     printk("Domain info:\n");
     loop = 0;

@@ -1831,10 +1845,10 @@ csched_dump(const struct scheduler *ops)
         struct csched_dom *sdom;
         sdom = list_entry(iter_sdom, struct csched_dom, sdom_elem);
 
-       printk("\tDomain: %d w %d v %d\n\t", 
-              sdom->dom->domain_id, 
-              sdom->weight, 
-              sdom->nr_vcpus);
+        printk("\tDomain: %d w %d v %d\n\t", 
+               sdom->dom->domain_id, 
+               sdom->weight, 
+               sdom->nr_vcpus);
 
         list_for_each( iter_svc, &sdom->vcpu )
         {

@@ -1842,9 +1856,13 @@ csched_dump(const struct scheduler *ops)
             svc = list_entry(iter_svc, struct csched_vcpu, sdom_elem);
 
             printk("\t%3d: ", ++loop);
+            vcpu_schedule_lock(svc->vcpu);
             csched_dump_vcpu(svc);
+            vcpu_schedule_unlock(svc->vcpu);
         }
     }
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void activate_runqueue(struct csched_private *prv, int rqi)

diff -r 15ab61865ecb xen/common/sched_sedf.c

xen/common/sched_sedf.c +15 -0

@@ -1219,13 +1219,25 @@ static void sedf_dump_domain(struct vcpu
 /* Dumps all domains on the specified cpu */
 static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
     struct list_head      *list, *queue, *tmp;
     struct sedf_vcpu_info *d_inf;
     struct domain         *d;
     struct vcpu    *ed;
+    unsigned long flags;
     int loop = 0;
+
+    /* Domains' parameters are changed under scheduler lock */
+    spin_lock_irqsave(&prv->lock, flags);
 
     printk("now=%"PRIu64"\n",NOW());
+
+    /*
+     * We need runq lock as well, and as there's one runq per CPU,
+     * this is the correct one to take for this CPU/runq.
+     */
+    pcpu_schedule_lock(i);
+
     queue = RUNQ(i);
     printk("RUNQ rq %lx   n: %lx, p: %lx\n",  (unsigned long)queue,
            (unsigned long) queue->next, (unsigned long) queue->prev);

@@ -1288,6 +1300,9 @@ static void sedf_dump_cpu_state(const st
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    pcpu_schedule_unlock(i);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

diff -r 15ab61865ecb xen/common/schedule.c

xen/common/schedule.c +1 -2

@@ -1417,6 +1417,7 @@ void schedule_dump(struct cpupool *c)
     struct scheduler *sched;
     cpumask_t        *cpus;
 
+    /* Proper locking is provided by each scheduler */
     sched = (c == NULL) ? &ops : c->sched;
     cpus = (c == NULL) ? &cpupool_free_cpus : c->cpu_valid;
     printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name);

@@ -1424,10 +1425,8 @@ void schedule_dump(struct cpupool *c)
 
     for_each_cpu (i, cpus)
     {
-        pcpu_schedule_lock(i);
         printk("CPU[%02d] ", i);
         SCHED_OP(sched, dump_cpu_state, i);
-        pcpu_schedule_unlock(i);
     }
 }

Dario Faggioli

author

2012-01-26 16:55 UTC #5 lore raw

Quoted 6 lines · George Dunlap

Quoted 2 lines · Dario Faggioli
As in all other paths, locking should be dealt with in the specific schedulers, not in schedule.c.
I think this looks right.

Glad to hear that. :-)

Quoted 4 lines

But you should probably add a comment at the
top of credit1 and sedf to say that now the locking order must be private -> schedule, to avoid deadlock.

Right, will do.

Thanks,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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