Re: [Xen-devel] crash in csched_load_balance after xl vcpu-pin

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 04/10/2018 12:29 PM, Dario Faggioli wrote:
> On Tue, 2018-04-10 at 11:59 +0100, George Dunlap wrote:
>> On 04/10/2018 11:33 AM, Dario Faggioli wrote:
>>> On Tue, 2018-04-10 at 09:34 +0000, George Dunlap wrote:
>>>> Assuming the bug is this one:
>>>>
>>>> BUG_ON( cpu != snext->vcpu->processor );
>>>>
>>>
>>> Yes, it is that one.
>>>
>>> Another stack trace, this time from a debug=y built hypervisor, of
>>> what
>>> we are thinking it is the same bug (although reproduced in a
>>> slightly
>>> different way) is this:
>>>
>>> (XEN) ----[ Xen-4.7.2_02-36.1.12847.11.PTF  x86_64  debug=y  Not
>>> tainted ]----
>>> (XEN) CPU:    45
>>> (XEN) RIP:    e008:[<ffff82d08012508f>]
>>> sched_credit.c#csched_schedule+0x361/0xaa9
>>> ...
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d08012508f>]
>>> sched_credit.c#csched_schedule+0x361/0xaa9
>>> (XEN)    [<ffff82d08012c233>] schedule.c#schedule+0x109/0x5d6
>>> (XEN)    [<ffff82d08012fb5f>] softirq.c#__do_softirq+0x7f/0x8a
>>> (XEN)    [<ffff82d08012fbb4>] do_softirq+0x13/0x15
>>> (XEN)    [<ffff82d0801fd5c5>] vmx_asm_do_vmentry+0x25/0x2a
>>>
>>> (I can provide it all, if necessary.)
>>>
>>> I've done some analysis, although when we still were not entirely
>>> sure
>>> that changing the affinities was the actual cause (or, at least,
>>> what
>>> is triggering the whole thing).
>>>
>>> In the specific case of this stack trace, the current vcpu running
>>> on
>>> CPU 45 is d3v11. It is not in the runqueue, because it has been
>>> removed, and not added back to it, and the reason is it is not
>>> runnable
>>> (it has VPF_migrating on in pause_flags).
>>>
>>> The runqueue of pcpu 45 looks fine (i.e., it is not corrupt or
>>> anything
>>> like that), it has d3v10,d9v1,d32767v45 in it (in this order)
>>>
>>> d3v11->processor is 45, so that is also fine.
>>>
>>> Basically, d3v11 wants to move away from pcpu 45, and this might
>>> (but
>>> that's not certain) be the reson because we're rescheduling. The
>>> fact
>>> that there are vcpus wanting to migrate can very well be the cause
>>> of
>>> affinity being changed.
>>>
>>> Now, the problem is that, looking into the runqueue, I found out
>>> that
>>> d3v10->processor=32. I.e., d3v10 is queued in pcpu 45's runqueue,
>>> with
>>> processor=32, which really shouldn't happen.
>>>
>>> This leads to the bug triggering, as, in csched_schedule(), we read
>>> the
>>> head of the runqueue with:
>>>
>>> snext = __runq_elem(runq->next);
>>>
>>> and then we pass snext to csched_load_balance(), where the BUG_ON
>>> is.
>>>
>>> Another thing that I've found out, is that all "misplaced" vcpus
>>> (i.e.,
>>> in this and also in other manifestations of this bug) have their
>>> csched_vcpu.flags=4, which is CSCHED_FLAGS_VCPU_MIGRATING.
>>>
>>> This, basically, is again a sign of vcpu_migrate() having been
>>> called,
>>> on d3v10 as well, which in turn has called csched_vcpu_pick().

Right; csched_cpu_pick() is only called from csched_vcpu_insert(), and
from vcpu_migrate() and restore_vcpu_affinity().

Assuming we haven't been messing around with suspend / resume or
cpupools, that means it must have happened as a result of vcpu_migrate().

If it happened as a result of vcpu_migrate(), then it can only be set
between the very first call to pick_cpu(), and the next vcpu_wake() --
whenever that is.  (Possibly at the end of the current call to
vcpu_migrate(), possibly at the end of a vcpu_migrate() triggered in
context_saved() due to VPF_migrating.)

vcpu_migrate() is called from:
 - vcpu_force_reschedule(), which is called from
VCPUOP_{set,stop}_periodic_timer
 - cpu_disable_schedler(), when doing hotplug or cpupool operations on a cpu
 - vcpu_set_affinity()
 - vcpu_pin_override()

But in any case, v->processor is only set from vcpu_move_locked(), which
is only called if v->is_running is false; if v->is_running is false,
then one way or another v can't be on any runqueue.  And if v isn't on
any runqueue, and we hold v's current processor lock, then it's safe to
modify v->processor.

But obviously there's a flaw in that logic somewhere. :-)

One thing we might consider doing is implementing the migrate() callback
for the Credit scheduler, and just have it make a bunch of sanity checks
(v->processor lock held, new_cpu lock held, vcpu not on any runqueue, &c).

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel