On Tue, Jan 17, 2017 at 5:26 PM, Dario Faggioli <dario.faggi...@citrix.com> wrote: > In fact, relying on the mask of what pCPUs belong to > which Credit2 runqueue is not enough. If we only do that, > when Credit2 is the boot scheduler, we may ASSERT() or > panic when moving a pCPU from Pool-0 to another cpupool. > > This is because pCPUs outside of any pool are considered > part of cpupool0. This puts us at risk of crash when those > same pCPUs are added to another pool and something > different than the idle domain is found to be running > on them. > > Note that, even if we prevent the above to happen (which > is the purpose of this patch), this is still pretty bad, > in fact, when we remove a pCPU from Pool-0: > - in Credit1, as we do *not* update prv->ncpus and > prv->credit, which means we're considering the wrong > total credits when doing accounting; > - in Credit2, the pCPU remains part of one runqueue, > and is hence at least considered during load balancing, > even if no vCPU should really run there. > > In Credit1, this "only" causes skewed accounting and > no crashes because there is a lot of `cpumask_and`ing > going on with the cpumask of the domains' cpupool > (which, BTW, comes at a price). > > A quick and not to involved (and easily backportable) > solution for Credit2, is to do exactly the same. > > Signed-off-by: Dario Faggioli <dario.faggi...@citrix.com
Blech. But I agree we need a fix we can backport: Acked-by: George Dunlap <george.dun...@citrix.com> > --- > Cc: George Dunlap <george.dun...@eu.citrix.com> > Cc: Juergen Gross <jgr...@suse.com> > Cc: Jan Beulich <jbeul...@suse.com> > --- > This is a bugfix, and should be backported to 4.8. > --- > The proper solution would mean calling deinit_pdata() when removing a pCPU > from > cpupool0, as well as a bit more of code reshuffling. > > And, although worth doing, it certainly will take more work, more time, and > will probably be hard (well, surely harder than this) to backport. > > Therefore, I'd argue in favor of both taking and backporting this change, > which > at least enables using Credit2 as default scheduler without risking a crash > when creating a second cpupool. > > Afterwards, a proper solution would be proposed for Xen 4.9. > > Finally, given the wide number of issues similar to this that I've found and > fixed in the last release cycle, I think it would be good to take a stab at > whether the interface between cpupools and the schedulers could not be > simplified. :-O > > Regards, > Dario > --- > xen/common/sched_credit2.c | 59 > ++++++++++++++++++++++++++++---------------- > 1 file changed, 38 insertions(+), 21 deletions(-) > > diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c > index 523922e..ce0e146 100644 > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -510,19 +510,22 @@ void smt_idle_mask_clear(unsigned int cpu, cpumask_t > *mask) > */ > static int get_fallback_cpu(struct csched2_vcpu *svc) > { > - int fallback_cpu, cpu = svc->vcpu->processor; > + struct vcpu *v = svc->vcpu; > + int cpu = v->processor; > > - if ( likely(cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity)) ) > - return cpu; > + cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity, > + cpupool_domain_cpumask(v->domain)); > > - cpumask_and(cpumask_scratch_cpu(cpu), svc->vcpu->cpu_hard_affinity, > - &svc->rqd->active); > - fallback_cpu = cpumask_first(cpumask_scratch_cpu(cpu)); > - if ( likely(fallback_cpu < nr_cpu_ids) ) > - return fallback_cpu; > + if ( likely(cpumask_test_cpu(cpu, cpumask_scratch_cpu(cpu))) ) > + return cpu; > > - cpumask_and(cpumask_scratch, svc->vcpu->cpu_hard_affinity, > - cpupool_domain_cpumask(svc->vcpu->domain)); > + if ( likely(cpumask_intersects(cpumask_scratch_cpu(cpu), > + &svc->rqd->active)) ) > + { > + cpumask_and(cpumask_scratch_cpu(cpu), &svc->rqd->active, > + cpumask_scratch_cpu(cpu)); > + return cpumask_first(cpumask_scratch_cpu(cpu)); > + } > > ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu))); > > @@ -940,6 +943,9 @@ runq_tickle(const struct scheduler *ops, struct > csched2_vcpu *new, s_time_t now) > (unsigned char *)&d); > } > > + cpumask_and(cpumask_scratch_cpu(cpu), new->vcpu->cpu_hard_affinity, > + cpupool_domain_cpumask(new->vcpu->domain)); > + > /* > * First of all, consider idle cpus, checking if we can just > * re-use the pcpu where we were running before. > @@ -952,7 +958,7 @@ runq_tickle(const struct scheduler *ops, struct > csched2_vcpu *new, s_time_t now) > cpumask_andnot(&mask, &rqd->idle, &rqd->smt_idle); > else > cpumask_copy(&mask, &rqd->smt_idle); > - cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > + cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu)); > i = cpumask_test_or_cycle(cpu, &mask); > if ( i < nr_cpu_ids ) > { > @@ -967,7 +973,7 @@ runq_tickle(const struct scheduler *ops, struct > csched2_vcpu *new, s_time_t now) > * gone through the scheduler yet. > */ > cpumask_andnot(&mask, &rqd->idle, &rqd->tickled); > - cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > + cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu)); > i = cpumask_test_or_cycle(cpu, &mask); > if ( i < nr_cpu_ids ) > { > @@ -983,7 +989,7 @@ runq_tickle(const struct scheduler *ops, struct > csched2_vcpu *new, s_time_t now) > */ > cpumask_andnot(&mask, &rqd->active, &rqd->idle); > cpumask_andnot(&mask, &mask, &rqd->tickled); > - cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > + cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu)); > if ( cpumask_test_cpu(cpu, &mask) ) > { > cur = CSCHED2_VCPU(curr_on_cpu(cpu)); > @@ -1525,6 +1531,9 @@ csched2_cpu_pick(const struct scheduler *ops, struct > vcpu *vc) > goto out; > } > > + cpumask_and(cpumask_scratch_cpu(cpu), vc->cpu_hard_affinity, > + cpupool_domain_cpumask(vc->domain)); > + > /* > * First check to see if we're here because someone else suggested a > place > * for us to move. > @@ -1536,13 +1545,13 @@ csched2_cpu_pick(const struct scheduler *ops, struct > vcpu *vc) > printk(XENLOG_WARNING "%s: target runqueue disappeared!\n", > __func__); > } > - else > + else if ( cpumask_intersects(cpumask_scratch_cpu(cpu), > + &svc->migrate_rqd->active) ) > { > - cpumask_and(cpumask_scratch_cpu(cpu), vc->cpu_hard_affinity, > + cpumask_and(cpumask_scratch_cpu(cpu), cpumask_scratch_cpu(cpu), > &svc->migrate_rqd->active); > new_cpu = cpumask_any(cpumask_scratch_cpu(cpu)); > - if ( new_cpu < nr_cpu_ids ) > - goto out_up; > + goto out_up; > } > /* Fall-through to normal cpu pick */ > } > @@ -1570,12 +1579,12 @@ csched2_cpu_pick(const struct scheduler *ops, struct > vcpu *vc) > */ > if ( rqd == svc->rqd ) > { > - if ( cpumask_intersects(vc->cpu_hard_affinity, &rqd->active) ) > + if ( cpumask_intersects(cpumask_scratch_cpu(cpu), &rqd->active) ) > rqd_avgload = max_t(s_time_t, rqd->b_avgload - svc->avgload, > 0); > } > else if ( spin_trylock(&rqd->lock) ) > { > - if ( cpumask_intersects(vc->cpu_hard_affinity, &rqd->active) ) > + if ( cpumask_intersects(cpumask_scratch_cpu(cpu), &rqd->active) ) > rqd_avgload = rqd->b_avgload; > > spin_unlock(&rqd->lock); > @@ -1597,7 +1606,7 @@ csched2_cpu_pick(const struct scheduler *ops, struct > vcpu *vc) > goto out_up; > } > > - cpumask_and(cpumask_scratch_cpu(cpu), vc->cpu_hard_affinity, > + cpumask_and(cpumask_scratch_cpu(cpu), cpumask_scratch_cpu(cpu), > &prv->rqd[min_rqi].active); > new_cpu = cpumask_any(cpumask_scratch_cpu(cpu)); > BUG_ON(new_cpu >= nr_cpu_ids); > @@ -1713,6 +1722,8 @@ static void migrate(const struct scheduler *ops, > __runq_deassign(svc); > > cpumask_and(cpumask_scratch_cpu(cpu), svc->vcpu->cpu_hard_affinity, > + cpupool_domain_cpumask(svc->vcpu->domain)); > + cpumask_and(cpumask_scratch_cpu(cpu), cpumask_scratch_cpu(cpu), > &trqd->active); > svc->vcpu->processor = cpumask_any(cpumask_scratch_cpu(cpu)); > ASSERT(svc->vcpu->processor < nr_cpu_ids); > @@ -1738,8 +1749,14 @@ static void migrate(const struct scheduler *ops, > static bool_t vcpu_is_migrateable(struct csched2_vcpu *svc, > struct csched2_runqueue_data *rqd) > { > + struct vcpu *v = svc->vcpu; > + int cpu = svc->vcpu->processor; > + > + cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity, > + cpupool_domain_cpumask(v->domain)); > + > return !(svc->flags & CSFLAG_runq_migrate_request) && > - cpumask_intersects(svc->vcpu->cpu_hard_affinity, &rqd->active); > + cpumask_intersects(cpumask_scratch_cpu(cpu), &rqd->active); > } > > static void balance_load(const struct scheduler *ops, int cpu, s_time_t now) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel