Re: [Xen-devel] [PATCH 03 of 11 v4] xen: sched_credit: when picking, make sure we get an idle one, if any

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On ven, 2013-03-15 at 08:14 +0000, Jan Beulich wrote:
> >>> On 15.03.13 at 03:30, Dario Faggioli <dario.faggioli@xxxxxxxxxx> wrote:
> > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> > --- a/xen/common/sched_credit.c
> > +++ b/xen/common/sched_credit.c
> > @@ -532,6 +532,18 @@ static int
> >      if ( vc->processor == cpu && IS_RUNQ_IDLE(cpu) )
> >          cpumask_set_cpu(cpu, &idlers);
> >      cpumask_and(&cpus, &cpus, &idlers);
> > +
> > +    /*
> > +     * It is important that cpu points to an idle processor, if a suitable
> > +     * one exists (and we can use cpus to check and, possibly, choose a new
> > +     * CPU, as we just &&-ed it with idlers). In fact, if we are on SMT, 
> > and
> > +     * cpu points to a busy thread with an idle sibling, both the threads
> > +     * will be considered the same, from the "idleness" calculation point
> > +     * of view", preventing vcpu from being moved to the thread that is
> > +     * actually idle.
> > +     */
> > +    if ( !cpumask_empty(&cpus) && !cpumask_test_cpu(cpu, &cpus) )
> 
> I think I had asked about this before - 
>
Did you? I don't remember anything like that but, if you did and I did
not answer, sorry for that! :-P

> what's the point of the
> left hand side of the &&? If the mask is empty, the right hand
> side will cover that quite well, at much less a price for high
> NR_CPUS (or nr_cpu_ids).
> 
And in fact it was not there, but ISTR having to add it because not
having it was leading in some very bad Xen crash... If I remember
correctly, this is what happens without it.

The code looks like this: 

    if ( !cpumask_empty(&cpus) && !cpumask_test_cpu(cpu, &cpus) )
        cpu = cpumask_cycle(cpu, &cpus);
    cpumask_clear_cpu(cpu, &cpus);

    while ( !cpumask_empty(&cpus) ) {
        ...
    }
    ...
    return cpu;

So, what happens if cpus is actually empty? As you say
cpumask_test_cpu(cpu,&cpus) will be false, which means cpu is updated
with the result of cpumask(cycle(cpu,&cpus)). If I'm reading the code
correctly, a cpumask_cycle() on an empty cpumask will give me
nr_cpu_ids, which is then what is returned (the while loop is not
entered, so nothing more happens to cpu), which makes things explode...
Does that make sense?

Time has passed since I saw that bugtrace, so it is possible that my
memories are not accurate... I surely can try to reproduce it, if you
want to see the "smoking gun" :-)

Perhaps I can turn the condition into something like this:

    if ( !cpumask_test_cpu(cpu, &cpus) )
        cpu = cpumask_empty(&cpus) ? cpu : cpumask_cycle(cpu, &cpus);

So that we pay the price less frequently?

> The ASSERT() a few lines earlier
> could be simplified in similar ways, btw.
> 
You mean this, right?

    online = cpupool_scheduler_cpumask(vc->domain->cpupool);
    cpumask_and(&cpus, online, vc->cpu_affinity);
    cpu = cpumask_test_cpu(vc->processor, &cpus)
            ? vc->processor
            : cpumask_cycle(vc->processor, &cpus);
    ASSERT( !cpumask_empty(&cpus) && cpumask_test_cpu(cpu, &cpus) );

Not sure. AFAIU the code, the ASSERT() is indeed willing to make sure
that cpus did not ended up being empty as a consequence of the
cpumask_and(), and that is done together with the cpumask_test_cpu()
just to have only one ASSERT() instead of two, but again, I might well
be wrong.

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: signature.asc
Description: This is a digitally signed message part

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