Re: [PATCH] sched: fix race between sched_move_domain() and vcpu_wake()

[Keir Fraser <keir.xen@gmail.com>]

On 10/10/2013 19:01, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

>> Just taking the lock for the old processor seemed sufficient to me as
>> anything seeing the new value would lock and unlock using the same new
>> value.  But do we need to take the schedule_lock for the new processor
>> as well (in the right order of course)?
> 
> David and I have been discussing this for a while, involving a
> whiteboard, and not come to a firm conclusion either way.
> 
> From my point of view, holding the appropriate vcpu schedule lock
> entitles you to play with vcpu scheduling state, which involves
> following v->sched_priv which we update outside the critical region later.
> 
> Only taking the one lock still leaves a race condition where another cpu
> can follow the new v->processor and obtain the schedule lock, at which
> point we have two threads both working on the internals of a vcpu.  The
> change below certainly will fix the current bug of locking one spinlock
> and unlocking another.
> 
> My gut feeling is that we do need to take both locks to be safe in terms
> of data access, but we would appreciate advice from someone more
> familiar with the scheduler locking.

If it's that tricky to work out, why not just take the two locks,
appropriately ordered? This isn't a hot path.

 -- Keir