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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 26/07/16 10:30, Dario Faggioli wrote:
> 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@xxxxxxxxxx>
>> ---
>> v2:
>> - Actually grab lock before calling vcpu_schedule_lock() to avoid
>>   tripping over a new ASSERT
>>
> Ah, yes... sorry! :-P

Haha, no worries.

> 
> 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?

Ah, yes -- thanks for catching that. :-)

> With that off (and I'd be fine if you want to do that while
> committing):
> 
> Reviwed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Great, thanks.

 -George


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