| Message ID | 148664875514.595.468730348267549995.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > 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> Hmm, not sure how I missed that -- I feel like I was calling "burn_credits()" for other vcpus all the time. Must have missed one. Anyway: Reviewed-by: George Dunlap <george.dunlap@citrix.com>
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index bfb4891..8057abf 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -1341,18 +1341,28 @@ 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; ASSERT(!is_idle_vcpu(svc->vcpu)); ASSERT(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 - * common case. */ + /* + * Add INIT * m, avoiding integer multiplication in the common case. + */ if ( likely(m==1) ) svc->credit += CSCHED2_CREDIT_INIT; else
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> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Anshul Makkar <anshul.makkar@citrix.com> --- xen/common/sched_credit2.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)