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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Wed, 2018-04-11 at 11:37 +0100, George Dunlap wrote:
> On 04/10/2018 11:59 PM, Dario Faggioli wrote:
> > 
> > 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.
> > 
> Yep, that looks correct.  I had considered some sort of race between
> set_affinity() and context_switch(), but just never noticed that it
> could omit to take the vcpu off the runqueue.
> 
Yeah, it's very subtle. IN fact, when I considered that race, I was
assuming that, if we are in context_saved() with VPF_migrating set, the
vcpu can't be in any runqueue, as the scheduler would have seen it was
not runnable, and not queue it.

I was missing the fact that someone could raise the flag on a vcpu
which is already in the runqueue, between when the scheduler lock is
dropped, and this check in context_saved()!

> > 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.
> 
> I think the idea is to make sure that the change to v->is_running
> happens before whatever happens to come next (i.e., that the compiler
> doesn't reorder the write as part of its normal optimization
> activities).  As it happens nothing that comes next looks like it
> really
> needs such ordering (particularly as you can't reorder things over a
> function call, AFAIUI), but it's good to have those in place in case
> anybody *does* add that sort of thing.
> 
Sure, I wasn't planning to remove them. I was curious, and in
particular, I was curious of whether they were actually meant at trying
to prevent this (or a similar) race... I'll do a bit of archeology, if
I find some time.

Thanks and 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/

Attachment: signature.asc
Description: This is a digitally signed message part

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