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

Hi,

This series aims at stopping the Credit1 scheduler from granting BOOST priority to vcpus that are migrated to a new pcpu.

In fact, BOOST is for vcpus that are waking up after, e.g., having received an event, to, well, "boost" their wakeup latency and get them quickly to handle the event itself. However, since vcpu_migrate() basically forces the vcpu into a sleep+wakeup cycle, vcpus being migrated to a new pcpu, were also being granted BOOST priority, and that is not correct.

Performance does not seem to be affected much, neither in good or bad way, by this series. In fact, I've run a few benchmarks:
- I've considered both a Xen build, and the Iperf network benchmark, running either inside 1 or 2 (HVM) VMs;
- VMs had 12 vcpus, on a 16 pcpus host. Therefore, when only 1 VM is used, the host has some "spare" pcpus, while when 2 VMs where used, the host was overloaded;
- each configuration was repeated 10 times, and I'm showing average and standard deviation;
- I've considered the following configurations:
1) `make xen' run inside 1 VM
2) `make xen' run, concurrently, inside 2 VMs
3) Iperf run inside 1 VM
4) Iperf run, concurretnly, inside 2 VMs
5) `make xen' and Iperf run, concurrently, each one inside one of the VMs

*** *** ***
1) XEN BUILD INSIDE 1 HVM GUEST (lower is better)
-------------------------------

baseline :  20.673+/-0.263
patched  :  20.812+/-0.797

2) XEN BUILD INSIDE 2 HVM GUESTS, CONCURRENTLY (lower is better)
----------------------------------------------

            vm1             vm2             overall
baseline :  35.641+/-1.236  35.104+/-0.392  35.372+/-0.934
patched  :  35.251+/-0.527  35.341+/-0.811  35.296+/-0.667

3) IPERF FROM 1 HVM GUEST TO HOST (higher is better)
---------------------------------

baseline :  25.25+/-0.63
patched  :  25.56+/-1.90

4) IPERF FROM 2 HVM GUESTS TO HOST, CONCURRENTLY (lower is better)
------------------------------------------------

            vm1            vm2           overall
baseline :  13.01+/-1.27   14.25+/-0.97  13.63+/-1.27
patched  :  13.29+/-1.87   14.11+/-1.48  13.70+/-1.69

5) MAKEXEN & IPERF IN ONE HVM GUEST EACH, CONCURRENTLY
------------------------------------------------------

           makexen          iperf
baseline : 30.483+/-0.843   14.92+/-1.59
patched  : 30.885+/-0.882   16.42+/-1.74

*** *** ***

So, basically:
* 1 and 2 shows that we negligibly regress on a Xen build done in underload conditions, as well as we negligibly improve on a Xen build (well multiple Xen builds) done in overload.
* 3 and 4, show negligible to small improvement in Iperf performance, with the series applied.
* 5 shows that, in an heterogeneous workload, including both cpu load and I/O, the CPU load is slightly penalized by the series, while the I/O load benefits from it. This can probably be explained by the fact that, without this series, the CPU hungry vcpus were being boosted from time to time, even if they shouldn't have been, and this was impacting the vcpus doing I/O.

If anyone can and/or want to run more benchmarks (and/or include this patch in benchmarks that you routinely run... Marcus, Malcolm, Andrew, maybe?), I'd be interested in seeing the results.

In any case, I think non-boosting on migration is, conceptually, the right thing to do, and that's what is done in patch 3.

In order to be able to tell, inside Credit1 code, whether a vcpu is waking up after being blocked/asleep, or because of a migration, the wake scheduler hook is augmented (in patch 2) with a flag field. The caller of the hook can specify, by means of a set of flags, what has been the actual cause of the wakeup. Right now, after all the series is applied, only two flags exists:
'regular wakeups', and 'migration wakeups'. The mechanism is extensible, and we can add more i future, it there will come the need for this.

While there, the series introduces (patch 1) also some tracing and a new performance counter about Credit1 boosting logic, for making it easier to identify issues like this in future.

Thanks and Regards,
Dario
---
Dario Faggioli (3):
xen: credit1: trace vCPU boost/unboost
xen: sched: add wakeup flags to the scheduler interface
xen: credit1: avoid boosting vCPUs being "just" migrated

 xen/common/sched_arinc653.c  |    2 +-
 xen/common/sched_credit.c    |   21 ++++++++++++++++-----
 xen/common/sched_credit2.c   |    2 +-
 xen/common/sched_rt.c        |    2 +-
 xen/common/schedule.c        |   13 +++++++++----
 xen/include/xen/perfc_defn.h |    1 +
 xen/include/xen/sched-if.h   |    3 ++-
 xen/include/xen/sched.h      |   10 ++++++++++
 8 files changed, 41 insertions(+), 13 deletions(-)

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

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.

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

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

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;
     }
 

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

For making it possible to pass to the specific scheduler code information about the nature of the wakeup, and let it act accordingly, if necessary.

For now, this will only be useful to Credit1, that needs to differentiate between 'regular' wakeups (happening, for instance, because an I/O event), and wakeups "artificially induced" for migrating a vCPU to another pCPU.

In this patch, only the WF_wakeup flag is introduced, and used everything. In fact, instead of changing the prototype of vcpu_wake(), the _vcpu_wake() helper is introduced. This can change, in case, at some point, the majority of wakeup paths will end up wanting to pass a flag.

---
Cc: George Dunlap <[email protected]>
Cc: Josh Whitehead <[email protected]>
Cc: Robert VanVossen <[email protected]>
Cc: Meng Xu <[email protected]>

---
 xen/common/sched_arinc653.c |    2 +-
 xen/common/sched_credit.c   |    2 +-
 xen/common/sched_credit2.c  |    2 +-
 xen/common/sched_rt.c       |    2 +-
 xen/common/schedule.c       |    9 +++++++--
 xen/include/xen/sched-if.h  |    3 ++-
 xen/include/xen/sched.h     |    7 +++++++
 7 files changed, 20 insertions(+), 7 deletions(-)

index 0606988..b8ea596 100644
@@ -537,7 +537,7 @@ a653sched_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
  * @param vc        Pointer to the VCPU structure for the current domain
  */
 static void
-a653sched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
+a653sched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc, unsigned wf)
 {
     if ( AVCPU(vc) != NULL )
         AVCPU(vc)->awake = 1;

index 5708701..f728ddd 100644
@@ -983,7 +983,7 @@ csched_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
 }
 
 static void
-csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
+csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc, unsigned wf)
 {
     struct csched_vcpu * const svc = CSCHED_VCPU(vc);
     const unsigned int cpu = vc->processor;

index 78220a7..a8ba61d 100644
@@ -940,7 +940,7 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
 }
 
 static void
-csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
+csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc, unsigned wf)
 {
     struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
     s_time_t now = 0;

index 2e5430f..4ee1fce 100644
@@ -1022,7 +1022,7 @@ out:
  * TODO: what if these two vcpus belongs to the same domain?
  */
 static void
-rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
+rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc, unsigned wf)
 {
     struct rt_vcpu * const svc = rt_vcpu(vc);
     s_time_t now = NOW();

index 7306d71..ea74c96 100644
@@ -406,7 +406,7 @@ void vcpu_sleep_sync(struct vcpu *v)
     sync_vcpu_execstate(v);
 }
 
-void vcpu_wake(struct vcpu *v)
+static void _vcpu_wake(struct vcpu *v, unsigned wake_flags)
 {
     unsigned long flags;
     spinlock_t *lock = vcpu_schedule_lock_irqsave(v, &flags);

@@ -415,7 +415,7 @@ void vcpu_wake(struct vcpu *v)
     {
         if ( v->runstate.state >= RUNSTATE_blocked )
             vcpu_runstate_change(v, RUNSTATE_runnable, NOW());
-        SCHED_OP(VCPU2OP(v), wake, v);
+        SCHED_OP(VCPU2OP(v), wake, v, wake_flags);
     }
     else if ( !(v->pause_flags & VPF_blocked) )
     {

@@ -428,6 +428,11 @@ void vcpu_wake(struct vcpu *v)
     TRACE_2D(TRC_SCHED_WAKE, v->domain->domain_id, v->vcpu_id);
 }
 
+void vcpu_wake(struct vcpu *v)
+{
+    return _vcpu_wake(v, WF_wakeup);
+}
+
 void vcpu_unblock(struct vcpu *v)
 {
     if ( !test_and_clear_bit(_VPF_blocked, &v->pause_flags) )

index 66dc9c8..ea1cd4a 100644
@@ -144,7 +144,8 @@ struct scheduler {
     void         (*remove_vcpu)    (const struct scheduler *, struct vcpu *);
 
     void         (*sleep)          (const struct scheduler *, struct vcpu *);
-    void         (*wake)           (const struct scheduler *, struct vcpu *);
+    void         (*wake)           (const struct scheduler *, struct vcpu *,
+                                    unsigned int);
     void         (*yield)          (const struct scheduler *, struct vcpu *);
     void         (*context_saved)  (const struct scheduler *, struct vcpu *);
 

index b47a3fe..9fdcfff 100644
@@ -758,6 +758,13 @@ static inline struct domain *next_domain_in_cpupool(
 #define _VPF_in_reset        7
 #define VPF_in_reset         (1UL<<_VPF_in_reset)
 
+/*
+ * VCPU wake up flags.
+ */
+/* 'Default' wakeup. */
+#define _WF_wakeup           0
+#define WF_wakeup            (1U<<_WF_wakeup)
+
 static inline int vcpu_runnable(struct vcpu *v)
 {
     return !(v->pause_flags |

Of course you can :-), and I was already down to change this... But, as a matter of fact, this patch is not going to be present in v2.

Thanks and Regards,
Dario

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

Moving a vCPU to a different pCPU means blocking it and 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 end result 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 things by introducing a new wakeup flag, and using it to tag the wakeups that happens because of migrations (and avoid boosting, in these cases).

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

---
 xen/common/sched_credit.c |   11 +++++++----
 xen/common/schedule.c     |    4 ++--
 xen/include/xen/sched.h   |    3 +++
 3 files changed, 12 insertions(+), 6 deletions(-)

index f728ddd..2a23a0b 100644
@@ -1022,11 +1022,14 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc, unsigned wf)
      * 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 a couple of exceptions, 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.
      */
-    if ( svc->pri == CSCHED_PRI_TS_UNDER &&
+    if ( !(wf & WF_migrated) &&
+         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);

index ea74c96..c9a4f52 100644
@@ -581,8 +581,8 @@ static void vcpu_migrate(struct vcpu *v)
     if ( old_cpu != new_cpu )
         sched_move_irqs(v);
 
-    /* Wake on new CPU. */
-    vcpu_wake(v);
+    /* Wake on new CPU  (and let the scheduler know it's a migration). */
+    _vcpu_wake(v, WF_migrated);
 }
 
 /*

index 9fdcfff..5f426ad 100644
@@ -764,6 +764,9 @@ static inline struct domain *next_domain_in_cpupool(
 /* 'Default' wakeup. */
 #define _WF_wakeup           0
 #define WF_wakeup            (1U<<_WF_wakeup)
+/* Post-migration wakeup. */
+#define _WF_migrated         1
+#define WF_migrated          (1U<<_WF_migrated)
 
 static inline int vcpu_runnable(struct vcpu *v)
 {

It can indeed. I've coded it up, and I like the way it came out better.

I'm rerunning the benchmarks right now (just in case! :-)). I'll send v2 out as soon as they finish.

I did like the idea of "wakeup flags", and I think they may actually turn out useful, but they're not necessary for this specific use case, as it appears. Well, next time. ;-)

Thanks and Regards,
Dario

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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