On 8/25/18 1:21 AM, Dario Faggioli wrote: > If a pCPU has been/is being offlined, we don't want it to be neither > assigned to any pCPU, nor in the wait list. > > Therefore, when we detect that a vcpu is going offline, remove it from > both places.
Hmm, this commit message wasn't very informative. It looks like what you really mean to do is: > Signed-off-by: Dario Faggioli <[email protected]> > --- > Cc: George Dunlap <[email protected]> > Cc: Stefano Stabellini <[email protected]> > Cc: Roger Pau Monne <[email protected]> > --- > xen/common/sched_null.c | 43 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c > index 1426124525..6259f4643e 100644 > --- a/xen/common/sched_null.c > +++ b/xen/common/sched_null.c > @@ -361,7 +361,8 @@ static void vcpu_assign(struct null_private *prv, struct > vcpu *v, > } > } > > -static void vcpu_deassign(struct null_private *prv, struct vcpu *v) > +/* Returns true if a cpu was tickled */ > +static bool vcpu_deassign(struct null_private *prv, struct vcpu *v) > { > unsigned int bs; > unsigned int cpu = v->processor; > @@ -406,11 +407,13 @@ static void vcpu_deassign(struct null_private *prv, > struct vcpu *v) > vcpu_assign(prv, wvc->vcpu, cpu); > cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ); > spin_unlock(&prv->waitq_lock); > - return; > + return true; > } > } > } > spin_unlock(&prv->waitq_lock); > + > + return false; > } > > /* Change the scheduler of cpu to us (null). */ > @@ -518,6 +521,14 @@ static void null_vcpu_remove(const struct scheduler > *ops, struct vcpu *v) > > lock = vcpu_schedule_lock_irq(v); > > + /* If offline, the vcpu shouldn't be assigned, nor in the waitqueue */ > + if ( unlikely(!is_vcpu_online(v)) ) > + { > + ASSERT(per_cpu(npc, v->processor).vcpu != v); > + ASSERT(list_empty(&nvc->waitq_elem)); > + goto out; > + } * Handle the case of an offline vcpu being removed (ASSERTing that it's neither on a processor nor on the waitqueue) But wait, isn't this fixing a important regression in patch 2? If after patch 2 but before patch 3, a VM is created with offline vcpus, and then destroyed, won't the offline vcpus reach here neither on the waitlist nor on a vcpu? Offlining/onlining vcpus is one thing; but creating and destroying guests is something different. > /* If v is in waitqueue, just get it out of there and bail */ > if ( unlikely(!list_empty(&nvc->waitq_elem)) ) > { > @@ -567,11 +578,31 @@ static void null_vcpu_wake(const struct scheduler *ops, > struct vcpu *v) > > static void null_vcpu_sleep(const struct scheduler *ops, struct vcpu *v) > { > + struct null_private *prv = null_priv(ops); > + unsigned int cpu = v->processor; > + bool tickled = false; > + > ASSERT(!is_idle_vcpu(v)); > > + /* We need to special case the handling of the vcpu being offlined */ > + if ( unlikely(!is_vcpu_online(v)) ) > + { > + struct null_vcpu *nvc = null_vcpu(v); > + > + printk("YYY d%dv%d going down?\n", v->domain->domain_id, v->vcpu_id); > + if ( unlikely(!list_empty(&nvc->waitq_elem)) ) > + { > + spin_lock(&prv->waitq_lock); > + list_del_init(&nvc->waitq_elem); > + spin_unlock(&prv->waitq_lock); > + } > + else if ( per_cpu(npc, cpu).vcpu == v ) > + tickled = vcpu_deassign(prv, v); > + } * Handle the unexpected(?) case of a vcpu being put to sleep as(?) it's offlined If it's not unexpected, then why the printk? And if it is unexpected, what is the expected path for a cpu going offline to be de-assigned from a vcpu (which is what the title seems to imply this patch is about)? > + > /* If v is not assigned to a pCPU, or is not running, no need to bother > */ > - if ( curr_on_cpu(v->processor) == v ) > - cpu_raise_softirq(v->processor, SCHEDULE_SOFTIRQ); > + if ( likely(!tickled && curr_on_cpu(cpu) == v) ) > + cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ); > > SCHED_STAT_CRANK(vcpu_sleep); > } > @@ -615,12 +646,12 @@ static void null_vcpu_migrate(const struct scheduler > *ops, struct vcpu *v, > * > * In the latter, there is just nothing to do. > */ > - if ( likely(list_empty(&nvc->waitq_elem)) ) > + if ( likely(per_cpu(npc, v->processor).vcpu == v) ) > { > vcpu_deassign(prv, v); > SCHED_STAT_CRANK(migrate_running); > } > - else > + else if ( !list_empty(&nvc->waitq_elem) ) > SCHED_STAT_CRANK(migrate_on_runq); * Teach null_vcpu_migrate() that !on_waitqueue != on_vcpu. It looks like the comment just above this hunk is now out of date: "v is either assigned to a pCPU, or in the waitqueue." -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
