[PATCH v2 0/5] Improving dumping of scheduler related info

[PATCH v2 0/5] Improving dumping of scheduler related info

[Dario Faggioli]

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(-)

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

[Dario Faggioli]

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 <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Meng Xu <xumengpanda@gmail.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
---
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(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index ffc5107..2b0b7c6 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -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);
 }

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

[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 <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Meng Xu <xumengpanda@gmail.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>
---
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(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index bec67ff..953ecb0 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -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
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index be6859a..ae9b359 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -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)
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 2b0b7c6..7c39a9e 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -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
diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
index c4f4b60..a1a4cb7 100644
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -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);
 }
 
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index ef79847..30eec0f 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -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);
     }
 }

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

[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 <dario.faggioli@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
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(+)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index cd6aab9..812a2f9 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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);
     }

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

[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 <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_credit2.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index ae9b359..8aa1438 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -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)

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

[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 <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Meng Xu <xumengpanda@gmail.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
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(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 7c39a9e..ec28956 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -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 *

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

[Meng Xu]

2015-03-17 11:33 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
> 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 <dario.faggioli@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <xumengpanda@gmail.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> ---
> Changes from v1:
>  * improved changelog;
>  * made a local variable to point to the correct
>    scratch mask, as suggested during review.
> ---

Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>

Thanks,

Best,

Meng

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

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

[Juergen Gross]

On 03/17/2015 04:33 PM, Dario Faggioli wrote:
> 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 <dario.faggioli@citrix.com>

Acked-by: Juergen Gross <jgross@suse.com>

> Cc: Juergen Gross <JGross@suse.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> ---
> 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(+)
>
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index cd6aab9..812a2f9 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -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);
>       }
>
>
>

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

[Jan Beulich]

>>> On 17.03.15 at 16:33, <dario.faggioli@citrix.com> wrote:
> @@ -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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 605 bytes --]

On Wed, 2015-03-18 at 11:03 +0000, Jan Beulich wrote:
> >>> On 17.03.15 at 16:33, <dario.faggioli@citrix.com> wrote:
> > @@ -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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[George Dunlap]

On 03/17/2015 03:33 PM, Dario Faggioli wrote:
> 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 <dario.faggioli@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <xumengpanda@gmail.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>
> ---
> 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(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index bec67ff..953ecb0 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -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
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index be6859a..ae9b359 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -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)
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 2b0b7c6..7c39a9e 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -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
> diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
> index c4f4b60..a1a4cb7 100644
> --- a/xen/common/sched_sedf.c
> +++ b/xen/common/sched_sedf.c
> @@ -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);
>  }

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 <george.dunlap@eu.citrix.com>

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

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

[George Dunlap]

On 03/17/2015 03:33 PM, Dario Faggioli wrote:
> 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 <dario.faggioli@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <xumengpanda@gmail.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> ---
> 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 <george.dunlap@eu.citrix.com>

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1429 bytes --]

On Mon, 2015-03-30 at 14:47 +0100, George Dunlap wrote:
> On 03/17/2015 03:33 PM, Dario Faggioli wrote:

> > 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 <dario.faggioli@citrix.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Meng Xu <xumengpanda@gmail.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: Keir Fraser <keir@xen.org>
> > ---
> > 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 <george.dunlap@eu.citrix.com>
> 
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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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