Re: [PATCH v4 0/7] xen: credit2: improve style, and tracing; fix two bugs

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

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

On Wed, 2017-03-01 at 15:52 +0100, Dario Faggioli wrote:
> Dario Faggioli (7):
>       xen: credit2: always mark a tickled pCPU as... tickled!
>       xen: credit2: don't miss accounting while doing a credit reset.
>
And these two being bugfixes, I request them to be backported to Xen
4.7 (but not any further, as that would be difficult and pretty
pointless).

Since that isn't exactly trivial, especially for the first patch, I
attach to this email what I actually propose to backport.

I've successfully tested them on top of current tip of staging-4.7.

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-credit2-always-mark.patch --]
[-- Type: text/x-patch, Size: 2224 bytes --]

commit eb0c1b77cc65ad40efd61234f8eb0dc73cd80632
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Wed Mar 1 16:00:53 2017 +0100

    xen: credit2: always mark a tickled pCPU as... tickled!
    
    In fact, whether or not a pCPU has been tickled, and is
    therefore about to re-schedule, is something we look at
    and base decisions on in various places.
    
    So, let's make sure that we do that basing on accurate
    information.
    
    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 35dad15..f14dc9c 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -559,6 +559,13 @@ __runq_remove(struct csched2_vcpu *svc)
 
 void burn_credits(struct csched2_runqueue_data *rqd, struct csched2_vcpu *, s_time_t);
 
+static inline void
+tickle_cpu(unsigned int cpu, struct csched2_runqueue_data *rqd)
+{
+    cpumask_set_cpu(cpu, &rqd->tickled);
+    cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
+}
+
 /* Check to see if the item on the runqueue is higher priority than what's
  * currently running; if so, wake up the processor */
 static /*inline*/ void
@@ -660,9 +667,8 @@ tickle:
                   sizeof(d),
                   (unsigned char *)&d);
     }
-    cpumask_set_cpu(ipid, &rqd->tickled);
     SCHED_STAT_CRANK(tickle_idlers_some);
-    cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
+    tickle_cpu(ipid, rqd);
 
 no_tickle:
     return;
@@ -1027,7 +1033,7 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
     SCHED_STAT_CRANK(vcpu_sleep);
 
     if ( curr_on_cpu(vc->processor) == vc )
-        cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
+        tickle_cpu(vc->processor, svc->rqd);
     else if ( __vcpu_on_runq(svc) )
     {
         BUG_ON(svc->rqd != RQD(ops, vc->processor));
@@ -1308,8 +1314,8 @@ static void migrate(const struct scheduler *ops,
         svc->migrate_rqd = trqd;
         set_bit(_VPF_migrating, &svc->vcpu->pause_flags);
         set_bit(__CSFLAG_runq_migrate_request, &svc->flags);
-        cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
         SCHED_STAT_CRANK(migrate_requested);
+        tickle_cpu(cpu, svc->rqd);
     }
     else
     {

[-- Attachment #1.1.3: xen-credit2-don-t-miss.patch --]
[-- Type: text/x-patch, Size: 3514 bytes --]

commit f36ce03fffab7526d9c94b46028a27a752e3f60e
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Wed Mar 1 16:22:43 2017 +0100

    xen: credit2: don't miss accounting while doing a credit reset.
    
    A credit reset basically means going through all the
    vCPUs of a runqueue and altering their credits, as a
    consequence of a 'scheduling epoch' having come to an
    end.
    
    Blocked or runnable vCPUs are fine, all the credits
    they've spent running so far have been accounted to
    them when they were scheduled out.
    
    But if a vCPU is running on a pCPU, when a reset event
    occurs (on another pCPU), that does not get properly
    accounted. Let's therefore begin to do so, for better
    accuracy and fairness.
    
    In fact, after this patch, we see this in a trace:
    
     csched2:schedule cpu 10, rq# 1, busy, not tickled
     csched2:burn_credits d1v5, credit = 9998353, delta = 202996
     runstate_continue d1v5 running->running
     ...
     csched2:schedule cpu 12, rq# 1, busy, not tickled
     csched2:burn_credits d1v6, credit = -1327, delta = 9999544
     csched2:reset_credits d0v13, credit_start = 10500000, credit_end = 10500000, mult = 1
     csched2:reset_credits d0v14, credit_start = 10500000, credit_end = 10500000, mult = 1
     csched2:reset_credits d0v7, credit_start = 10500000, credit_end = 10500000, mult = 1
     csched2:burn_credits d1v5, credit = 201805, delta = 9796548
     csched2:reset_credits d1v5, credit_start = 201805, credit_end = 10201805, mult = 1
     csched2:burn_credits d1v6, credit = -1327, delta = 0
     csched2:reset_credits d1v6, credit_start = -1327, credit_end = 9998673, mult = 1
    
    Which shows how d1v5 actually executed for ~9.796 ms,
    on pCPU 10, when reset_credit() is executed, on pCPU
    12, because of d1v6's credits going below 0.
    
    Without this patch, this 9.796ms are not accounted
    to anyone. With this patch, d1v5 is charged for that,
    and its credits drop down from 9796548 to 201805.
    
    And this is important, as it means that it will
    begin the new epoch with 10201805 credits, instead
    of 10500000 (which he would have, before this patch).
    
    Basically, we were forgetting one round of accounting
    in epoch x, for the vCPUs that are running at the time
    the epoch ends. And this meant favouring a little bit
    these same vCPUs, in epoch x+1, providing them with
    the chance of execute longer than their fair share.
    
    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 905fb12..c86f548 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -707,14 +707,23 @@ static void reset_credit(const struct scheduler *ops, int cpu, s_time_t now,
 
     list_for_each( iter, &rqd->svc )
     {
+        unsigned int svc_cpu;
         struct csched2_vcpu * svc;
         int start_credit;
 
         svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
+        svc_cpu = svc->vcpu->processor;
 
         BUG_ON( is_idle_vcpu(svc->vcpu) );
         BUG_ON( svc->rqd != rqd );
 
+        /*
+         * If svc is running, it is our responsibility to make sure, here,
+         * that the credit it has spent so far get accounted.
+         */
+        if ( svc->vcpu == curr_on_cpu(svc_cpu) )
+            burn_credits(rqd, svc, now);
+
         start_credit = svc->credit;
 
         /* And add INIT * m, avoiding integer multiplication in the

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel