On Wed, 2018-04-11 at 00:59 +0200, Dario Faggioli wrote: > [Adding Andrew, not because I expect anything, but just because > we've chatted about this issue on IRC :-) ] > Except, I did not add it. :-P
Anyway... > On Tue, 2018-04-10 at 22:37 +0200, Olaf Hering wrote: > > On Tue, Apr 10, Dario Faggioli wrote: > > > > BUG_ON(__vcpu_on_runq(CSCHED_VCPU(vc))); > > ... patch attached. Olaf, can you give it a try? It should be fine to run it on top of the last debug patch (the one that produced this crash). Regards, Dario > > (XEN) Xen BUG at sched_credit.c:876 > > (XEN) ----[ Xen-4.11.20180410T125709.50f8ba84a5- > > 3.bug1087289_411 x86_64 debug=y Not tainted ]---- > > (XEN) CPU: 118 > > (XEN) RIP: e008:[<ffff82d080229ab4>] > > sched_credit.c#csched_vcpu_migrate+0x27/0x51 > > ... > > (XEN) Xen call trace: > > (XEN) [<ffff82d080229ab4>] > > sched_credit.c#csched_vcpu_migrate+0x27/0x51 > > (XEN) [<ffff82d080236348>] schedule.c#vcpu_move_locked+0xbb/0xc2 > > (XEN) [<ffff82d08023764c>] schedule.c#vcpu_migrate+0x226/0x25b > > (XEN) [<ffff82d08023935f>] context_saved+0x8d/0x94 > > (XEN) [<ffff82d08027797d>] context_switch+0xe66/0xeb0 > > (XEN) [<ffff82d080236943>] schedule.c#schedule+0x5f4/0x627 > > (XEN) [<ffff82d080239f15>] softirq.c#__do_softirq+0x85/0x90 > > (XEN) [<ffff82d080239f6a>] do_softirq+0x13/0x15 > > (XEN) [<ffff82d08031f5db>] vmx_asm_do_vmentry+0x2b/0x30 > > > > Hey... unless I've really put there a totally bogous BUG_ON(), this > looks interesting and potentially useful. > > It says that the vcpu which is being context switched out, and on > which > we are calling vcpu_migrate() on, because we found it to be > VPF_migrating, is actually in the runqueue already when we actually > get > to execute vcpu_migrate()->vcpu_move_locked(). > > Mmm... let's see. > > CPU A CPU B > . . > schedule(current == v) vcpu_set_affinity(v) > prev = current // == v . > schedule_lock(CPU A) . > csched_schedule() schedule_lock(CPU A) > if (runnable(v)) //YES x > runq_insert(v) x > return next != v x > schedule_unlock(CPU A) x // takes the lock > context_switch(prev,next) set_bit(v, > VPF_migrating) [*] > context_saved(prev) // still == v . > v->is_running = 0 schedule_unlock(CPU A) > SMP_MB . > if (test_bit(v, VPF_migrating)) // YES!! > vcpu_migrate(v) . > for { . > schedule_lock(CPU A) . > SCHED_OP(v, pick_cpu) . > set_bit(v, CSCHED_MIGRATING) . > return CPU C . > pick_called = 1 . > schedule_unlock(CPU A) . > schedule_lock(CPU A + CPU C) . > if (pick_called && ...) // YES . > break . > } . > // v->is_running is 0 . > //!test_and_clear(v, VPF_migrating)) is false!! > clear_bit(v, VPF_migrating) . > vcpu_move_locked(v, CPU C) . > BUG_ON(__vcpu_on_runq(v)) . > > [*] after this point, and until someone manages to call > vcpu_sleep(), > v sits in CPU A's runqueue with the VPF_migrating pause flag > set > > So, basically, the race is between context_saved() and > vcpu_set_affinity(). Basically, vcpu_set_affinity() sets the > VPF_migrating pause flags on a vcpu in a runqueue, with the intent of > letting either a vcpu_sleep_nosync() or a reschedule remove it from > there, but context_saved() manage to see the flag, before the removal > can happen. > > And I think this explains also the original BUG at > sched_credit.c:1694 > (it's just a bit more involved). > > As it can be seen above (and also in the code comment in ) there is a > barrier (which further testify that this is indeed a tricky passage), > but I guess it is not that effective! :-/ > > TBH, I have actually never fully understood what that comment really > meant, what the barrier was protecting, and how... e.g., isn't it > missing its paired one? In fact, there's another comment, clearly > related, right in vcpu_set_affinity(). But again I'm a bit at loss at > properly figuring out what the big idea is. > > George, what do you think? Does this make sense? > > Well, I'll think more about this, and to a possible fix, tomorrow > morning. > > Regards, > Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Software Engineer @ SUSE https://www.suse.com/
commit 4d052ed2cb95dc69f45da6772b805f8e5beb654b Author: Dario Faggioli <dfaggi...@suse.com> Date: Wed Apr 11 09:03:19 2018 +0200 xen: sched: fix race between context switch and setting affinity vcpu_set_affinity() may set the VPF_migrating flag on the vcpu that is being context switched out, without having the chance to also call vcpu_sleep_nosync() on it, before that context switching code (in context_saved()) calls vcpu_migrate(). This, eventually, results in vcpu_move_locked() being called on a runnable vcpu, which causes various issues in sched_credit.c, sched_credit2.c, etc. For instance, when using Credit, it leads to this crash: https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00664.html Signed-off-by: Dario Faggioli <dfaggi...@suse.com> --- Cc: George Dunlap <george.dun...@citrix.com> Cc: Olaf Hering <o...@aepfle.de> Cc: Andrew Cooper <andrew.coop...@citrix.com> diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 343ab6306e..2a60301849 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1554,7 +1554,17 @@ void context_saved(struct vcpu *prev) SCHED_OP(vcpu_scheduler(prev), context_saved, prev); if ( unlikely(prev->pause_flags & VPF_migrating) ) + { + /* + * If someone (e.g., vcpu_set_affinity()) has set VPF_migrating + * on prev in between when schedule() releases the scheduler + * lock and here, we need to make sure we properly mark the + * vcpu as not runnable (and all it comes with that), with + * vcpu_sleep_nosync(), before calling vcpu_migrate(). + */ + vcpu_sleep_nosync(prev); vcpu_migrate(prev); + } } /* The scheduler timer: force a run through the scheduler */
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel