Re: [PATCH v2 2/3] xen: Have schedulers revise initial placement

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

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

On Mon, 2016-07-25 at 16:28 +0100, George Dunlap wrote:
> The generic domain creation logic in
> xen/common/domctl.c:default_vcpu0_location() attempts to try to do
> initial placement load-balancing by placing vcpu 0 on the least-busy
> non-primary hyperthread available.  Unfortunately, the logic can end
> up picking a pcpu that's not in the online mask.  When this is passed
> to a scheduler such which assumes that the initial assignment is
> valid, it causes a null pointer dereference looking up the runqueue.
> 
Looking more at Credit2 code, I think there are a couple of missing
checks that a cpu that is about to be used for something, is actually
in online.

However, that is orthogonal with this patch and...

> Furthermore, this initial placement doesn't take into account hard or
> soft affinity, or any scheduler-specific knowledge (such as historic
> runqueue load, as in credit2).
> 
... using pick_cpu() here is, IMO, a really really really good idea, so
I think this patch should go in (and I'll work on and, if I am right,
add the missing checks).

> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> v2:
> - Actually grab lock before calling vcpu_schedule_lock() to avoid
>   tripping over a new ASSERT
> 
Ah, yes... sorry! :-P

Just one thing:

> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -2055,12 +2055,21 @@ csched2_vcpu_insert(const struct scheduler
> *ops, struct vcpu *vc)
>      ASSERT(!is_idle_vcpu(vc));
>      ASSERT(list_empty(&svc->runq_elem));
>  
> -    /* Add vcpu to runqueue of initial processor */
> +    /* csched2_cpu_pick() expects the pcpu lock to be held */
>      lock = vcpu_schedule_lock_irq(vc);
>  
> +    vc->processor = csched2_cpu_pick(ops, vc);
> +
> +    spin_unlock_irq(lock);
> +
> +    lock = vcpu_schedule_lock_irq(vc);
> +
> +    /* Add vcpu to runqueue of initial processor */
>      runq_assign(ops, vc);
>  
>      vcpu_schedule_unlock_irq(lock, vc);
> +    
> +    local_irq_enable();
> 
This local_irq_enable() is not necessary any longer, is it?

With that off (and I'd be fine if you want to do that while
committing):

Reviwed-by: Dario Faggioli <dario.faggioli@citrix.com>

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