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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.13 at 03:30, Dario Faggioli <dario.faggioli@xxxxxxxxxx> wrote:
> The pcpu picking algorithm treats two threads of a SMT core the same.
> More specifically, if one is idle and the other one is busy, they both
> will be assigned a weight of 1. Therefore, when picking begins, if the
> first target pcpu is the busy thread (and if there are no other idle
> pcpu than its sibling), that will never change.
> 
> This change fixes this by ensuring that, before entering the core of
> the picking algorithm, the target pcpu is an idle one (if there is an
> idle pcpu at all, of course).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Acked-by: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
> Acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> 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 - 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).

The only thing to check is whether "cpu" is out of range (because
of "cpus" having been empty already before the cpumask_and()
right before your addition). The ASSERT() a few lines earlier
could be simplified in similar ways, btw.

Jan

> +        cpu = cpumask_cycle(cpu, &cpus);
>      cpumask_clear_cpu(cpu, &cpus);
>  
>      while ( !cpumask_empty(&cpus) )




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