Re: [BUG] mistakenly wake in Xen's credit scheduler

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2883 bytes --]

On Tue, 2015-10-27 at 14:11 -0600, suokun wrote:
> On Tue, Oct 27, 2015 at 3:44 AM, George Dunlap <dunlapg@umich.edu>
> wrote:
> > On Tue, Oct 27, 2015 at 5:59 AM, suokun <suokunstar@gmail.com>
> > wrote:

> Thank you for your reply. I have test credit2 this morning. The I/O
> performance is correct, however, the CPU accounting seems not
> correct.
> Here is my experiment on credit2:
> 
> VM-IO:          1-vCPU pinned to a pCPU, running netperf
> VM-CPU:      1-vCPU pinned the the same pCPU, running a while(1) loop
> The throughput of netperf is the same(941Mbps) as VM-IO runs alone.
> 
> However, when I use xl top to show the VM CPU utilization, VM-IO
> takes
> 73% of CPU time and VM-CPU takes 99% CPU time. Their sum is more than
> 100%. I doubt it is due to the CPU utilization accounting in credit2
> scheduler.
> 
Yeah, well, sorry, but even if we both (me and George) encouraged you
to try Credit2, that wasn't a great idea. :-(  In fact, you're using
pinning for this test, and Credit2 does not have pinning (yet)! :-P

That explains why utilizations are summing up to higher than 100%:
vCPUs are just not being confined to one processor.

Pinning for Credit2 is just around the corner. Let's try this again
when it will be there, ok? :-D

> > Also, do you have a patch to fix it in credit1? :-)
> > 
> 
> For the patch to my problem in credit1. I have two ideas:
> 
> 1) if the vCPU cannot migrate(e.g. pinned, CPU affinity, even only
> has
> one physical CPU), do not set the _VPF_migrating flag.
> 
Yep, that's step 1. I hadn't seen this mail, so I produced a patch
myself (see my other reply). Is it similar to your one? If you could
test it, it would be great.

Even after this is done, though, we still need to fix the fact that
Credit1 boosts vCPUs upon migrations, which looks utterly crazy to me!
I've got (drafted) patches for that too, but I want to stress test them
a bit more before submitting them officially. I'm attaching them to
this email, feel free to have a look and provide your views.

> 2) let the BOOST state can preempt with each other.
> 
Yeah, but...

> Actually I have tested both separately and they both work. But
> personally I prefer the first option because it solved the problem
> from the source.
> 
... I don't like 2) that much either. Credit1 is, by design, round
-robin within equal priority levels. There are already quite a few
hacks in that code, and breaking even that rather basic assumption
would scary an awful lot!! :-O

Thanks a lot again fro your report and your analysis.

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)


[-- Attachment #1.1.2: xen-credit1-avoid-boosting-when-migrating.patch --]
[-- Type: text/x-patch, Size: 1350 bytes --]

commit 6d60b1ecf7d79d00d946ae19a52f256d4bc2a823
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Wed Oct 28 01:15:35 2015 +0100

    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 683feeb..29a9175 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1005,15 +1005,17 @@ 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:
+     *  - VCPUs of capped domains unpausing after earning credits
+     *    they had overspent;
+     *  - VCPUs that are being migrated to another pCPU, rather
+     *    than actually waking up after being blocked.
+     * We don't boost those.
      */
     if ( svc->pri == CSCHED_PRI_TS_UNDER &&
-         !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
-    {
+         !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) &&
+         !(wf & WF_migrating) )
         svc->pri = CSCHED_PRI_TS_BOOST;
-    }
 
     /* Put the VCPU on the runq and tickle CPUs */
     __runq_insert(cpu, svc);

[-- Attachment #1.1.3: xen-sched-introduce-wakeup-flags.patch --]
[-- Type: text/x-patch, Size: 4984 bytes --]

commit b71e3f10898a6d1c849bd9135f15aa8367d897d0
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Wed Oct 28 00:47:17 2015 +0100

    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index dbe02ed..de65e96 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -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;
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index b8f28fe..683feeb 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -966,7 +966,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;
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6695729..6b32778 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -957,7 +957,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;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 6a341b1..677e3f4 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1031,7 +1031,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();
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c5f640f..bacee73 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -407,7 +407,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);
@@ -416,7 +416,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) )
     {
@@ -429,6 +429,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) )
@@ -577,8 +582,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_migrating);
 }
 
 /*
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 493d43f..af1ed60 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -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 *);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3729b0f..6e7a108 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -753,6 +753,16 @@ 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.
+ */
+/* VCPU being actually woken up. */
+#define _WF_wakeup           0
+#define WF_wakeup            (1U<<_WF_wakeup)
+/* VCPU being (woken just after having been) migrated. */
+#define _WF_migrating        1
+#define WF_migrating         (1U<<_WF_migrating)
+
 static inline int vcpu_runnable(struct vcpu *v)
 {
     return !(v->pause_flags |

[-- 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