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 12:32:43 +0100 Message-ID: <5257E1DB.8070002@eu.citrix.com> References: <1381426196-11392-1-git-send-email-david.vrabel@citrix.com> <5257D3D2.4020907@eu.citrix.com> <1381490132.5006.33.camel@Abyss.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1381490132.5006.33.camel@Abyss.lan> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: Andrew Cooper , Juergen Gross , David Vrabel , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 11/10/13 12:15, Dario Faggioli wrote: > On ven, 2013-10-11 at 11:32 +0100, George Dunlap wrote: >> 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. >> > Just to make sure I understood what's going on, the SCHED_OP you're > referring is SCHED_OP(..,migrate,..) here, right? > >> In the case of move_domain, this is unnecessary, >> since it is removed from the old scheduler and then added to the new one. >> > I think that too, and that's why I wouldn't take both locks: it'd > actually be misleading rather than enlightening for people reading the > code, at least that's how I see it. > > Perhaps, we can put a comment somewhere (as George is also saying). > > Regarding the patch, I personally like Jan's idea. > >> 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. >> > Which, if I'm not mistaken, we sort of get for free it we go Jan's way, > don't we? You mean, we could just call vcpu_schedule_lock..() instead of writing a bespoke loop code? Sure, that's definitely an advantage. -George