Re: [Xen-devel] get_gfn_query() locking

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Oct 30, 2012, at 11:06 AM, Tim Deegan <tim@xxxxxxx> wrote:

> 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);
> 
Has the side effect of leaking the current mfn (if any) in the p2m entry for 
the remainder of the lifetime of the domain.

Digression: is this something we want to just go ahead and fix? Or is that 
semantic intended?

> ?
> 
>>> 
>>> 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).

Good, same page then, phew
Andres

> 
> Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel