Improving dumping of scheduler related info

PATCH Superseded Dario Faggioli 5 participants 13 messages Mar 17, 2015 – Mar 31, 2015

Share & tools

Dario Faggioli

author

2015-03-17 15:32 UTC #0 lore raw

v2 cover Improving dumping of scheduler related info

Take 2. Some of the patches have been checked-in already, so here's what's remaining:
- fix a bug in the RTDS scheduler (patch 1),
- improve how the whole process of dumping scheduling info is serialized, by moving all locking code into specific schedulers (patch 2),
- print more useful scheduling related information (patches 3, 4 and 5).

Git branch here:

git://xenbits.xen.org/people/dariof/xen.git rel/sched/dump-v2
http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/sched/dump-v2

I think I addressed all the comments raised upon v1. More details in the changelogs of the various patches.

Thanks and Regards,
Dario

---
Dario Faggioli (5):
xen: sched_rt: avoid ASSERT()ing on runq dump if there are no domains
xen: rework locking for dump of scheduler info (debug-key r)
xen: print online pCPUs and free pCPUs when dumping
xen: sched_credit2: more info when dumping
xen: sched_rt: print useful affinity info when dumping

 xen/common/cpupool.c       |   12 +++++++++
 xen/common/sched_credit.c  |   42 ++++++++++++++++++++++++++++++-
 xen/common/sched_credit2.c |   53 +++++++++++++++++++++++++++++++++-------
 xen/common/sched_rt.c      |   59 ++++++++++++++++++++++++++++++++------------
 xen/common/sched_sedf.c    |   16 ++++++++++++
 xen/common/schedule.c      |    5 +---
 6 files changed, 157 insertions(+), 30 deletions(-)

Dario Faggioli

author

2015-03-17 15:32 UTC #1 lore raw

PATCH v2 1/5 xen: sched_rt: avoid ASSERT()ing on runq dump if there are no domains

being serviced by the RTDS scheduler, as that is a legit situation to be in: think, for instance, of a newly created RTDS cpupool, with no domains migrated to it yet.

While there:
- move the spinlock acquisition up, to effectively protect the domain list and avoid races;
- the mask of online pCPUs was being retrieved but then not used anywhere in the function: get rid of that.

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

Cc George Dunlap <[email protected]>

Cc Meng Xu <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

Reviewed-by Meng Xu <[email protected]>

Acked-by George Dunlap <[email protected]>

---
Changes from v1:
* updated the changelog as requested during review;
* fixed coding style, as requested during review;
* fixed label indentation, as requested during review.

---
 xen/common/sched_rt.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

xen/common/sched_rt.c +5 -5

index ffc5107..2b0b7c6 100644
@@ -264,18 +264,17 @@ rt_dump(const struct scheduler *ops)
     struct list_head *iter_sdom, *iter_svc, *runq, *depletedq, *iter;
     struct rt_private *prv = rt_priv(ops);
     struct rt_vcpu *svc;
-    cpumask_t *online;
     struct rt_dom *sdom;
     unsigned long flags;
 
-    ASSERT(!list_empty(&prv->sdom));
+    spin_lock_irqsave(&prv->lock, flags);
+
+    if ( list_empty(&prv->sdom) )
+        goto out;
 
-    sdom = list_entry(prv->sdom.next, struct rt_dom, sdom_elem);
-    online = cpupool_scheduler_cpumask(sdom->dom->cpupool);
     runq = rt_runq(ops);
     depletedq = rt_depletedq(ops);
 
-    spin_lock_irqsave(&prv->lock, flags);
     printk("Global RunQueue info:\n");
     list_for_each( iter, runq )
     {

@@ -303,6 +302,7 @@ rt_dump(const struct scheduler *ops)
         }
     }
 
+ out:
     spin_unlock_irqrestore(&prv->lock, flags);
 }

Dario Faggioli

author

2015-03-17 15:33 UTC #2 lore raw

PATCH v2 2/5 xen: rework locking for dump of scheduler info (debug-key r)

In reply to Dario Faggioli

such as it is taken care of by the various schedulers, rather than happening in schedule.c. In fact, it is the schedulers that know better which locks are necessary for the specific dumping operations.

While there, fix a few style issues (indentation, trailing whitespace, parentheses and blank line after var declarations)

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

Cc George Dunlap <[email protected]>

Cc Meng Xu <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

Reviewed-by Meng Xu <[email protected]>

---
Changes from v1:
* take care of SEDF too, as requested during review;
---
As far as tags are concerned, I kept Meng's 'Reviewed-by', as I think this applies mostly to chenges to sched_rt.c. I, OTOH, dropped George's one, to give him the chance to look at changes to sched_sedf.c.

---
 xen/common/sched_credit.c  |   42 ++++++++++++++++++++++++++++++++++++++++--
 xen/common/sched_credit2.c |   40 ++++++++++++++++++++++++++++++++--------
 xen/common/sched_rt.c      |    7 +++++--
 xen/common/sched_sedf.c    |   16 ++++++++++++++++
 xen/common/schedule.c      |    5 ++---
 5 files changed, 95 insertions(+), 15 deletions(-)

xen/common/sched_credit.c +40 -2

index bec67ff..953ecb0 100644
@@ -26,6 +26,23 @@
 
 
 /*
+ * Locking:
+ * - Scheduler-lock (a.k.a. runqueue lock):
+ *  + is per-runqueue, and there is one runqueue per-cpu;
+ *  + serializes all runqueue manipulation operations;
+ * - Private data lock (a.k.a. private scheduler lock):
+ *  + serializes accesses to the scheduler global state (weight,
+ *    credit, balance_credit, etc);
+ *  + serializes updates to the domains' scheduling parameters.
+ *
+ * Ordering is "private lock always comes first":
+ *  + if we need both locks, we must acquire the private
+ *    scheduler lock for first;
+ *  + if we already own a runqueue lock, we must never acquire
+ *    the private scheduler lock.
+ */
+
+/*
  * Basic constants
  */
 #define CSCHED_DEFAULT_WEIGHT       256

@@ -1750,11 +1767,24 @@ 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;
+    spinlock_t *lock = lock;
+    unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
 
+    /*
+     * We need both locks:
+     * - csched_dump_vcpu() wants to access domains' scheduling
+     *   parameters, which are protected by the private scheduler lock;
+     * - we scan through the runqueue, so we need the proper runqueue
+     *   lock (the one of the runqueue of this cpu).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = pcpu_schedule_lock(cpu);
+
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;

@@ -1781,6 +1811,9 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
             csched_dump_vcpu(svc);
         }
     }
+
+    pcpu_schedule_unlock(lock, cpu);
+    spin_unlock_irqrestore(&prv->lock, flags);
 #undef cpustr
 }

@@ -1792,7 +1825,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

@@ -1835,15 +1868,20 @@ csched_dump(const struct scheduler *ops)
         list_for_each( iter_svc, &sdom->active_vcpu )
         {
             struct csched_vcpu *svc;
+            spinlock_t *lock;
+
             svc = list_entry(iter_svc, struct csched_vcpu, active_vcpu_elem);
+            lock = vcpu_schedule_lock(svc->vcpu);
 
             printk("\t%3d: ", ++loop);
             csched_dump_vcpu(svc);
+
+            vcpu_schedule_unlock(lock, svc->vcpu);
         }
     }
 #undef idlers_buf
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static int

xen/common/sched_credit2.c +32 -8

index be6859a..ae9b359 100644
@@ -51,8 +51,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

@@ -1832,12 +1830,24 @@ csched2_dump_vcpu(struct csched2_vcpu *svc)
 static void
 csched2_dump_pcpu(const struct scheduler *ops, int cpu)
 {
+    struct csched2_private *prv = CSCHED2_PRIV(ops);
     struct list_head *runq, *iter;
     struct csched2_vcpu *svc;
+    unsigned long flags;
+    spinlock_t *lock;
     int loop;
     char cpustr[100];
 
-    /* FIXME: Do locking properly for access to runqueue structures */
+    /*
+     * We need both locks:
+     * - csched2_dump_vcpu() wants to access domains' scheduling
+     *   parameters, which are protected by the private scheduler lock;
+     * - we scan through the runqueue, so we need the proper runqueue
+     *   lock (the one of the runqueue this cpu is associated to).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = per_cpu(schedule_data, cpu).schedule_lock;
+    spin_lock(lock);
 
     runq = &RQD(ops, cpu)->runq;

@@ -1864,6 +1874,9 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
             csched2_dump_vcpu(svc);
         }
     }
+
+    spin_unlock(lock);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

@@ -1871,8 +1884,13 @@ csched2_dump(const struct scheduler *ops)
 {
     struct list_head *iter_sdom, *iter_svc;
     struct csched2_private *prv = CSCHED2_PRIV(ops);
+    unsigned long flags;
     int i, loop;
 
+    /* We need the private lock as we access global scheduler data
+     * and (below) the list of active domains. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     printk("Active queues: %d\n"
            "\tdefault-weight     = %d\n",
            cpumask_weight(&prv->active_queues),

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

@@ -1904,20 +1921,27 @@ csched2_dump(const struct scheduler *ops)
         struct csched2_dom *sdom;
         sdom = list_entry(iter_sdom, struct csched2_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 )
         {
             struct csched2_vcpu *svc;
+            spinlock_t *lock;
+
             svc = list_entry(iter_svc, struct csched2_vcpu, sdom_elem);
+            lock = vcpu_schedule_lock(svc->vcpu);
 
             printk("\t%3d: ", ++loop);
             csched2_dump_vcpu(svc);
+
+            vcpu_schedule_unlock(lock, svc->vcpu);
         }
     }
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void activate_runqueue(struct csched2_private *prv, int rqi)

xen/common/sched_rt.c +5 -2

index 2b0b7c6..7c39a9e 100644
@@ -253,9 +253,12 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
 static void
 rt_dump_pcpu(const struct scheduler *ops, int cpu)
 {
-    struct rt_vcpu *svc = rt_vcpu(curr_on_cpu(cpu));
+    struct rt_private *prv = rt_priv(ops);
+    unsigned long flags;
 
-    rt_dump_vcpu(ops, svc);
+    spin_lock_irqsave(&prv->lock, flags);
+    rt_dump_vcpu(ops, rt_vcpu(curr_on_cpu(cpu)));
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

xen/common/sched_sedf.c +16 -0

index c4f4b60..a1a4cb7 100644
@@ -1206,12 +1206,25 @@ static void sedf_dump_domain(struct vcpu *d)
 /* 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;
+    spinlock_t *lock;
+    unsigned long flags;
     int loop = 0;
  
+    /*
+     * We need both locks, as:
+     * - we access domains' parameters, which are protected by the
+     *   private scheduler lock;
+     * - we scan through the various queues, so we need the proper
+     *   runqueue lock (i.e., the one for this pCPU).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = pcpu_schedule_lock(i);
+
     printk("now=%"PRIu64"\n",NOW());
     queue = RUNQ(i);
     printk("RUNQ rq %lx   n: %lx, p: %lx\n",  (unsigned long)queue,

@@ -1275,6 +1288,9 @@ static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    pcpu_schedule_unlock(lock, i);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

xen/common/schedule.c +2 -3

index ef79847..30eec0f 100644
@@ -1515,6 +1515,8 @@ void schedule_dump(struct cpupool *c)
     struct scheduler *sched;
     cpumask_t        *cpus;
 
+    /* Locking, if necessary, must be handled withing each scheduler */
+
     sched = (c == NULL) ? &ops : c->sched;
     cpus = cpupool_scheduler_cpumask(c);
     printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name);

@@ -1522,11 +1524,8 @@ void schedule_dump(struct cpupool *c)
 
     for_each_cpu (i, cpus)
     {
-        spinlock_t *lock = pcpu_schedule_lock(i);
-
         printk("CPU[%02d] ", i);
         SCHED_OP(sched, dump_cpu_state, i);
-        pcpu_schedule_unlock(lock, i);
     }
 }

George Dunlap

Reviewed-by on v2 2/5

2015-03-30 13:34 UTC #3 lore raw

Re: [PATCH v2 2/5] xen: rework locking for dump of scheduler info (debug-key r)

Quoted 276 lines · Dario Faggioli

While there, fix a few style issues (indentation, trailing whitespace, parentheses and blank line after var declarations)

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

Cc George Dunlap <[email protected]>

Cc Meng Xu <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

Reviewed-by Meng Xu <[email protected]>

---
 xen/common/sched_credit.c  |   42 ++++++++++++++++++++++++++++++++++++++++--
 xen/common/sched_credit2.c |   40 ++++++++++++++++++++++++++++++++--------
 xen/common/sched_rt.c      |    7 +++++--
 xen/common/sched_sedf.c    |   16 ++++++++++++++++
 xen/common/schedule.c      |    5 ++---
 5 files changed, 95 insertions(+), 15 deletions(-)

xen/common/sched_credit.c +40 -2

index bec67ff..953ecb0 100644
@@ -26,6 +26,23 @@
 
 
 /*
+ * Locking:
+ * - Scheduler-lock (a.k.a. runqueue lock):
+ *  + is per-runqueue, and there is one runqueue per-cpu;
+ *  + serializes all runqueue manipulation operations;
+ * - Private data lock (a.k.a. private scheduler lock):
+ *  + serializes accesses to the scheduler global state (weight,
+ *    credit, balance_credit, etc);
+ *  + serializes updates to the domains' scheduling parameters.
+ *
+ * Ordering is "private lock always comes first":
+ *  + if we need both locks, we must acquire the private
+ *    scheduler lock for first;
+ *  + if we already own a runqueue lock, we must never acquire
+ *    the private scheduler lock.
+ */
+
+/*
  * Basic constants
  */
 #define CSCHED_DEFAULT_WEIGHT       256

@@ -1750,11 +1767,24 @@ 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;
+    spinlock_t *lock = lock;
+    unsigned long flags;
     int loop;
 #define cpustr keyhandler_scratch
 
+    /*
+     * We need both locks:
+     * - csched_dump_vcpu() wants to access domains' scheduling
+     *   parameters, which are protected by the private scheduler lock;
+     * - we scan through the runqueue, so we need the proper runqueue
+     *   lock (the one of the runqueue of this cpu).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = pcpu_schedule_lock(cpu);
+
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;

@@ -1781,6 +1811,9 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
             csched_dump_vcpu(svc);
         }
     }
+
+    pcpu_schedule_unlock(lock, cpu);
+    spin_unlock_irqrestore(&prv->lock, flags);
 #undef cpustr
 }

@@ -1792,7 +1825,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

@@ -1835,15 +1868,20 @@ csched_dump(const struct scheduler *ops)
         list_for_each( iter_svc, &sdom->active_vcpu )
         {
             struct csched_vcpu *svc;
+            spinlock_t *lock;
+
             svc = list_entry(iter_svc, struct csched_vcpu, active_vcpu_elem);
+            lock = vcpu_schedule_lock(svc->vcpu);
 
             printk("\t%3d: ", ++loop);
             csched_dump_vcpu(svc);
+
+            vcpu_schedule_unlock(lock, svc->vcpu);
         }
     }
 #undef idlers_buf
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static int

xen/common/sched_credit2.c +32 -8

index be6859a..ae9b359 100644
@@ -51,8 +51,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

@@ -1832,12 +1830,24 @@ csched2_dump_vcpu(struct csched2_vcpu *svc)
 static void
 csched2_dump_pcpu(const struct scheduler *ops, int cpu)
 {
+    struct csched2_private *prv = CSCHED2_PRIV(ops);
     struct list_head *runq, *iter;
     struct csched2_vcpu *svc;
+    unsigned long flags;
+    spinlock_t *lock;
     int loop;
     char cpustr[100];
 
-    /* FIXME: Do locking properly for access to runqueue structures */
+    /*
+     * We need both locks:
+     * - csched2_dump_vcpu() wants to access domains' scheduling
+     *   parameters, which are protected by the private scheduler lock;
+     * - we scan through the runqueue, so we need the proper runqueue
+     *   lock (the one of the runqueue this cpu is associated to).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = per_cpu(schedule_data, cpu).schedule_lock;
+    spin_lock(lock);
 
     runq = &RQD(ops, cpu)->runq;

@@ -1864,6 +1874,9 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
             csched2_dump_vcpu(svc);
         }
     }
+
+    spin_unlock(lock);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

@@ -1871,8 +1884,13 @@ csched2_dump(const struct scheduler *ops)
 {
     struct list_head *iter_sdom, *iter_svc;
     struct csched2_private *prv = CSCHED2_PRIV(ops);
+    unsigned long flags;
     int i, loop;
 
+    /* We need the private lock as we access global scheduler data
+     * and (below) the list of active domains. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     printk("Active queues: %d\n"
            "\tdefault-weight     = %d\n",
            cpumask_weight(&prv->active_queues),

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

@@ -1904,20 +1921,27 @@ csched2_dump(const struct scheduler *ops)
         struct csched2_dom *sdom;
         sdom = list_entry(iter_sdom, struct csched2_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 )
         {
             struct csched2_vcpu *svc;
+            spinlock_t *lock;
+
             svc = list_entry(iter_svc, struct csched2_vcpu, sdom_elem);
+            lock = vcpu_schedule_lock(svc->vcpu);
 
             printk("\t%3d: ", ++loop);
             csched2_dump_vcpu(svc);
+
+            vcpu_schedule_unlock(lock, svc->vcpu);
         }
     }
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void activate_runqueue(struct csched2_private *prv, int rqi)

xen/common/sched_rt.c +5 -2

index 2b0b7c6..7c39a9e 100644
@@ -253,9 +253,12 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
 static void
 rt_dump_pcpu(const struct scheduler *ops, int cpu)
 {
-    struct rt_vcpu *svc = rt_vcpu(curr_on_cpu(cpu));
+    struct rt_private *prv = rt_priv(ops);
+    unsigned long flags;
 
-    rt_dump_vcpu(ops, svc);
+    spin_lock_irqsave(&prv->lock, flags);
+    rt_dump_vcpu(ops, rt_vcpu(curr_on_cpu(cpu)));
+    spin_unlock_irqrestore(&prv->lock, flags);
 }
 
 static void

xen/common/sched_sedf.c +13 -0

index c4f4b60..a1a4cb7 100644
@@ -1206,12 +1206,25 @@ static void sedf_dump_domain(struct vcpu *d)
 /* 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;
+    spinlock_t *lock;
+    unsigned long flags;
     int loop = 0;
 
+    /*
+     * We need both locks, as:
+     * - we access domains' parameters, which are protected by the
+     *   private scheduler lock;
+     * - we scan through the various queues, so we need the proper
+     *   runqueue lock (i.e., the one for this pCPU).
+     */
+    spin_lock_irqsave(&prv->lock, flags);
+    lock = pcpu_schedule_lock(i);
+
     printk("now=%"PRIu64"\n",NOW());
     queue = RUNQ(i);
     printk("RUNQ rq %lx   n: %lx, p: %lx\n",  (unsigned long)queue,

xen/common/sched_sedf.c static void sedf_dump_cpu_state(const struct scheduler *ops, int i) · line 1294

Quoted 7 lines

xen/common/sched_sedf.c +3 -0

@@ -1275,6 +1288,9 @@ static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    pcpu_schedule_unlock(lock, i);
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

We're grabbing the private lock to protect reading the domain-related data, but AFAICT the only other place the lock is taken is when the data is being *written* (in sched_adjust()).

Well anyway; you're making it more correct than it was, and you're fixing the regression in v1 of the patch, so I think this is fine. :-)

Reviewed-by George Dunlap <[email protected]>

(Also, I have no idea how it got to be nearly 2 weeks without reviewing this one...)

Dario Faggioli

author

2015-03-17 15:33 UTC #4 lore raw

PATCH v2 3/5 xen: print online pCPUs and free pCPUs when dumping

In reply to Dario Faggioli

e.g., with `xl debug-key r', like this:

(XEN) Online Cpus: 0-15
(XEN) Free Cpus: 8-15

Also, for each cpupool, print the set of pCPUs it contains, like this:

(XEN) Cpupool 0:
(XEN) Cpus: 0-7
(XEN) Scheduler: SMP Credit Scheduler (credit)

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

Cc Juergen Gross <[email protected]>

Cc George Dunlap <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

---
Changes from v1:
* _print_cpumap() becomes print_cpumap() (i.e., the leading '_' was not particularly useful in this case), as suggested during review
* changed the output such as (1) we only print the maps, not the number of elements, and (2) we avoid printing the free cpus map when empty
* improved the changelog
---
I'm not including any Reviewed-by / Acked-by tag, since the patch changed.

---
 xen/common/cpupool.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

xen/common/cpupool.c +12 -0

index cd6aab9..812a2f9 100644
@@ -17,6 +17,7 @@
 #include <xen/percpu.h>
 #include <xen/sched.h>
 #include <xen/sched-if.h>
+#include <xen/keyhandler.h>
 #include <xen/cpu.h>
 
 #define for_each_cpupool(ptr)    \

@@ -658,6 +659,12 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
     return ret;
 }
 
+static void print_cpumap(const char *str, const cpumask_t *map)
+{
+    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), map);
+    printk("%s: %s\n", str, keyhandler_scratch);
+}
+
 void dump_runq(unsigned char key)
 {
     unsigned long    flags;

@@ -671,12 +678,17 @@ void dump_runq(unsigned char key)
             sched_smt_power_savings? "enabled":"disabled");
     printk("NOW=0x%08X%08X\n",  (u32)(now>>32), (u32)now);
 
+    print_cpumap("Online Cpus", &cpu_online_map);
+    if ( cpumask_weight(&cpupool_free_cpus) )
+        print_cpumap("Free Cpus", &cpupool_free_cpus);
+
     printk("Idle cpupool:\n");
     schedule_dump(NULL);
 
     for_each_cpupool(c)
     {
         printk("Cpupool %d:\n", (*c)->cpupool_id);
+        print_cpumap("Cpus", (*c)->cpu_valid);
         schedule_dump(*c);
     }

Juergen Gross

Acked-by on v2 3/5

2015-03-18 04:29 UTC #5 lore raw

Re: [PATCH v2 3/5] xen: print online pCPUs and free pCPUs when dumping

Quoted 5 lines · Dario Faggioli

e.g., with `xl debug-key r', like this:
   (XEN) Online Cpus: 0-15
   (XEN) Free Cpus: 8-15

Quoted 8 lines

Also, for each cpupool, print the set of pCPUs it
contains, like this:
(XEN) Cpupool 0:
(XEN) Cpus: 0-7
(XEN) Scheduler: SMP Credit Scheduler (credit)
Signed-off-by Dario Faggioli <[email protected]>

Acked-by Juergen Gross <[email protected]>

Quoted 65 lines

Cc Juergen Gross <[email protected]>

Cc George Dunlap <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

---
  xen/common/cpupool.c |   12 ++++++++++++
  1 file changed, 12 insertions(+)

xen/common/cpupool.c +12 -0

index cd6aab9..812a2f9 100644
@@ -17,6 +17,7 @@
  #include <xen/percpu.h>
  #include <xen/sched.h>
  #include <xen/sched-if.h>
+#include <xen/keyhandler.h>
  #include <xen/cpu.h>
 
  #define for_each_cpupool(ptr)    \

@@ -658,6 +659,12 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
      return ret;
  }
 
+static void print_cpumap(const char *str, const cpumask_t *map)
+{
+    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), map);
+    printk("%s: %s\n", str, keyhandler_scratch);
+}
+
  void dump_runq(unsigned char key)
  {
      unsigned long    flags;

@@ -671,12 +678,17 @@ void dump_runq(unsigned char key)
              sched_smt_power_savings? "enabled":"disabled");
      printk("NOW=0x%08X%08X\n",  (u32)(now>>32), (u32)now);
 
+    print_cpumap("Online Cpus", &cpu_online_map);
+    if ( cpumask_weight(&cpupool_free_cpus) )
+        print_cpumap("Free Cpus", &cpupool_free_cpus);
+
      printk("Idle cpupool:\n");
      schedule_dump(NULL);
 
      for_each_cpupool(c)
      {
          printk("Cpupool %d:\n", (*c)->cpupool_id);
+        print_cpumap("Cpus", (*c)->cpu_valid);
          schedule_dump(*c);
      }

Jan Beulich

2015-03-18 11:03 UTC #6 lore raw

Re: [PATCH v2 3/5] xen: print online pCPUs and free pCPUs when dumping

In reply to Dario Faggioli

xen/common/cpupool.c void dump_runq(unsigned char key) · line 682

Quoted 7 lines

Quoted 1 line

Quoted 1 line · On 17.03.15 at 16:33, <[email protected]> wrote:

xen/common/cpupool.c +2 -0

@@ -671,12 +678,17 @@ void dump_runq(unsigned char key)
             sched_smt_power_savings? "enabled":"disabled");
     printk("NOW=0x%08X%08X\n",  (u32)(now>>32), (u32)now);
 
+    print_cpumap("Online Cpus", &cpu_online_map);
+    if ( cpumask_weight(&cpupool_free_cpus) )

This is certainly more expensive than necessary - mind if I change this to !cpumask_empty() on commit?

Jan

Dario Faggioli

author

2015-03-18 11:26 UTC #7 lore raw

Re: [PATCH v2 3/5] xen: print online pCPUs and free pCPUs when dumping

Quoted 11 lines · Jan Beulich

xen/common/cpupool.c void dump_runq(unsigned char key) · line 682
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 17.03.15 at 16:33, <[email protected]> wrote:
xen/common/cpupool.c +2 -0
@@ -671,12 +678,17 @@ void dump_runq(unsigned char key)
             sched_smt_power_savings? "enabled":"disabled");
     printk("NOW=0x%08X%08X\n",  (u32)(now>>32), (u32)now);
 
+    print_cpumap("Online Cpus", &cpu_online_map);
+    if ( cpumask_weight(&cpupool_free_cpus) )
This is certainly more expensive than necessary - mind if I change this to !cpumask_empty() on commit?

Go ahead... That is indeed what I meant, sorry! :-)

Dario

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

Dario Faggioli

author

2015-03-17 15:33 UTC #8 lore raw

PATCH v2 4/5 xen: sched_credit2: more info when dumping

In reply to Dario Faggioli

more specifically, for each runqueue, print what pCPUs belong to it, which ones are idle and which ones have been tickled.

While there, also convert the whole file to use keyhandler_scratch for printing cpumask-s.

Signed-off-b: Dario Faggioli <[email protected]>

Cc George Dunlap <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

Reviewed-by George Dunlap <[email protected]>

---
 xen/common/sched_credit2.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

xen/common/sched_credit2.c +12 -1

index ae9b359..8aa1438 100644
@@ -25,6 +25,7 @@
 #include <xen/errno.h>
 #include <xen/trace.h>
 #include <xen/cpu.h>
+#include <xen/keyhandler.h>
 
 #define d2printk(x...)
 //#define d2printk printk

@@ -1836,7 +1837,7 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
     unsigned long flags;
     spinlock_t *lock;
     int loop;
-    char cpustr[100];
+#define cpustr keyhandler_scratch
 
     /*
      * We need both locks:

@@ -1877,6 +1878,7 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
 
     spin_unlock(lock);
     spin_unlock_irqrestore(&prv->lock, flags);
+#undef cpustr
 }
 
 static void

@@ -1886,6 +1888,7 @@ csched2_dump(const struct scheduler *ops)
     struct csched2_private *prv = CSCHED2_PRIV(ops);
     unsigned long flags;
     int i, loop;
+#define cpustr keyhandler_scratch
 
     /* We need the private lock as we access global scheduler data
      * and (below) the list of active domains. */

@@ -1901,17 +1904,24 @@ csched2_dump(const struct scheduler *ops)
         
         fraction = prv->rqd[i].avgload * 100 / (1ULL<<prv->load_window_shift);
 
+        cpulist_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].active);
         printk("Runqueue %d:\n"
                "\tncpus              = %u\n"
+               "\tcpus               = %s\n"
                "\tmax_weight         = %d\n"
                "\tinstload           = %d\n"
                "\taveload            = %3"PRI_stime"\n",
                i,
                cpumask_weight(&prv->rqd[i].active),
+               cpustr,
                prv->rqd[i].max_weight,
                prv->rqd[i].load,
                fraction);
 
+        cpumask_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].idle);
+        printk("\tidlers: %s\n", cpustr);
+        cpumask_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].tickled);
+        printk("\ttickled: %s\n", cpustr);
     }
 
     printk("Domain info:\n");

@@ -1942,6 +1952,7 @@ csched2_dump(const struct scheduler *ops)
     }
 
     spin_unlock_irqrestore(&prv->lock, flags);
+#undef cpustr
 }
 
 static void activate_runqueue(struct csched2_private *prv, int rqi)

Dario Faggioli

author

2015-03-17 15:33 UTC #9 lore raw

PATCH v2 5/5 xen: sched_rt: print useful affinity info when dumping

In reply to Dario Faggioli

In fact, printing the cpupool's CPU online mask for each vCPU is just redundant, as that is the same for all the vCPUs of all the domains in the same cpupool, while hard affinity is already part of the output of dumping domains info.

Instead, print the intersection between hard affinity and online CPUs, which is --in case of this scheduler-- the effective affinity always used for the vCPUs.

This change also takes the chance to add a scratch cpumask area, to avoid having to either put one (more) cpumask_t on the stack, or dynamically allocate it within the dumping routine. (The former being bad because hypervisor stack size is limited, the latter because dynamic allocations can fail, if the hypervisor was built for a large enough number of CPUs.)

Such scratch area can be used to kill most of the cpumasks{_var}_t local variables in other functions in the file, but that is *NOT* done in this chage.

Finally, convert the file to use keyhandler scratch, instead of open coded string buffers.

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

Cc George Dunlap <[email protected]>

Cc Meng Xu <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Keir Fraser <[email protected]>

---
Changes from v1:
* improved changelog;
* made a local variable to point to the correct scratch mask, as suggested during review.

---
 xen/common/sched_rt.c |   42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

xen/common/sched_rt.c +33 -9

index 7c39a9e..ec28956 100644
@@ -124,6 +124,12 @@
 #define TRC_RTDS_BUDGET_REPLENISH TRC_SCHED_CLASS_EVT(RTDS, 4)
 #define TRC_RTDS_SCHED_TASKLET    TRC_SCHED_CLASS_EVT(RTDS, 5)
 
+ /*
+  * Useful to avoid too many cpumask_var_t on the stack.
+  */
+static cpumask_t **_cpumask_scratch;
+#define cpumask_scratch _cpumask_scratch[smp_processor_id()]
+
 /*
  * Systme-wide private data, include global RunQueue/DepletedQ
  * Global lock is referenced by schedule_data.schedule_lock from all

@@ -218,8 +224,7 @@ __q_elem(struct list_head *elem)
 static void
 rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
 {
-    char cpustr[1024];
-    cpumask_t *cpupool_mask;
+    cpumask_t *cpupool_mask, *mask;
 
     ASSERT(svc != NULL);
     /* idle vcpu */

@@ -229,10 +234,22 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
         return;
     }
 
-    cpumask_scnprintf(cpustr, sizeof(cpustr), svc->vcpu->cpu_hard_affinity);
+    /*
+     * We can't just use 'cpumask_scratch' because the dumping can
+     * happen from a pCPU outside of this scheduler's cpupool, and
+     * hence it's not right to use the pCPU's scratch mask (which
+     * may even not exist!). On the other hand, it is safe to use
+     * svc->vcpu->processor's own scratch space, since we hold the
+     * runqueue lock.
+     */
+    mask = _cpumask_scratch[svc->vcpu->processor];
+
+    cpupool_mask = cpupool_scheduler_cpumask(svc->vcpu->domain->cpupool);
+    cpumask_and(mask, cpupool_mask, svc->vcpu->cpu_hard_affinity);
+    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), mask);
     printk("[%5d.%-2u] cpu %u, (%"PRI_stime", %"PRI_stime"),"
            " cur_b=%"PRI_stime" cur_d=%"PRI_stime" last_start=%"PRI_stime"\n"
-           " \t\t onQ=%d runnable=%d cpu_hard_affinity=%s ",
+           " \t\t onQ=%d runnable=%d flags=%x effective hard_affinity=%s\n",
             svc->vcpu->domain->domain_id,
             svc->vcpu->vcpu_id,
             svc->vcpu->processor,

@@ -243,11 +260,8 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
             svc->last_start,
             __vcpu_on_q(svc),
             vcpu_runnable(svc->vcpu),
-            cpustr);
-    memset(cpustr, 0, sizeof(cpustr));
-    cpupool_mask = cpupool_scheduler_cpumask(svc->vcpu->domain->cpupool);
-    cpumask_scnprintf(cpustr, sizeof(cpustr), cpupool_mask);
-    printk("cpupool=%s\n", cpustr);
+            svc->flags,
+            keyhandler_scratch);
 }
 
 static void

@@ -409,6 +423,10 @@ rt_init(struct scheduler *ops)
     if ( prv == NULL )
         return -ENOMEM;
 
+    _cpumask_scratch = xmalloc_array(cpumask_var_t, nr_cpu_ids);
+    if ( _cpumask_scratch == NULL )
+        return -ENOMEM;
+
     spin_lock_init(&prv->lock);
     INIT_LIST_HEAD(&prv->sdom);
     INIT_LIST_HEAD(&prv->runq);

@@ -426,6 +444,7 @@ rt_deinit(const struct scheduler *ops)
 {
     struct rt_private *prv = rt_priv(ops);
 
+    xfree(_cpumask_scratch);
     xfree(prv);
 }

@@ -443,6 +462,9 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu)
     per_cpu(schedule_data, cpu).schedule_lock = &prv->lock;
     spin_unlock_irqrestore(&prv->lock, flags);
 
+    if ( !alloc_cpumask_var(&_cpumask_scratch[cpu]) )
+        return NULL;
+
     /* 1 indicates alloc. succeed in schedule.c */
     return (void *)1;
 }

@@ -462,6 +484,8 @@ rt_free_pdata(const struct scheduler *ops, void *pcpu, int cpu)
     sd->schedule_lock = &sd->_lock;
 
     spin_unlock_irqrestore(&prv->lock, flags);
+
+    free_cpumask_var(_cpumask_scratch[cpu]);
 }
 
 static void *

Meng Xu

Reviewed-by on v2 5/5

2015-03-18 01:09 UTC #10 lore raw

Re: [PATCH v2 5/5] xen: sched_rt: print useful affinity info when dumping

2015-03-17 11:33 GMT-04:00 Dario Faggioli <[email protected]>:

Quoted 32 lines

In fact, printing the cpupool's CPU online mask for each vCPU is just redundant, as that is the same for all the vCPUs of all the domains in the same cpupool, while hard affinity is already part of the output of dumping domains info.
Instead, print the intersection between hard affinity and online CPUs, which is --in case of this scheduler-- the effective affinity always used for the vCPUs.
This change also takes the chance to add a scratch cpumask area, to avoid having to either put one (more) cpumask_t on the stack, or dynamically allocate it within the dumping routine. (The former being bad because hypervisor stack size is limited, the latter because dynamic allocations can fail, if the hypervisor was built for a large enough number of CPUs.)
Such scratch area can be used to kill most of the cpumasks{_var}_t local variables in other functions in the file, but that is *NOT* done in this chage.
Finally, convert the file to use keyhandler scratch, instead of open coded string buffers.
Signed-off-by Dario Faggioli <[email protected]>
Cc George Dunlap <[email protected]>
Cc Meng Xu <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Keir Fraser <[email protected]>

Quoted 6 lines

---
Changes from v1:
* improved changelog;
* made a local variable to point to the correct scratch mask, as suggested during review.
---

Reviewed-by Meng Xu <[email protected]>

Thanks,

Best,

Meng

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

George Dunlap

Reviewed-by on v2 5/5

2015-03-30 13:47 UTC #11 lore raw

Re: [PATCH v2 5/5] xen: sched_rt: print useful affinity info when dumping

In reply to Dario Faggioli

Quoted 32 lines · Dario Faggioli

In fact, printing the cpupool's CPU online mask for each vCPU is just redundant, as that is the same for all the vCPUs of all the domains in the same cpupool, while hard affinity is already part of the output of dumping domains info.
Instead, print the intersection between hard affinity and online CPUs, which is --in case of this scheduler-- the effective affinity always used for the vCPUs.
This change also takes the chance to add a scratch cpumask area, to avoid having to either put one (more) cpumask_t on the stack, or dynamically allocate it within the dumping routine. (The former being bad because hypervisor stack size is limited, the latter because dynamic allocations can fail, if the hypervisor was built for a large enough number of CPUs.)
Such scratch area can be used to kill most of the cpumasks{_var}_t local variables in other functions in the file, but that is *NOT* done in this chage.
Finally, convert the file to use keyhandler scratch, instead of open coded string buffers.
Signed-off-by Dario Faggioli <[email protected]>
Cc George Dunlap <[email protected]>
Cc Meng Xu <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Keir Fraser <[email protected]>

Quoted 5 lines

---
Changes from v1:
* improved changelog;
* made a local variable to point to the correct scratch mask, as suggested during review.

Thanks,

Reviewed-by George Dunlap <[email protected]>

Dario Faggioli

author

2015-03-31 16:58 UTC #12 lore raw

Re: [PATCH v2 5/5] xen: sched_rt: print useful affinity info when dumping

Quoted 20 lines · George Dunlap

Quoted 12 lines · Dario Faggioli
Such scratch area can be used to kill most of the cpumasks{_var}_t local variables in other functions in the file, but that is *NOT* done in this chage.
Finally, convert the file to use keyhandler scratch, instead of open coded string buffers.
Signed-off-by Dario Faggioli <[email protected]>
Cc George Dunlap <[email protected]>
Cc Meng Xu <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Keir Fraser <[email protected]>
Quoted 5 lines
---
Changes from v1:
* improved changelog;
* made a local variable to point to the correct scratch mask, as suggested during review.

Quoted 4 lines

Thanks,
Reviewed-by George Dunlap <[email protected]>

Thanks to you, but I think this has an issue.

The same one that you spotted in Justin's Credit2 affinity series: the scratch mask array should not be allocated in rt_init(), or at least not without checking that it's not there already, or the creation of a new cpupool with the same scheduler will screw things up.

So, at least this very patch needs a v3. It's pretty independent from the rest of the series, which should be all acked by now, but I can resend it all if it's easier/better, just let me know. :-)

Thanks again and Regards,
Dario

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