Re: [PATCH 1/3] xen: sched: introduce the 'null' semi-static scheduler

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

[-- Attachment #1.1: Type: text/plain, Size: 9015 bytes --]

On Mon, 2017-03-20 at 16:21 -0700, Stefano Stabellini wrote:
> On Fri, 17 Mar 2017, Dario Faggioli wrote:
> > 
> > --- /dev/null
> > +++ b/xen/common/sched_null.c

> > +/*
> > + * Virtual CPU
> > + */
> > +struct null_vcpu {
> > +    struct list_head waitq_elem;
> > +    struct null_dom *ndom;
> 
> This field is redundant, given that from struct vcpu you can get
> struct
> domain and from struct domain you can get struct null_dom. I would
> remove it.
> 
It's kind of a paradigm, followed by pretty much all schedulers. So
it's good to uniform to that, and it's often quite useful (or it may be
in future)... I'll have a look, though, at whether it is really
important to have it in this simple scheduler too, and do remove it if
it's not worth.

> > +    struct vcpu *vcpu;
> > +    int pcpu;          /* To what pCPU the vCPU is assigned (-1 if
> > none) */
> 
> Isn't this always the same as struct vcpu->processor?
> If it's only different when the vcpu is waiting in the waitq, then
> you
> can just remove this field and replace the pcpu == -1 test with
> list_empty(waitq_elem).
> 
I'll think about it. Right now, it's useful for ASSERTing and
consistency checking, which is something I want, at least in this
phase. It is also useful for reporting to what pcpu a vcpu is currently
assigned, for which thing you can't trust v->processor. That only
happens in `xl debug-key r' for now, but we may want to have less
tricky way of exporting such information in future.

Anyway, as I said, I'll ponder whether I can get rid of it.

> > +static void null_vcpu_remove(const struct scheduler *ops, struct
> > vcpu *v)
> > +{
> > +    struct null_private *prv = null_priv(ops);
> > +    struct null_vcpu *wvc, *nvc = null_vcpu(v);
> > +    unsigned int cpu;
> > +    spinlock_t *lock;
> > +
> > +    ASSERT(!is_idle_vcpu(v));
> > +
> > +    lock = vcpu_schedule_lock_irq(v);
> > +
> > +    cpu = v->processor;
> > +
> > +    /* If v is in waitqueue, just get it out of there and bail */
> > +    if ( unlikely(nvc->pcpu == -1) )
> > +    {
> > +        spin_lock(&prv->waitq_lock);
> > +
> > +        ASSERT(!list_empty(&null_vcpu(v)->waitq_elem));
> > +        list_del_init(&nvc->waitq_elem);
> > +
> > +        spin_unlock(&prv->waitq_lock);
> > +
> > +        goto out;
> > +    }
> > +
> > +    /*
> > +     * If v is assigned to a pCPU, let's see if there is someone
> > waiting.
> > +     * If yes, we assign it to cpu, in spite of v. If no, we just
> > set
> > +     * cpu free.
> > +     */
> > +
> > +    ASSERT(per_cpu(npc, cpu).vcpu == v);
> > +    ASSERT(!cpumask_test_cpu(cpu, &prv->cpus_free));
> > +
> > +    spin_lock(&prv->waitq_lock);
> > +    wvc = list_first_entry_or_null(&prv->waitq, struct null_vcpu,
> > waitq_elem);
> > +    if ( wvc )
> > +    {
> > +        vcpu_assign(prv, wvc->vcpu, cpu);
> 
> The vcpu_assign in null_vcpu_insert is protected by the pcpu runq
> lock,
> while this call is protected by the waitq_lock lock. Is that safe?
> 
vcpu assignment is always protected by the runqueue lock. Both in
null_vcpu_insert and() in here, we take it with:

 lock = vcpu_schedule_lock_irq(v);

at the beginning of the function (left in context, see above).

Taking the waitq_lock here serializes access to the waitqueue
(prv->waitqueue), i.e., the list_first_entry_or_null() call above, and
the list_del_init() call below.

> > +        list_del_init(&wvc->waitq_elem);
> > +        cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
> > +    }
> > +    else
> > +    {
> > +        vcpu_deassign(prv, v, cpu);
> > +    }
> > +    spin_unlock(&prv->waitq_lock);
> > +
> > + out:
> > +    vcpu_schedule_unlock_irq(lock, v);
> > +
> > +    SCHED_STAT_CRANK(vcpu_remove);
> > +}
> > +
> > +static void null_vcpu_wake(const struct scheduler *ops, struct
> > vcpu *v)
> > +{
> > +    ASSERT(!is_idle_vcpu(v));
> > +
> > +    if ( unlikely(curr_on_cpu(v->processor) == v) )
> > +    {
> > +        SCHED_STAT_CRANK(vcpu_wake_running);
> > +        return;
> > +    }
> > +
> > +    if ( null_vcpu(v)->pcpu == -1 )
> > +    {
> > +	/* Not exactly "on runq", but close enough for reusing the
> > counter */
> > +        SCHED_STAT_CRANK(vcpu_wake_onrunq);
> > +	return;
> 
> coding style
> 
Yeah... I need to double check the style of all the file (patch
series?). I mostly wrote this while travelling, with an editor set
differently from what I use when at home. I thought I had done that,
but I clearly missed a couple of sports. Sorry.

> > +static void null_vcpu_migrate(const struct scheduler *ops, struct
> > vcpu *v,
> > +                              unsigned int new_cpu)
> > +{
> > +    struct null_private *prv = null_priv(ops);
> > +    struct null_vcpu *nvc = null_vcpu(v);
> > +    unsigned int cpu = v->processor;
> > +
> > +    ASSERT(!is_idle_vcpu(v));
> > +
> > +    if ( v->processor == new_cpu )
> > +        return;
> > +
> > +    /*
> > +     * v is either in the waitqueue, or assigned to a pCPU.
> > +     *
> > +     * In the former case, there is nothing to do.
> > +     *
> > +     * In the latter, the pCPU to which it was assigned would
> > become free,
> > +     * and we, therefore, should check whether there is anyone in
> > the
> > +     * waitqueue that can be assigned to it.
> > +     */
> > +    if ( likely(nvc->pcpu != -1) )
> > +    {
> > +        struct null_vcpu *wvc;
> > +
> > +        spin_lock(&prv->waitq_lock);
> > +        wvc = list_first_entry_or_null(&prv->waitq, struct
> > null_vcpu, waitq_elem);
> > +        if ( wvc && cpumask_test_cpu(cpu,
> > cpupool_domain_cpumask(v->domain)) )
> > +        {
> > +            vcpu_assign(prv, wvc->vcpu, cpu);
> > +            list_del_init(&wvc->waitq_elem);
> > +            cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
> > +        }
> > +        else
> > +        {
> > +            vcpu_deassign(prv, v, cpu);
> > +        }
> > +        spin_unlock(&prv->waitq_lock);
> 
> This looks very similar to null_vcpu_remove, maybe you want to
> refactor
> the code and call a single shared service function.
> 
Yes, maybe I can.

> > +	SCHED_STAT_CRANK(migrate_running);
> > +    }
> > +    else
> > +        SCHED_STAT_CRANK(migrate_on_runq);
> > +
> > +    SCHED_STAT_CRANK(migrated);
> > +
> > +    /*
> > +     * Let's now consider new_cpu, which is where v is being sent.
> > It can be
> > +     * either free, or have a vCPU already assigned to it.
> > +     *
> > +     * In the former case, we should assign v to it, and try to
> > get it to run.
> > +     *
> > +     * In latter, all we can do is to park v in the waitqueue.
> > +     */
> > +    if ( per_cpu(npc, new_cpu).vcpu == NULL )
> > +    {
> > +        /* We don't know whether v was in the waitqueue. If yes,
> > remove it */
> > +        spin_lock(&prv->waitq_lock);
> > +        list_del_init(&nvc->waitq_elem);
> > +        spin_unlock(&prv->waitq_lock);
> > +
> > +        vcpu_assign(prv, v, new_cpu);
> 
> This vcpu_assign call seems to be unprotected. Should it be within a
> spin_lock'ed area?
> 
Lock is taken by the caller. Check calls to SCHED_OP(...,migrate) in
xen/common/schedule.c.

That's by design, and it's like that for most functions (with the sole
exceptions of debug dump and insert/remove, IIRC), for all schedulers.

> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index 223a120..b482037 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -1785,6 +1785,8 @@ int schedule_cpu_switch(unsigned int cpu,
> > struct cpupool *c)
> >  
> >   out:
> >      per_cpu(cpupool, cpu) = c;
> > +    /* Trigger a reschedule so the CPU can pick up some work ASAP.
> > */
> > +    cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
> >  
> >      return 0;
> >  }
> 
> Why? This change is not explained in the commit message.
> 
You're right. This may well go into it's own patch, actually. I'll see
how I like it better.

Thanks and 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.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