From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] sched: fix race between sched_move_domain() and vcpu_wake() Date: Fri, 11 Oct 2013 11:32:50 +0100 Message-ID: <5257D3D2.4020907@eu.citrix.com> References: <1381426196-11392-1-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1381426196-11392-1-git-send-email-david.vrabel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xen.org Cc: Andrew Cooper , Juergen Gross List-Id: xen-devel@lists.xenproject.org On 10/10/13 18:29, David Vrabel wrote: > From: David Vrabel > > sched_move_domain() changes v->processor for all the domain's VCPUs. > If another domain, softirq etc. triggers a simultaneous call to > vcpu_wake() (e.g., by setting an event channel as pending), then > vcpu_wake() may lock one schedule lock and try to unlock another. > > vcpu_schedule_lock() attempts to handle this but only does so for the > window between reading the schedule_lock from the per-CPU data and the > spin_lock() call. This does not help with sched_move_domain() > changing v->processor between the calls to vcpu_schedule_lock() and > vcpu_schedule_unlock(). > > Fix the race by taking the schedule_lock for v->processor in > sched_move_domain(). > > Signed-off-by: David Vrabel > Cc: George Dunlap > Cc: Juergen Gross > Cc: Andrew Cooper > --- > > 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)? So going through the code and trying to reconstruct all the state in my head... If you look at vcpu_migrate(), it grabs both locks. But it looks like the main purpose for that is so that we can call the migrate SCHED_OP(), which for credit2 needs to do some mucking about with runqueues, and thus needs both locks. In the case of move_domain, this is unnecessary, since it is removed from the old scheduler and then added to the new one. In a sense, Andrew, you're right: if you change v->processor, then you no longer hold v's schedule lock (unless you do what vcpu_migrate() does, and grab the lock of the processor you're moving to as well). In this case, it doesn't matter, because you're just about to release the lock anyway. But it may be misleading to people in the future trying to figure out what the right thing is to do -- we should at very least put a comment saying that changing v->processor without having the new lock effectively unlocks v, so don't do any more changes to the processor state. (Or we can do as Keir says, and do the double-locking, but that's a bit of a pain, as you can see from vcpu_migrate().) But I think this patch is still not quite right: both v->processor and per_cpu(schedule_data, ...).schedule_lock may change under your feet; so you always need to do the lock in a loop, checking to make sure that you *still* have the right lock after you have actually grabbed it. The gears on this code are rusty, however, so please do double-check my thinking here... -George