* [PATCH 0 of 1] sched: rework locking for dump debugkey.
@ 2012-01-18 16:13 Dario Faggioli
2012-01-18 16:22 ` Jan Beulich
2012-01-18 16:24 ` [PATCH 1 " Dario Faggioli
0 siblings, 2 replies; 6+ messages in thread
From: Dario Faggioli @ 2012-01-18 16:13 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel@lists.xensource.com, Keir (Xen.org)
[-- Attachment #1.1: Type: text/plain, Size: 897 bytes --]
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
--
<<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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0 of 1] sched: rework locking for dump debugkey.
2012-01-18 16:13 [PATCH 0 of 1] sched: rework locking for dump debugkey Dario Faggioli
@ 2012-01-18 16:22 ` Jan Beulich
2012-01-18 16:28 ` Dario Faggioli
2012-01-18 16:24 ` [PATCH 1 " Dario Faggioli
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-01-18 16:22 UTC (permalink / raw)
To: Dario Faggioli
Cc: George Dunlap, xen-devel@lists.xensource.com, Keir (Xen.org)
>>> On 18.01.12 at 17:13, Dario Faggioli <raistlin@linux.it> wrote:
> 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?
> 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
>
> --
> <<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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1 of 1] sched: rework locking for dump debugkey.
2012-01-18 16:13 [PATCH 0 of 1] sched: rework locking for dump debugkey Dario Faggioli
2012-01-18 16:22 ` Jan Beulich
@ 2012-01-18 16:24 ` Dario Faggioli
2012-01-26 16:37 ` George Dunlap
1 sibling, 1 reply; 6+ messages in thread
From: Dario Faggioli @ 2012-01-18 16:24 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel@lists.xensource.com, Keir (Xen.org)
[-- Attachment #1.1.1: Type: text/plain, Size: 8097 bytes --]
As in all other paths, locking should be dealt with in the
specific schedulers, not in schedule.c.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
diff -r 15ab61865ecb xen/common/sched_credit.c
--- a/xen/common/sched_credit.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_credit.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/sched_credit2.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_credit2.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/sched_sedf.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_sedf.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/schedule.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/schedule.c Wed Jan 18 15:02:30 2012 +0000
@@ -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);
}
}
--
<<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)
[-- Attachment #1.1.2: rework-locking-for-dump-status.patch --]
[-- Type: text/x-patch, Size: 7879 bytes --]
# 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 <dario.faggioli@citrix.com>
diff -r 15ab61865ecb xen/common/sched_credit.c
--- a/xen/common/sched_credit.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_credit.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/sched_credit2.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_credit2.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/sched_sedf.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/sched_sedf.c Wed Jan 18 15:02:30 2012 +0000
@@ -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
--- a/xen/common/schedule.c Tue Jan 17 12:40:52 2012 +0000
+++ b/xen/common/schedule.c Wed Jan 18 15:02:30 2012 +0000
@@ -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);
}
}
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0 of 1] sched: rework locking for dump debugkey.
2012-01-18 16:22 ` Jan Beulich
@ 2012-01-18 16:28 ` Dario Faggioli
0 siblings, 0 replies; 6+ messages in thread
From: Dario Faggioli @ 2012-01-18 16:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, xen-devel@lists.xensource.com, Keir (Xen.org)
[-- Attachment #1.1: Type: text/plain, Size: 843 bytes --]
On Wed, 2012-01-18 at 16:22 +0000, Jan Beulich wrote:
> > 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
--
<<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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 1] sched: rework locking for dump debugkey.
2012-01-18 16:24 ` [PATCH 1 " Dario Faggioli
@ 2012-01-26 16:37 ` George Dunlap
2012-01-26 16:55 ` Dario Faggioli
0 siblings, 1 reply; 6+ messages in thread
From: George Dunlap @ 2012-01-26 16:37 UTC (permalink / raw)
To: Dario Faggioli
Cc: George Dunlap, xen-devel@lists.xensource.com, Keir (Xen.org)
On Wed, 2012-01-18 at 16:24 +0000, Dario Faggioli wrote:
> 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
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> diff -r 15ab61865ecb xen/common/sched_credit.c
> --- a/xen/common/sched_credit.c Tue Jan 17 12:40:52 2012 +0000
> +++ b/xen/common/sched_credit.c Wed Jan 18 15:02:30 2012 +0000
> @@ -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
> --- a/xen/common/sched_credit2.c Tue Jan 17 12:40:52 2012 +0000
> +++ b/xen/common/sched_credit2.c Wed Jan 18 15:02:30 2012 +0000
> @@ -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
> --- a/xen/common/sched_sedf.c Tue Jan 17 12:40:52 2012 +0000
> +++ b/xen/common/sched_sedf.c Wed Jan 18 15:02:30 2012 +0000
> @@ -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
> --- a/xen/common/schedule.c Tue Jan 17 12:40:52 2012 +0000
> +++ b/xen/common/schedule.c Wed Jan 18 15:02:30 2012 +0000
> @@ -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);
> }
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 1] sched: rework locking for dump debugkey.
2012-01-26 16:37 ` George Dunlap
@ 2012-01-26 16:55 ` Dario Faggioli
0 siblings, 0 replies; 6+ messages in thread
From: Dario Faggioli @ 2012-01-26 16:55 UTC (permalink / raw)
To: George Dunlap
Cc: George Dunlap, xen-devel@lists.xensource.com, Keir (Xen.org)
[-- Attachment #1.1: Type: text/plain, Size: 765 bytes --]
On Thu, 2012-01-26 at 16:37 +0000, George Dunlap wrote:
> On Wed, 2012-01-18 at 16:24 +0000, Dario Faggioli wrote:
> > 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. :-)
> 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
--
<<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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-26 16:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-18 16:13 [PATCH 0 of 1] sched: rework locking for dump debugkey Dario Faggioli
2012-01-18 16:22 ` Jan Beulich
2012-01-18 16:28 ` Dario Faggioli
2012-01-18 16:24 ` [PATCH 1 " Dario Faggioli
2012-01-26 16:37 ` George Dunlap
2012-01-26 16:55 ` Dario Faggioli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).