xen: sched_credit: fix tickling and add some tracing

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

Quoted 3 lines · Dario Faggioli

xen/common/sched_credit.c +3 -0

+            /*
+             * If there aren't idlers for new, then letting new preempt cur and
+             * try to migrate cur becomes ineviable. If that is the case, update

"inevitable"

(I have nothing more intelligent to say...)

So that it becomes possible to create specific scheduling event 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]>

xen/common/sched_credit2.c +0 -9

@@ -29,18 +29,24 @@
 #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

xen/common/sched_credit2.c +13 -0

+
+#define TRC_CSCHED2_TICK             (TRC_CSCHED2_EVENT(1))
+#define TRC_CSCHED2_RUNQ_POS         (TRC_CSCHED2_EVENT(2))
+#define TRC_CSCHED2_CREDIT_BURN      (TRC_CSCHED2_EVENT(3))
+#define TRC_CSCHED2_CREDIT_ADD       (TRC_CSCHED2_EVENT(4))
+#define TRC_CSCHED2_TICKLE_CHECK     (TRC_CSCHED2_EVENT(5))
+#define TRC_CSCHED2_TICKLE           (TRC_CSCHED2_EVENT(6))
+#define TRC_CSCHED2_CREDIT_RESET     (TRC_CSCHED2_EVENT(7))
+#define TRC_CSCHED2_SCHED_TASKLET    (TRC_CSCHED2_EVENT(8))
+#define TRC_CSCHED2_UPDATE_LOAD      (TRC_CSCHED2_EVENT(9))
+#define TRC_CSCHED2_RUNQ_ASSIGN      (TRC_CSCHED2_EVENT(10))
+#define TRC_CSCHED2_UPDATE_VCPU_LOAD (TRC_CSCHED2_EVENT(11))
+#define TRC_CSCHED2_UPDATE_RUNQ_LOAD (TRC_CSCHED2_EVENT(12))
 
 /*
  * WARNING: This is still in an experimental phase.  Status and work can

be found at the

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

@@ -57,6 +57,26 @@
 #define TRC_SCHED_CLASS     0x00022000   /* Scheduler-specific    */
 #define TRC_SCHED_VERBOSE   0x00028000   /* More inclusive scheduling */
 
+/*
+ * Per-scheduler masks, to identify scheduler specific events.
+ *
+ * The highest 3 bits of the last 12 bits of the above TRC_SCHED_*
+ * tracing classes are reserved for encoding what scheduler is producing
+ * the event. The last 9 bits is where the scheduler specific event is
+ * encoded.
+ *
+ * This means we can have at most 8 tracing scheduling masks (which
+ * means at most 8 schedulers generating tracing events) and, in each

#define TRC_SCHED_CSCHED 0
#define TRC_SCHED_CSCHED2
/*...*/

Quoted 11 lines · George Dunlap

Quoted 10 lines · Dario Faggioli

xen/common/sched_credit.c +10 -0

+            /* Avoid TRACE_* to avoid a lot of useless !tb_init_done checks */
+            for_each_cpu(cpu, &mask)
+            {
+                struct {
+                    unsigned cpu:8;
+                } d;
+                d.cpu = cpu;
+                trace_var(TRC_CSCHED_TICKLE, 0,
+                          sizeof(d),
+                          (unsigned char*)&d);

Quoted 2 lines

Why not just TRC_1D()?

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D). I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

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

Quoted 10 lines · Dario Faggioli

xen/common/sched_credit.c +10 -0

+            /* Avoid TRACE_* to avoid a lot of useless !tb_init_done checks */
+            for_each_cpu(cpu, &mask)
+            {
+                struct {
+                    unsigned cpu:8;
+                } d;
+                d.cpu = cpu;
+                trace_var(TRC_CSCHED_TICKLE, 0,
+                          sizeof(d),
+                          (unsigned char*)&d);

Why not just TRC_1D()?

The tracing infrastructure can only set the size
at a granularity of 32-bit words anyway, and at this point cpu is "unsigned int", which will be a single word.

Other than that, everything looks good.

+/*
+ * Credit2 tracing events ("only" 512 available!). Check
+ * include/public/trace.h for more details.
+ */
+#define TRC_CSCHED2_EVENT(_e) ((TRC_SCHED_CLASS|
TRC_MASK_CSCHED2) + _e)

I think I would make this generic, and put it in trace.h

-- maybe something like this? (Haven't run this through a compiler)

#define TRC_SCHED_CLASS_EVT(_c, _e) \

((TRC_SCHED_CLASS|(TRC_SCHED_##_c<<TRC_SCHED_MASK_SHIFT))+(_e&TRC_SCHED_CLASS_MASK))

+#define TRC_SCHED_ID_BITS 3
+#define TRC_SCHED_MASK_SHIFT (TRC_SUBCLS_SHIFT - TRC_SCHED_ID_BITS)
+
+#define TRC_MASK_CSCHED (0 << TRC_SCHED_MASK_SHIFT)
+#define TRC_MASK_CSCHED2 (1 << TRC_SCHED_MASK_SHIFT)
+#define TRC_MASK_SEDF (2 << TRC_SCHED_MASK_SHIFT)
+#define TRC_MASK_ARINC653 (3 << TRC_SCHED_MASK_SHIFT)

I don't think "mask" is right here -- these aren't masks, they're numerical values. :-) If we use something like the #define above, then we can do:

#define TRC_SCHED_CSCHED 0
#define TRC_SCHED_CSCHED2
/*...*/

Quoted 12 lines · George Dunlap

Quoted 10 lines · Dario Faggioli

xen/common/sched_credit.c +10 -0

+            /* Avoid TRACE_* to avoid a lot of useless !tb_init_done checks */
+            for_each_cpu(cpu, &mask)
+            {
+                struct {
+                    unsigned cpu:8;
+                } d;
+                d.cpu = cpu;
+                trace_var(TRC_CSCHED_TICKLE, 0,
+                          sizeof(d),
+                          (unsigned char*)&d);

Quoted 2 lines

Why not just TRC_1D()?

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D).

I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

Quoted 4 lines

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D).

If tb_init_done isn't marked volatile or anything like that isn't the check hoisted out of the loop by the compiler?

Quoted 3 lines

I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

Or maybe consider __TRACE_1D and friends which omit the check?

Right now, when a VCPU wakes-up, we check if the 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 what PCPUs to tickle upon VCPU wake-up, considers both what it is likely to happen on the PCPU where the wakeup occurs, as well as whether or not there are idle PCPUs where to run the waking VCPU.
In fact, if there are idlers where the new VCPU can run, we can avoid interrupting the running VCPU. OTOH, in case there aren't any of these PCPUs, preemption and migration are the way to go.

This has been tested 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 has 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.704933 +/- 0.0277184 |
  | 6   | 63.259472 +/- 0.1137586 | 62.227367 +/- 0.3880619 |
  | 10  | 91.246797 +/- 0.1154008 | 91.174820 +/- 0.0928781 |
$ sysbench --test=memory ... (throughput, higher is better)
  | VMs | w/o this change         | w/ this change          |
  | 2   | 485.56333 +/- 6.0527356 | 525.57833 +/- 25.085826 |
  | 6   | 401.36278 +/- 1.9745916 | 421.96111 +/- 9.0364048 |
  | 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  | 42720.632 +/- 1937.4488 |
  | 6   | 29274.29 +/- 1024.4042  | 29518.171 +/- 1014.5239 |
  | 10  | 19061.28 +/- 512.88561  | 19050.141 +/- 458.77327 |

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 | 49610.98  +/- 1242.1675 |
  | 6   | 33538.247 +/- 1089.2115 | 33682.222 +/- 1216.1078 |
  | 10  | 21927.870 +/- 831.88742 | 21801.138 +/- 561.97068 |

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

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

Quoted 8 lines · Ian Campbell

Quoted 4 lines

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D).

If tb_init_done isn't marked volatile or anything like that isn't the check hoisted out of the loop by the compiler?

Good point. As they're all macros, yes, I think that is something very likely to happen... Although, I haven't checked the generated code, I'll take a look. Thanks.

Quoted 4 lines

Quoted 3 lines

I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

Quoted 2 lines

Or maybe consider __TRACE_1D and friends which omit the check?

Mmm... It may well be me, but my

$ grep __TRACE xen/* -R

does not show any results... What am I missing?

Quoted 15 lines · Dario Faggioli

Quoted 7 lines · Ian Campbell

Quoted 4 lines

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D).

If tb_init_done isn't marked volatile or anything like that isn't the check hoisted out of the loop by the compiler?

Good point. As they're all macros, yes, I think that is something very likely to happen... Although, I haven't checked the generated code, I'll take a look. Thanks.

Quoted 3 lines

Quoted 3 lines

I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

Quoted 7 lines

Quoted 2 lines

Or maybe consider __TRACE_1D and friends which omit the check?

Mmm... It may well be me, but my

$ grep __TRACE xen/* -R

does not show any results... What am I missing?

I meant to define + use those macros.

Quoted 7 lines · Ian Campbell

Quoted 4 lines

As I tried to explain in the comment, I just wanted to avoid checking for !tb_init_done more than once, as this happens within a loop and, at least potentially, there may be more CPUs to tickle (and thus more calls to TRACE_1D).

If tb_init_done isn't marked volatile or anything like that isn't the check hoisted out of the loop by the compiler?

Good point. As they're all macros, yes, I think that is something very likely to happen... Although, I haven't checked the generated code, I'll take a look. Thanks.

Quoted 6 lines

Quoted 4 lines

I take this comment of yours as you not thinking that is something worthwhile, right? If so, I can definitely turn this into a "standard" TRACE_1D() call.

Or maybe consider __TRACE_1D and friends which omit the check?

Mmm... It may well be me, but my

$ grep __TRACE xen/* -R

does not show any results... What am I missing?

Dario