xen: sched: Credit1 shouldn't boost vcpus being migrated.

Add tracepoints and a performance counter for boosting and unboosting in Credit1.

Note that they (the trace points) do not cover the case of the idle vCPU being boosted to run a tasklet, as there already is TRC_CSCHED_SCHED_TASKLET for that.

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

---
Cc: George Dunlap <[email protected]>

---
 xen/common/sched_credit.c    |    8 ++++++++
 xen/include/xen/perfc_defn.h |    1 +
 2 files changed, 9 insertions(+)

xen/common/sched_credit.c +8 -0

index 671bbee..5708701 100644
@@ -126,6 +126,8 @@
 #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)
+#define TRC_CSCHED_BOOST_START   TRC_SCHED_CLASS_EVT(CSCHED, 7)
+#define TRC_CSCHED_BOOST_END     TRC_SCHED_CLASS_EVT(CSCHED, 8)
 
 
 /*

@@ -856,7 +858,11 @@ csched_vcpu_acct(struct csched_private *prv, unsigned int cpu)
      * amount of CPU resources and should no longer be boosted.
      */
     if ( svc->pri == CSCHED_PRI_TS_BOOST )
+    {
         svc->pri = CSCHED_PRI_TS_UNDER;
+        TRACE_2D(TRC_CSCHED_BOOST_END, svc->sdom->dom->domain_id,
+                 svc->vcpu->vcpu_id);
+    }
 
     /*
      * Update credits

@@ -1023,6 +1029,8 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
     if ( svc->pri == CSCHED_PRI_TS_UNDER &&
          !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
     {
+        TRACE_2D(TRC_CSCHED_BOOST_START, vc->domain->domain_id, vc->vcpu_id);
+        SCHED_STAT_CRANK(vcpu_boost);
         svc->pri = CSCHED_PRI_TS_BOOST;
     }
 

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

index 76ee803..21c1e0b 100644
@@ -40,6 +40,7 @@ PERFCOUNTER(acct_reorder,           "csched: acct_reorder")
 PERFCOUNTER(acct_min_credit,        "csched: acct_min_credit")
 PERFCOUNTER(acct_vcpu_active,       "csched: acct_vcpu_active")
 PERFCOUNTER(acct_vcpu_idle,         "csched: acct_vcpu_idle")
+PERFCOUNTER(vcpu_boost,             "csched: vcpu_boost")
 PERFCOUNTER(vcpu_park,              "csched: vcpu_park")
 PERFCOUNTER(vcpu_unpark,            "csched: vcpu_unpark")
 PERFCOUNTER(load_balance_idle,      "csched: load_balance_idle")

Moving a vCPU to a different pCPU means offlining it and then waking it up, on the new pCPU. Credit1 grants BOOST priority to vCPUs that wakes up, with the aim of improving I/O latency. The net effect of this all is that vCPUs get boosted when migrating, which shouldn't happen.

For instance, this causes scheduling anomalies and, potentially, performance problems, as reported here:
http://lists.xen.org/archives/html/xen-devel/2015-10/msg02851.html

This patch fixes this by noting down (by means of a flag) the fact that the vCPU is about to undergo a migration. This way we can tell, later, during a wakeup, whether the vCPU is migrating or unblocking, and decide whether or not to apply the boosting.

Note that it is important that atomic-safe bit operations are used when manipulating vCPUs' flags. Take the chance and add a comment about this.

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

---
Cc: George Dunlap <[email protected]>
Cc: Jan Beulich <[email protected]>
---
Changes from v2:
* test_and_clear() is necessary when accessing svc->flags;
* added a comment about such need at the top, where the flags are defined.

Changes from v1:
* rewritten, following suggestion got during review: there are no wakeup flags any longer, and all is done in sched_credit.c by setting a flag in csched_cpu_pick() and testing (and cleating) it in csched_vcpu_wake().

---
 xen/common/sched_credit.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

xen/common/sched_credit.c +19 -3

index 5708701..597a784 100644
@@ -63,9 +63,14 @@
 
 /*
  * Flags
+ *
+ * Note that svc->flags (where these flags live) is protected by an
+ * inconsistent set of locks. Therefore atomic-safe bit operations must
+ * be used for accessing it.
  */
 #define CSCHED_FLAG_VCPU_PARKED    0x0  /* VCPU over capped credits */
 #define CSCHED_FLAG_VCPU_YIELD     0x1  /* VCPU yielding */
+#define CSCHED_FLAG_VCPU_MIGRATING 0x2  /* VCPU may have moved to a new pcpu */
 
 
 /*

@@ -787,6 +792,16 @@ _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc, bool_t commit)
 static int
 csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
 {
+    struct csched_vcpu *svc = CSCHED_VCPU(vc);
+
+    /*
+     * We have been called by vcpu_migrate() (in schedule.c), as part
+     * of the process of seeing if vc can be migrated to another pcpu.
+     * We make a note about this in svc->flags so that later, in
+     * csched_vcpu_wake() (still called from vcpu_migrate()) we won't
+     * get boosted, which we don't deserve as we are "only" migrating.
+     */
+    set_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags);
     return _csched_cpu_pick(ops, vc, 1);
 }
 

@@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
      * more CPU resource intensive VCPUs without impacting overall 
      * system fairness.
      *
-     * The one exception is for VCPUs of capped domains unpausing
-     * after earning credits they had overspent. We don't boost
-     * those.
+     * There are two cases, when we don't want to boost:
+     *  - VCPUs that are waking up after a migration, rather than
+     *    after having block;
+     *  - VCPUs of capped domains unpausing after earning credits

Quoted 22 lines · Dario Faggioli

(quoted diff) +0 -0

@@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler
 *ops, struct vcpu *vc)
       * more CPU resource intensive VCPUs without impacting

overall
      * system fairness.
      *
-     * The one exception is for VCPUs of capped domains unpausing -     * after earning credits they had overspent. We don't boost -     * those.
+     * There are two cases, when we don't want to boost:
+     *  - VCPUs that are waking up after a migration, rather than +     *    after having block;
+     *  - VCPUs of capped domains unpausing after earning credits +     *    they had overspent.
+     *
+     * Note that checking whether we are "only" migrating must be +     * done up front, as we do not want the clearing of the bit we +     * set in csched_cpu_pick() to be short-circuited away. */
-    if ( svc->pri == CSCHED_PRI_TS_UNDER &&
+    if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc-

Quoted 3 lines

Quoted 1 line

flags)  &&

+         svc->pri == CSCHED_PRI_TS_UNDER && !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )

Sorry to be late reviewing this.

So we always want to clear the 'migrating' flag, regardless of whether
we do anything with boosting.  Would that logic be clearer if we cleared
it as a separate step, storing the result in a local variable?  E.g.:

bool migrating;

...

/* Always clear migrating flag if it's set */
migrating = test_and_clear_bit(...)

if ( !migrating && ...) {
}

Then we wouldn't need the last paragraph in the comment.

That said, this is v3, so if you'd rather just get this in as it is, then you can have my Acked-by as well.

Quoted 23 lines · George Dunlap

Quoted 22 lines · Dario Faggioli

(quoted diff) +0 -0

@@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler
 *ops, struct vcpu *vc)
      * more CPU resource intensive VCPUs without impacting

overall
* system fairness.
*

xen/common/sched_credit.c +10 -4

-     * The one exception is for VCPUs of capped domains unpausing
-     * after earning credits they had overspent. We don't boost
-     * those.
+     * There are two cases, when we don't want to boost:
+     *  - VCPUs that are waking up after a migration, rather than
+     *    after having block;
+     *  - VCPUs of capped domains unpausing after earning credits
+     *    they had overspent.
+     *
+     * Note that checking whether we are "only" migrating must be
+     * done up front, as we do not want the clearing of the bit we
+     * set in csched_cpu_pick() to be short-circuited away.
      */
-    if ( svc->pri == CSCHED_PRI_TS_UNDER &&
+    if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc-

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

flags) &&

+ svc->pri == CSCHED_PRI_TS_UNDER &&

          !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )

Sorry to be late reviewing this.

No problem. Thanks for getting to it, actually, as I've got a few more patches stacked on top of these outstanding series.

Quoted 10 lines

So we always want to clear the 'migrating' flag, regardless of whether
we do anything with boosting. Would that logic be clearer if we cleared
it as a separate step, storing the result in a local variable? E.g.:

bool migrating;

...

Quoted 8 lines

/* Always clear migrating flag if it's set */
migrating = test_and_clear_bit(...)

if ( !migrating && ...) {
}

Then we wouldn't need the last paragraph in the comment.

Yes, I think I like this better.

Quoted 3 lines

That said, this is v3, so if you'd rather just get this in as it is, then you can have my Acked-by as well.

No, I'll resend... If I make (only) this change, can I resend directly with your Acked-by?

Quoted 4 lines · Dario Faggioli

No, I'll resend... If I make (only) this change, can I resend directly
with your Acked-by?

Well I won't know what it looks like until I see it, will I? :-) I prioritize recently-reviewed series, so if you resend I should be able
to give an Acked-by the same day.