From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: get_gfn_query() locking Date: Tue, 30 Oct 2012 15:06:12 +0000 Message-ID: <20121030150612.GC34613@ocelot.phlegethon.org> References: <508FAA9402000078000A5612@nat28.tlf.novell.com> <20121030093626.GB34613@ocelot.phlegethon.org> <4E449D48-D643-46FB-91BD-B15EF21AAC57@gridcentric.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4E449D48-D643-46FB-91BD-B15EF21AAC57@gridcentric.ca> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar-Cavilla Cc: Andres Lagar-Cavilla , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org At 10:53 -0400 on 30 Oct (1351594401), Andres Lagar-Cavilla wrote: > >> And then again, with the p2m lock being recursive these > >> days, I don't think there's any harm calling the other methods > >> here with that lock held. > > Is the patch you refer to http://www.gossamer-threads.com/lists/xen/devel/261025 and the hunk in question the following? > + get_gfn_query(d, pfn, &pt); > + p2m_change_type(d, pfn, pt, p2m_ram_broken); > + put_gfn(d, pfn); > > There really is no way to get rid of that p2m lock-protected critical > section if the domain allows for paging etc. You might want to > introduce a syntactically cleaner unconditional p2m_change_type > variant that doesn't cmpxchg with the previous type -- that is > effectively what goes on here. Should be a tiny amount of refactoring > and the code will be cleaner, no need for query or put. I don't think that change-type is even what's wanted here. You want to use some more raw form of set_p2m_entry(), since keeping the MFN is not important. How about: guest_physmap_add_entry(d, pfn, MFN_INVALID, p2m_ram_broken); ? > > > > True, but it wouldn't be safe to call it with the paging lock held. > > OTOH since we're not seeing any crashes from the lock-ordering > > constraints maybe we don't do that. > > > > Andres, what do you think? Can we just drop/amend that comment? > > > > If you refer to removing the ordering constraints Noooooooooooooo! :) I think we should drop the comment that says it's OK to call get_gfn_query() with the paging lock held (and audit to check that we don't do that anywhere). Tim.