xen: sched_credit: fix picking & tickling and also add some tracing

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/sched_credit.c +10 -4

@@ -59,6 +59,8 @@
 #define CSCHED_VCPU(_vcpu)  ((struct csched_vcpu *) (_vcpu)->sched_priv)
 #define CSCHED_DOM(_dom)    ((struct csched_dom *) (_dom)->sched_priv)
 #define RUNQ(_cpu)          (&(CSCHED_PCPU(_cpu)->runq))
+/* Is the first element of _cpu's runq its idle vcpu? */
+#define IS_RUNQ_IDLE(_cpu)  (is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu))
 
 
 /*

@@ -479,9 +481,14 @@ static int
      * distinct cores first and guarantees we don't do something stupid
      * like run two VCPUs on co-hyperthreads while there are idle cores
      * or sockets.
+     *
+     * Notice that, when computing the "idleness" of cpu, we may want to
+     * discount vc. That is, iff vc is the currently running and the only
+     * runnable vcpu on cpu, we add cpu to the idlers.
      */
     cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
-    cpumask_set_cpu(cpu, &idlers);
+    if ( current_on_cpu(cpu) == vc && IS_RUNQ_IDLE(cpu) )
+        cpumask_set_cpu(cpu, &idlers);
     cpumask_and(&cpus, &cpus, &idlers);
     cpumask_clear_cpu(cpu, &cpus);
 

@@ -489,7 +496,7 @@ static int
     {
         cpumask_t cpu_idlers;
         cpumask_t nxt_idlers;
-        int nxt, weight_cpu, weight_nxt;
+        int nxt, nr_idlers_cpu, nr_idlers_nxt;
         int migrate_factor;
 
         nxt = cpumask_cycle(cpu, &cpus);

@@ -513,12 +520,12 @@ static int
             cpumask_and(&nxt_idlers, &idlers, per_cpu(cpu_core_mask, nxt));
         }
 
-        weight_cpu = cpumask_weight(&cpu_idlers);
-        weight_nxt = cpumask_weight(&nxt_idlers);
+        nr_idlers_cpu = cpumask_weight(&cpu_idlers);

xen/include/xen/sched.h +0 -0

 #define DOMAIN_DESTROYED (1<<31) /* assumes atomic_t is >= 32 bits */
 #define put_domain(_d) \
   if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)

xen/common/sched_credit.c static int · line 531

Quoted 13 lines

xen/common/sched_credit.c +4 -4

-        weight_cpu = cpumask_weight(&cpu_idlers);
-        weight_nxt = cpumask_weight(&nxt_idlers);
+        nr_idlers_cpu = cpumask_weight(&cpu_idlers);
+        nr_idlers_nxt = cpumask_weight(&nxt_idlers);
         /* smt_power_savings: consolidate work rather than spreading it */
         if ( sched_smt_power_savings ?
-             weight_cpu > weight_nxt :
-             weight_cpu * migrate_factor < weight_nxt )
+             nr_idlers_cpu > nr_idlers_nxt :
+             nr_idlers_cpu * migrate_factor < nr_idlers_nxt )
         {
             cpumask_and(&nxt_idlers, &cpus, &nxt_idlers);
             spc = CSCHED_PCPU(nxt);

Despite you mentioning this in the description, these last two hunks are, afaict, only renaming variables (and that's even debatable, as the current names aren't really misleading imo), and hence I don't think belong in a patch that clearly has the potential for causing (performance) regressions.

That said - I don't think it will (and even more, I'm agreeable to the change done).

Quoted 6 lines

xen/include/xen/sched.h +0 -0

@@ -396,6 +396,9 @@ extern struct vcpu *idle_vcpu[NR_CPUS];
 #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
 #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
 

xen/include/xen/sched.h extern struct vcpu *idle_vcpu[NR_CPUS]; · line 401

Quoted 3 lines

xen/include/xen/sched.h +3 -0

+#define current_on_cpu(_c) \
+  ( (per_cpu(schedule_data, _c).curr) )
+

This, imo, really belings into sched-if.h.

Plus - what's the point of double parentheses, when in fact none at all would be needed?

And finally, why "_c" and not just "c"?

Quoted 1 line

Quoted 1 line · Dario Faggioli

Quoted 6 lines · Jan Beulich

Despite you mentioning this in the description, these last two hunks are, afaict, only renaming variables (and that's even debatable, as the current names aren't really misleading imo), and hence I don't think belong in a patch that clearly has the potential for causing (performance) regressions.

Ok, I think I can live with the current names too... Just a matter of taste. :-)

Quoted 3 lines

That said - I don't think it will (and even more, I'm agreeable to the change done).

It has been benchmarked, together with the next change, and the results are in the changelog of 2/6. Numbers there show that the combination of those two changes are much more an improvement than anything else, at least for the workloads I considered (which includes sysbench and specjbb2005).

Anyway, I think I see your point, and I can either move the remane somewhere else or kill it entirely.

Quoted 2 lines

Anyway, I think I see your point, and I can either move the remane somewhere else or kill it entirely.

Yes please; I'll leave it to George to decide upon an eventual separate renaming patch.

Btw., when you resend, can you please also fix the subject, so grepping the changeset titles for "idle" would actually hit on this change?

Quoted 44 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/sched_credit.c +10 -4

@@ -59,6 +59,8 @@
  #define CSCHED_VCPU(_vcpu)  ((struct csched_vcpu *) (_vcpu)->sched_priv)
  #define CSCHED_DOM(_dom)    ((struct csched_dom *) (_dom)->sched_priv)
  #define RUNQ(_cpu)          (&(CSCHED_PCPU(_cpu)->runq))
+/* Is the first element of _cpu's runq its idle vcpu? */
+#define IS_RUNQ_IDLE(_cpu)  (is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu))
 
 
  /*

@@ -479,9 +481,14 @@ static int
       * distinct cores first and guarantees we don't do something stupid
       * like run two VCPUs on co-hyperthreads while there are idle cores
       * or sockets.
+     *
+     * Notice that, when computing the "idleness" of cpu, we may want to
+     * discount vc. That is, iff vc is the currently running and the only
+     * runnable vcpu on cpu, we add cpu to the idlers.
       */
      cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
-    cpumask_set_cpu(cpu, &idlers);
+    if ( current_on_cpu(cpu) == vc && IS_RUNQ_IDLE(cpu) )
+        cpumask_set_cpu(cpu, &idlers);
      cpumask_and(&cpus, &cpus, &idlers);
      cpumask_clear_cpu(cpu, &cpus);
 

@@ -489,7 +496,7 @@ static int
      {
          cpumask_t cpu_idlers;
          cpumask_t nxt_idlers;
-        int nxt, weight_cpu, weight_nxt;
+        int nxt, nr_idlers_cpu, nr_idlers_nxt;
          int migrate_factor;
 
          nxt = cpumask_cycle(cpu, &cpus);

@@ -513,12 +520,12 @@ static int
              cpumask_and(&nxt_idlers, &idlers, per_cpu(cpu_core_mask, nxt));
          }
 
-        weight_cpu = cpumask_weight(&cpu_idlers);
-        weight_nxt = cpumask_weight(&nxt_idlers);
+        nr_idlers_cpu = cpumask_weight(&cpu_idlers);

xen/common/sched_credit.c static int · line 531

Quoted 10 lines

xen/common/sched_credit.c +3 -2

+        nr_idlers_nxt = cpumask_weight(&nxt_idlers);
          /* smt_power_savings: consolidate work rather than spreading it */
          if ( sched_smt_power_savings ?
-             weight_cpu > weight_nxt :
-             weight_cpu * migrate_factor < weight_nxt )
+             nr_idlers_cpu > nr_idlers_nxt :
+             nr_idlers_cpu * migrate_factor < nr_idlers_nxt )
          {
              cpumask_and(&nxt_idlers, &cpus, &nxt_idlers);
              spc = CSCHED_PCPU(nxt);

Despite you mentioning this in the description, these last two hunks are, afaict, only renaming variables (and that's even debatable, as the current names aren't really misleading imo), and hence I don't think belong in a patch that clearly has the potential for causing (performance) regressions.

That said - I don't think it will (and even more, I'm agreeable to the change done).

Quoted 6 lines

xen/include/xen/sched.h +0 -0

@@ -396,6 +396,9 @@ extern struct vcpu *idle_vcpu[NR_CPUS];
  #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
  #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
 

xen/include/xen/sched.h extern struct vcpu *idle_vcpu[NR_CPUS]; · line 401

Quoted 3 lines

xen/include/xen/sched.h +3 -0

+#define current_on_cpu(_c) \
+  ( (per_cpu(schedule_data, _c).curr) )
+

This, imo, really belings into sched-if.h.

Plus - what's the point of double parentheses, when in fact none at all would be needed?

And finally, why "_c" and not just "c"?

Quoted 1 line

Quoted 1 line · George Dunlap

Quoted 69 lines · Jan Beulich

Quoted 44 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/sched_credit.c +10 -4

@@ -59,6 +59,8 @@
  #define CSCHED_VCPU(_vcpu)  ((struct csched_vcpu *) (_vcpu)->sched_priv)
  #define CSCHED_DOM(_dom)    ((struct csched_dom *) (_dom)->sched_priv)
  #define RUNQ(_cpu)          (&(CSCHED_PCPU(_cpu)->runq))
+/* Is the first element of _cpu's runq its idle vcpu? */
+#define IS_RUNQ_IDLE(_cpu)  (is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu))
 
 
  /*

@@ -479,9 +481,14 @@ static int
       * distinct cores first and guarantees we don't do something stupid
       * like run two VCPUs on co-hyperthreads while there are idle cores
       * or sockets.
+     *
+     * Notice that, when computing the "idleness" of cpu, we may want to
+     * discount vc. That is, iff vc is the currently running and the only
+     * runnable vcpu on cpu, we add cpu to the idlers.
       */
      cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
-    cpumask_set_cpu(cpu, &idlers);
+    if ( current_on_cpu(cpu) == vc && IS_RUNQ_IDLE(cpu) )
+        cpumask_set_cpu(cpu, &idlers);
      cpumask_and(&cpus, &cpus, &idlers);
      cpumask_clear_cpu(cpu, &cpus);
 

@@ -489,7 +496,7 @@ static int
      {
          cpumask_t cpu_idlers;
          cpumask_t nxt_idlers;
-        int nxt, weight_cpu, weight_nxt;
+        int nxt, nr_idlers_cpu, nr_idlers_nxt;
          int migrate_factor;
 
          nxt = cpumask_cycle(cpu, &cpus);

@@ -513,12 +520,12 @@ static int
              cpumask_and(&nxt_idlers, &idlers, per_cpu(cpu_core_mask, nxt));
          }
 
-        weight_cpu = cpumask_weight(&cpu_idlers);
-        weight_nxt = cpumask_weight(&nxt_idlers);
+        nr_idlers_cpu = cpumask_weight(&cpu_idlers);

xen/common/sched_credit.c static int · line 531

Quoted 10 lines

xen/common/sched_credit.c +3 -2

+        nr_idlers_nxt = cpumask_weight(&nxt_idlers);
          /* smt_power_savings: consolidate work rather than spreading it */
          if ( sched_smt_power_savings ?
-             weight_cpu > weight_nxt :
-             weight_cpu * migrate_factor < weight_nxt )
+             nr_idlers_cpu > nr_idlers_nxt :
+             nr_idlers_cpu * migrate_factor < nr_idlers_nxt )
          {
              cpumask_and(&nxt_idlers, &cpus, &nxt_idlers);
              spc = CSCHED_PCPU(nxt);

Despite you mentioning this in the description, these last two hunks are, afaict, only renaming variables (and that's even debatable, as the current names aren't really misleading imo), and hence I don't think belong in a patch that clearly has the potential for causing (performance) regressions.

That said - I don't think it will (and even more, I'm agreeable to the change done).

Quoted 6 lines

xen/include/xen/sched.h +0 -0

@@ -396,6 +396,9 @@ extern struct vcpu *idle_vcpu[NR_CPUS];
  #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
  #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
 

Quoted 4 lines

xen/include/xen/sched.h extern struct vcpu *idle_vcpu[NR_CPUS]; · line 401

Quoted 3 lines

xen/include/xen/sched.h +3 -0

+#define current_on_cpu(_c) \
+  ( (per_cpu(schedule_data, _c).curr) )
+

This, imo, really belings into sched-if.h.

Hmm, it looks like there are a number of things that could live in either sched-if.h or sched.h; but I think this one probably most closely links with thins like vcpu_is_runnable() and cpu_is_haltable(), both of which are in sched.h; so sched.h is where I'd put it.

Quoted 4 lines

Plus - what's the point of double parentheses, when in fact none at all would be needed?

And finally, why "_c" and not just "c"?

I think the underscore is pretty standard in macros.

Quoted 8 lines

Quoted 1 line

Quoted 1 line · George Dunlap

Quoted 1 line · Jan Beulich

This, imo, really belings into sched-if.h.

Hmm, it looks like there are a number of things that could live in either sched-if.h or sched.h; but I think this one probably most closely links with thins like vcpu_is_runnable() and cpu_is_haltable(), both of which are in sched.h; so sched.h is where I'd put it.

Any use of schedule_data, the type of which is declared in sched-if.h, should be in sched-if.h - someone only including sched.h can't make use of it anyway (and it's intended to be used by scheduler code, i.e. shouldn't be visible to other code).

In _csched_cpu_pick() we try to select the best possible CPU for running a VCPU, considering the characteristics of the underlying hardware (i.e., how many threads, core, sockets, and how busy they are). What we want is "the idle execution vehicle with the most idling neighbours in its grouping".

In order to achieve it, we select a CPU from the VCPU's affinity, giving preference to its current processor if possible, as the basis for the comparison with all the other CPUs. Problem is, to discount the VCPU itself when computing this "idleness" (in an attempt to be fair wrt its current processor), we arbitrarily and unconditionally consider that selected CPU as idle, even when it is not the case, for instance:
1. If the CPU is not the one where the VCPU is running (perhaps due

     to the affinity being changed);

2. The CPU is where the VCPU is running, but it has other VCPUs in its runq, so it won't go idle even if the VCPU in question goes.

xen/common/sched_credit.c +2 -0

@@ -59,6 +59,8 @@
  #define CSCHED_VCPU(_vcpu)  ((struct csched_vcpu *) (_vcpu)->sched_priv)
  #define CSCHED_DOM(_dom)    ((struct csched_dom *) (_dom)->sched_priv)
  #define RUNQ(_cpu)          (&(CSCHED_PCPU(_cpu)->runq))
+/* Is the first element of _cpu's runq its idle vcpu? */
+#define IS_RUNQ_IDLE(_cpu)  (is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu))
 
 
  /*

@@ -479,9 +481,14 @@ static int
       * distinct cores first and guarantees we don't do something stupid
       * like run two VCPUs on co-hyperthreads while there are idle cores

Right now, when a VCPU wakes-up, we check whether it should preempt what is running on the PCPU, and whether or not the waking VCPU can be migrated (by tickling some idlers). However, this can result in suboptimal or even wrong behaviour, as explained here:

http://lists.xen.org/archives/html/xen-devel/2012-10/msg01732.html

This change, instead, when deciding which PCPU(s) to tickle, upon VCPU wake-up, considers both what it is likely to happen on the PCPU where the wakeup occurs,and whether or not there are idlers where the woken-up VCPU can run. In fact, if there are, we can avoid interrupting the running VCPU. Only in case there aren't any of these PCPUs, preemption and migration are the way to go.

This has been tested (on top of the previous change) by running the following benchmarks inside 2, 6 and 10 VMs, concurrently, on a shared host, each with 2 VCPUs and 960 MB of memory (host had 16 ways and 12 GB RAM).

1) All VMs had 'cpus="all"' in their config file.

$ sysbench --test=cpu ... (time, lower is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 50.078467 +/- 1.6676162 | 49.673667 +/- 0.0094321 |
  | 6   | 63.259472 +/- 0.1137586 | 61.680011 +/- 1.0208723 |
  | 10  | 91.246797 +/- 0.1154008 | 90.396720 +/- 1.5900423 |
$ sysbench --test=memory ... (throughput, higher is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 485.56333 +/- 6.0527356 | 487.83167 +/- 0.7602850 |
  | 6   | 401.36278 +/- 1.9745916 | 409.96778 +/- 3.6761092 |
  | 10  | 294.43933 +/- 0.8064945 | 302.49033 +/- 0.2343978 |
$ specjbb2005 ... (throughput, higher is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 43150.63 +/- 1359.5616  | 43275.427 +/- 606.28185 |
  | 6   | 29274.29 +/- 1024.4042  | 29716.189 +/- 1290.1878 |
  | 10  | 19061.28 +/- 512.88561  | 19192.599 +/- 605.66058 |

2) All VMs had their VCPUs statically pinned to the host's PCPUs.

$ sysbench --test=cpu ... (time, lower is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 47.8211   +/- 0.0215504 | 47.826900 +/- 0.0077872 |
  | 6   | 62.689122 +/- 0.0877173 | 62.764539 +/- 0.3882493 |
  | 10  | 90.321097 +/- 1.4803867 | 89.974570 +/- 1.1437566 |
$ sysbench --test=memory ... (throughput, higher is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 550.97667 +/- 2.3512355 | 550.87000 +/- 0.8140792 |
  | 6   | 443.15000 +/- 5.7471797 | 454.01056 +/- 8.4373466 |
  | 10  | 313.89233 +/- 1.3237493 | 321.81167 +/- 0.3528418 |
$ specjbb2005 ... (throughput, higher is better)
  | 2   | 49591.057 +/- 952.93384 | 49594.195 +/- 799.57976 |
  | 6   | 33538.247 +/- 1089.2115 | 33671.758 +/- 1077.6806 |
  | 10  | 21927.870 +/- 831.88742 | 21891.131 +/- 563.37929 |

Numbers show how the change has either no or very limited impact (specjbb2005 case) or, when it does have some impact, that is a real improvement in performances (sysbench-memory case).

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

---
Changes from v1:
* Rewritten as per George's suggestion, in order to improve readability.
* Killed some of the stats, with the only exception of tickle_idlers_none' and tickle_idlers_some'. They don't make things look that terrible and I think they could be useful.
* The preemption+migration of the currently running VCPU has been turned into a migration request, instead than just tickling. I traced this thing some more, and it looks like that is the way to go. Tickling is not effective here, because the woken PCPU would expect cur to be out of the scheduler tail, which is likely false (cur->is_running is still set to 1).

xen/common/sched_credit.c +51 -36

@@ -133,6 +133,7 @@ struct csched_vcpu {
          uint32_t state_idle;
          uint32_t migrate_q;
          uint32_t migrate_r;
+        uint32_t kicked_away;
      } stats;
  #endif
  };

@@ -251,54 +252,67 @@ static inline void
      struct csched_vcpu * const cur =
          CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
      struct csched_private *prv = CSCHED_PRIV(per_cpu(scheduler, cpu));
-    cpumask_t mask;
+    cpumask_t mask, idle_mask;
+    int idlers_empty;
 
      ASSERT(cur);
      cpumask_clear(&mask);
 
-    /* If strictly higher priority than current VCPU, signal the CPU */
-    if ( new->pri > cur->pri )
+    idlers_empty = cpumask_empty(prv->idlers);
+
+    /*
+     * If the pcpu is idle, or there are no idlers and the new
+     * vcpu is a higher priority than the old vcpu, run it here.
+     *
+     * If there are idle cpus, first try to find one suitable to run
+     * new, so we can avoid preempting cur.  If we cannot find a
+     * suitable idler on which to run new, run it here, but try to
+     * find a suitable idler on which to run cur instead.
+     */
+    if ( cur->pri == CSCHED_PRI_IDLE
+         || (idlers_empty && new->pri > cur->pri) )
      {
-        if ( cur->pri == CSCHED_PRI_IDLE )
-            SCHED_STAT_CRANK(tickle_local_idler);
-        else if ( cur->pri == CSCHED_PRI_TS_OVER )
-            SCHED_STAT_CRANK(tickle_local_over);
-        else if ( cur->pri == CSCHED_PRI_TS_UNDER )
-            SCHED_STAT_CRANK(tickle_local_under);
-        else
-            SCHED_STAT_CRANK(tickle_local_other);
-
+        if ( cur->pri != CSCHED_PRI_IDLE )
+            SCHED_STAT_CRANK(tickle_idlers_none);
          cpumask_set_cpu(cpu, &mask);
      }
+    else if ( !idlers_empty )
+    {
+        /* Check whether or not there are idlers that can run new */
+        cpumask_and(&idle_mask, prv->idlers, new->vcpu->cpu_affinity);
 
-    /*
-     * If this CPU has at least two runnable VCPUs, we tickle any idlers to
-     * let them know there is runnable work in the system...
-     */
-    if ( cur->pri > CSCHED_PRI_IDLE )
-    {
-        if ( cpumask_empty(prv->idlers) )
+        /*
+         * If there are no suitable idlers for new, and it's higher
+         * priority than cur, ask the scheduler to migrate cur away.
+         * We have to act like this (instead of just waking some of
+         * the idlers suitable for cur) because cur is running.
+         *
+         * If there are suitable idlers for new, no matter priorities,
+         * leave cur alone (as it is running and is, likely, cache-hot)
+         * and wake some of them (which is waking up and so is, likely,
+         * cache cold anyway).
+         */
+        if ( cpumask_empty(&idle_mask) && new->pri > cur->pri )
          {
              SCHED_STAT_CRANK(tickle_idlers_none);
+            SCHED_VCPU_STAT_CRANK(cur, kicked_away);
+            SCHED_VCPU_STAT_CRANK(cur, migrate_r);
+            SCHED_STAT_CRANK(migrate_kicked_away);
+            set_bit(_VPF_migrating, &cur->vcpu->pause_flags);
+            cpumask_set_cpu(cpu, &mask);
          }
-        else
+        else if ( !cpumask_empty(&idle_mask) )
          {
-            cpumask_t idle_mask;
-
-            cpumask_and(&idle_mask, prv->idlers, new->vcpu->cpu_affinity);
-            if ( !cpumask_empty(&idle_mask) )
+            /* Which of the idlers suitable for new shall we wake up? */
+            SCHED_STAT_CRANK(tickle_idlers_some);
+            if ( opt_tickle_one_idle )
              {
-                SCHED_STAT_CRANK(tickle_idlers_some);
-                if ( opt_tickle_one_idle )
-                {
-                    this_cpu(last_tickle_cpu) =
-                        cpumask_cycle(this_cpu(last_tickle_cpu), &idle_mask);
-                    cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
-                }
-                else
-                    cpumask_or(&mask, &mask, &idle_mask);
+                this_cpu(last_tickle_cpu) =
+                    cpumask_cycle(this_cpu(last_tickle_cpu), &idle_mask);
+                cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
              }
-            cpumask_and(&mask, &mask, new->vcpu->cpu_affinity);
+            else
+                cpumask_or(&mask, &mask, &idle_mask);
          }
      }
 

@@ -1456,13 +1470,14 @@ csched_dump_vcpu(struct csched_vcpu *svc
      {
          printk(" credit=%i [w=%u]", atomic_read(&svc->credit), sdom->weight);
  #ifdef CSCHED_STATS
-        printk(" (%d+%u) {a/i=%u/%u m=%u+%u}",
+        printk(" (%d+%u) {a/i=%u/%u m=%u+%u (k=%u)}",
                  svc->stats.credit_last,
                  svc->stats.credit_incr,
                  svc->stats.state_active,
                  svc->stats.state_idle,
                  svc->stats.migrate_q,
-                svc->stats.migrate_r);
+                svc->stats.migrate_r,
+                svc->stats.kicked_away);
  #endif
      }
 

xen/include/xen/perfc_defn.h +1 -4

@@ -39,10 +39,6 @@ PERFCOUNTER(vcpu_wake_runnable,     "csc
  PERFCOUNTER(vcpu_wake_not_runnable, "csched: vcpu_wake_not_runnable")
  PERFCOUNTER(vcpu_park,              "csched: vcpu_park")
  PERFCOUNTER(vcpu_unpark,            "csched: vcpu_unpark")
-PERFCOUNTER(tickle_local_idler,     "csched: tickle_local_idler")
-PERFCOUNTER(tickle_local_over,      "csched: tickle_local_over")
-PERFCOUNTER(tickle_local_under,     "csched: tickle_local_under")
-PERFCOUNTER(tickle_local_other,     "csched: tickle_local_other")
  PERFCOUNTER(tickle_idlers_none,     "csched: tickle_idlers_none")
  PERFCOUNTER(tickle_idlers_some,     "csched: tickle_idlers_some")
  PERFCOUNTER(load_balance_idle,      "csched: load_balance_idle")

@@ -52,6 +48,7 @@ PERFCOUNTER(steal_trylock_failed,   "csc
  PERFCOUNTER(steal_peer_idle,        "csched: steal_peer_idle")
  PERFCOUNTER(migrate_queued,         "csched: migrate_queued")
  PERFCOUNTER(migrate_running,        "csched: migrate_running")
+PERFCOUNTER(migrate_kicked_away,    "csched: migrate_kicked_away")
  PERFCOUNTER(vcpu_hot,               "csched: vcpu_hot")
 
  PERFCOUNTER(need_flush_tlb_flush,   "PG_need_flush tlb flushes")

Defined by the previous change, using it wherever it is appropriate throughout sched_credit.c makes the code easier to read.

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

xen/common/sched_credit.c +7 -8

@@ -231,7 +231,7 @@ static void burn_credits(struct csched_v
      unsigned int credits;
 
      /* Assert svc is current */
-    ASSERT(svc==CSCHED_VCPU(per_cpu(schedule_data, svc->vcpu->processor).curr));
+    ASSERT( svc == CSCHED_VCPU(current_on_cpu(svc->vcpu->processor)) );
 
      if ( (delta = now - svc->start_time) <= 0 )
          return;

@@ -249,8 +249,7 @@ DEFINE_PER_CPU(unsigned int, last_tickle
  static inline void
  __runq_tickle(unsigned int cpu, struct csched_vcpu *new)
  {
-    struct csched_vcpu * const cur =
-        CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
+    struct csched_vcpu * const cur = CSCHED_VCPU(current_on_cpu(cpu));
      struct csched_private *prv = CSCHED_PRIV(per_cpu(scheduler, cpu));
      cpumask_t mask, idle_mask;
      int idlers_empty;

@@ -387,7 +386,7 @@ csched_alloc_pdata(const struct schedule
          per_cpu(schedule_data, cpu).sched_priv = spc;
 
      /* Start off idling... */
-    BUG_ON(!is_idle_vcpu(per_cpu(schedule_data, cpu).curr));
+    BUG_ON(!is_idle_vcpu(current_on_cpu(cpu)));
      cpumask_set_cpu(cpu, prv->idlers);
 
      spin_unlock_irqrestore(&prv->lock, flags);

@@ -730,7 +729,7 @@ csched_vcpu_sleep(const struct scheduler
 
      BUG_ON( is_idle_vcpu(vc) );
 
-    if ( per_cpu(schedule_data, vc->processor).curr == vc )
+    if ( current_on_cpu(vc->processor) == vc )
          cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
      else if ( __vcpu_on_runq(svc) )
          __runq_remove(svc);

@@ -744,7 +743,7 @@ csched_vcpu_wake(const struct scheduler
 
      BUG_ON( is_idle_vcpu(vc) );
 
-    if ( unlikely(per_cpu(schedule_data, cpu).curr == vc) )
+    if ( unlikely(current_on_cpu(cpu) == vc) )
      {
          SCHED_STAT_CRANK(vcpu_wake_running);
          return;

@@ -1213,7 +1212,7 @@ static struct csched_vcpu *
  csched_runq_steal(int peer_cpu, int cpu, int pri)
  {
      const struct csched_pcpu * const peer_pcpu = CSCHED_PCPU(peer_cpu);
-    const struct vcpu * const peer_vcpu = per_cpu(schedule_data, peer_cpu).curr;
+    const struct vcpu * const peer_vcpu = current_on_cpu(peer_cpu);
      struct csched_vcpu *speer;
      struct list_head *iter;
      struct vcpu *vc;

@@ -1502,7 +1501,7 @@ csched_dump_pcpu(const struct scheduler
      printk("core=%s\n", cpustr);
 
      /* current VCPU */
-    svc = CSCHED_VCPU(per_cpu(schedule_data, cpu).curr);
+    svc = CSCHED_VCPU(current_on_cpu(cpu));
      if ( svc )
      {
          printk("\trun: ");

Hmm, I hadn't read this patch when I commented about removing the macro from the first patch. :-)

I personally think that using vc->processor is better in that patch anyway; but using this macro elsewhere is probably fine.

I think from a taste point of view, I would have put this patch, with the new definition, as the first patch in the series, and the had the second patch just use it.

When looking at traces, it turns out to be useful to know where a waking-up VCPU is being queued. Yes, that is always the CPU where it ran last, but that information can well be lost in past trace records!

Quoted 4 lines · Dario Faggioli

When looking at traces, it turns out to be useful to know where a waking-up VCPU is being queued. Yes, that is always the CPU where it ran last, but that information can well be lost in past trace records!

When you say "lost in past trace records", do you primarily mean that the records themselves have been lost (due to the per-cpu trace buffers filling up), or do you mean that it may be way way back and you don't want to go back and find it?

If the latter, I think the best thing to do would be to just augment xenalyze to keep track of that information and print it when it sees the wake record.

So that it becomes possible to create scheduler specific trace records, within each scheduler, without worrying about the overlapping, and also without giving up being able to recognise them univocally. The latter is deemed as useful, since we can have more than one scheduler running at the same time, thanks to cpupools.

The event ID is 12 bits, and this change uses the upper 3 of them for the 'scheduler ID'. This means we're limited to 8 schedulers and to 512 scheduler specific tracing events. Both seem reasonable limitations as of now.

This also converts the existing credit2 tracing (the only scheduler generating tracing events up to now) to the new system.

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

---
Changes from v1:
* The event ID generaion macro is now called `TRC_SCHED_CLASS_EVT()', and has been generalized and put in trace.h, as suggested.
* The handling of per-scheduler tracing IDs and masks have been restructured, properly naming "ID" the numerical identifiers and "MASK" the bitmasks, as requested.

xen/common/sched_credit2.c +16 -12

@@ -29,18 +29,22 @@
  #define d2printk(x...)
  //#define d2printk printk
 
-#define TRC_CSCHED2_TICK        TRC_SCHED_CLASS + 1
-#define TRC_CSCHED2_RUNQ_POS    TRC_SCHED_CLASS + 2
-#define TRC_CSCHED2_CREDIT_BURN TRC_SCHED_CLASS + 3
-#define TRC_CSCHED2_CREDIT_ADD  TRC_SCHED_CLASS + 4
-#define TRC_CSCHED2_TICKLE_CHECK TRC_SCHED_CLASS + 5
-#define TRC_CSCHED2_TICKLE       TRC_SCHED_CLASS + 6
-#define TRC_CSCHED2_CREDIT_RESET TRC_SCHED_CLASS + 7
-#define TRC_CSCHED2_SCHED_TASKLET TRC_SCHED_CLASS + 8
-#define TRC_CSCHED2_UPDATE_LOAD   TRC_SCHED_CLASS + 9
-#define TRC_CSCHED2_RUNQ_ASSIGN   TRC_SCHED_CLASS + 10
-#define TRC_CSCHED2_UPDATE_VCPU_LOAD   TRC_SCHED_CLASS + 11
-#define TRC_CSCHED2_UPDATE_RUNQ_LOAD   TRC_SCHED_CLASS + 12
+/*
+ * Credit2 tracing events ("only" 512 available!). Check
+ * include/public/trace.h for more details.
+ */
+#define TRC_CSCHED2_TICK             TRC_SCHED_CLASS_EVT(CSCHED2, 1)
+#define TRC_CSCHED2_RUNQ_POS         TRC_SCHED_CLASS_EVT(CSCHED2, 2)
+#define TRC_CSCHED2_CREDIT_BURN      TRC_SCHED_CLASS_EVT(CSCHED2, 3)
+#define TRC_CSCHED2_CREDIT_ADD       TRC_SCHED_CLASS_EVT(CSCHED2, 4)
+#define TRC_CSCHED2_TICKLE_CHECK     TRC_SCHED_CLASS_EVT(CSCHED2, 5)
+#define TRC_CSCHED2_TICKLE           TRC_SCHED_CLASS_EVT(CSCHED2, 6)
+#define TRC_CSCHED2_CREDIT_RESET     TRC_SCHED_CLASS_EVT(CSCHED2, 7)
+#define TRC_CSCHED2_SCHED_TASKLET    TRC_SCHED_CLASS_EVT(CSCHED2, 8)
+#define TRC_CSCHED2_UPDATE_LOAD      TRC_SCHED_CLASS_EVT(CSCHED2, 9)
+#define TRC_CSCHED2_RUNQ_ASSIGN      TRC_SCHED_CLASS_EVT(CSCHED2, 10)
+#define TRC_CSCHED2_UPDATE_VCPU_LOAD TRC_SCHED_CLASS_EVT(CSCHED2, 11)
+#define TRC_CSCHED2_UPDATE_RUNQ_LOAD TRC_SCHED_CLASS_EVT(CSCHED2, 12)
 
  /*
   * WARNING: This is still in an experimental phase.  Status and work can be found at the

xen/include/public/trace.h +26 -0

@@ -57,6 +57,32 @@
  #define TRC_SCHED_CLASS     0x00022000   /* Scheduler-specific    */
  #define TRC_SCHED_VERBOSE   0x00028000   /* More inclusive scheduling */
 
+/*
+ * The highest 3 bits of the last 12 bits of TRC_SCHED_CLASS above are
+ * reserved for encoding what scheduler produced the information. The
+ * actual event is encoded in the last 9 bits.
+ *
+ * This means we have 8 scheduling IDs available (which means at most 8
+ * schedulers generating events) and, in each scheduler, up to 512
+ * different events.
+ */
+#define TRC_SCHED_ID_BITS 3
+#define TRC_SCHED_ID_SHIFT (TRC_SUBCLS_SHIFT - TRC_SCHED_ID_BITS)
+#define TRC_SCHED_ID_MASK (((1UL<<TRC_SCHED_ID_BITS) - 1) << TRC_SCHED_ID_SHIFT)
+#define TRC_SCHED_EVT_MASK (~(TRC_SCHED_ID_MASK))
+
+/* Per-scheduler IDs, to identify scheduler specific events */
+#define TRC_SCHED_CSCHED   0
+#define TRC_SCHED_CSCHED2  1
+#define TRC_SCHED_SEDF     2
+#define TRC_SCHED_ARINC653 3
+
+/* Per-scheduler tracing */
+#define TRC_SCHED_CLASS_EVT(_c, _e) \
+  ( ( TRC_SCHED_CLASS | \
+      ((TRC_SCHED_##_c << TRC_SCHED_ID_SHIFT) & TRC_SCHED_ID_MASK) ) + \
+    (_e & TRC_SCHED_EVT_MASK) )
+
  /* Trace classes for Hardware */
  #define TRC_HW_PM           0x00801000   /* Power management traces */
  #define TRC_HW_IRQ          0x00802000   /* Traces relating to the handling of IRQs */

About tickling, and PCPU selection.

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

---
Changes from v1:
* Dummy struct d {}', accommodating cpu' only, removed
in spite of the much more readable `trace_var(..., sizeof(cpu), &cpu)', as suggested.

xen/common/sched_credit.c +13 -0

@@ -21,6 +21,7 @@
  #include <asm/atomic.h>
  #include <xen/errno.h>
  #include <xen/keyhandler.h>
+#include <xen/trace.h>
 
 
  /*

@@ -97,6 +98,18 @@
 
 
  /*
+ * Credit tracing events ("only" 512 available!). Check
+ * include/public/trace.h for more details.
+ */
+#define TRC_CSCHED_SCHED_TASKLET TRC_SCHED_CLASS_EVT(CSCHED, 1)
+#define TRC_CSCHED_ACCOUNT_START TRC_SCHED_CLASS_EVT(CSCHED, 2)
+#define TRC_CSCHED_ACCOUNT_STOP  TRC_SCHED_CLASS_EVT(CSCHED, 3)
+#define TRC_CSCHED_STOLEN_VCPU   TRC_SCHED_CLASS_EVT(CSCHED, 4)
+#define TRC_CSCHED_PICKED_CPU    TRC_SCHED_CLASS_EVT(CSCHED, 5)
+#define TRC_CSCHED_TICKLE        TRC_SCHED_CLASS_EVT(CSCHED, 6)
+
+
+/*
   * Boot parameters
   */
  static int __read_mostly sched_credit_tslice_ms = CSCHED_DEFAULT_TSLICE_MS;

@@ -315,9 +328,18 @@ static inline void
          }
      }

xen/common/sched_credit.c +14 -0

+
+        /* Send scheduler interrupts to designated CPUs */
          cpumask_raise_softirq(&mask, SCHEDULE_SOFTIRQ);
+    }
  }
 
  static void

@@ -554,6 +576,8 @@ static int
      if ( commit && spc )
         spc->idle_bias = cpu;
 
+    TRACE_3D(TRC_CSCHED_PICKED_CPU, vc->domain->domain_id, vc->vcpu_id, cpu);
+
      return cpu;
  }
 

@@ -586,6 +610,9 @@ static inline void
          }
      }
 
+    TRACE_3D(TRC_CSCHED_ACCOUNT_START, sdom->dom->domain_id,
+             svc->vcpu->vcpu_id, sdom->active_vcpu_count);
+
      spin_unlock_irqrestore(&prv->lock, flags);
  }
 

@@ -608,6 +635,9 @@ static inline void
      {
          list_del_init(&sdom->active_sdom_elem);
      }
+
+    TRACE_3D(TRC_CSCHED_ACCOUNT_STOP, sdom->dom->domain_id,
+             svc->vcpu->vcpu_id, sdom->active_vcpu_count);
  }
 
  static void

@@ -1241,6 +1271,8 @@ csched_runq_steal(int peer_cpu, int cpu,
              if (__csched_vcpu_is_migrateable(vc, cpu))
              {
                  /* We got a candidate. Grab it! */
+                TRACE_3D(TRC_CSCHED_STOLEN_VCPU, peer_cpu,
+                         vc->domain->domain_id, vc->vcpu_id);
                  SCHED_VCPU_STAT_CRANK(speer, migrate_q);
                  SCHED_STAT_CRANK(migrate_queued);
                  WARN_ON(vc->is_urgent);

@@ -1401,6 +1433,7 @@ csched_schedule(
      /* Tasklet work (which runs in idle VCPU context) overrides all else. */
      if ( tasklet_work_scheduled )
      {
+        TRACE_0D(TRC_CSCHED_SCHED_TASKLET);
          snext = CSCHED_VCPU(idle_vcpu[cpu]);
          snext->pri = CSCHED_PRI_TS_BOOST;
      }

xen/common/sched_credit.c static inline void · line 338

Quoted 9 lines

xen/common/sched_credit.c +7 -1

-    /* Send scheduler interrupts to designated CPUs */
      if ( !cpumask_empty(&mask) )
+    {
+        if ( unlikely(tb_init_done) )
+        {
+            /* Avoid TRACE_*: saves checking !tb_init_done each step */
+            for_each_cpu(cpu, &mask)
+                trace_var(TRC_CSCHED_TICKLE, 0, sizeof(cpu), &cpu);
+        }

Hmm, probably should have pointed this out before, but trace_var() is a static inline which checks tb_init_done -- you want __trace_var(). :-)