Re: [Xen-devel] [PATCH 12 of 18] x86/mm: Make page_lock/unlock() in arch/x86/mm.c externally callable

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.11 at 03:54, "Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx> 
>>> wrote:
>>  At 02:47 -0500 on 08 Dec (1323312447), Andres Lagar-Cavilla wrote:
>>> This is necessary for a new consumer of page_lock/unlock to follow in
>>> the series.
>>>
>>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>
>> Nak, I'm afraid.
>>
>> These were OK as local functions but if they're going to be made
>> generally visible, they need clear comments describing what this
>> locking protects and what the discipline is for avoiding deadlocks.
> 
> How about something along the lines of
> "page_lock() is used for two purposes: pte serialization, and memory
> sharing. All users of page lock for pte serialization live in mm.c, use it
> to lock a page table page during pte updates, do not take other locks
> within the critical section delimited by page_lock/unlock, and perform no
> nesting. All users of page lock for memory sharing live in
> mm/mem_sharing.c. For memory sharing, nesting may happen when sharing (and
> locking) two pages -- deadlock is avoided by locking pages in increasing
> order. Memory sharing may take the p2m_lock within a page_lock/unlock
> critical section. These two users (pte serialization and memory sharing)
> should never collide, as long as page table pages are properly unshared
> prior to updating."

This would seem to me like very undesirable lock ordering - a very
coarse (per-domain) lock taken inside a very fine grained (per-page)
one.

> Now those are all pretty words, but here are the two things I (think I)
> need to do:
> - Prior to updating pte's, we do get_gfn on the page table page. We should
> be using get_gfn_unshare. Regardless of this patch. It's likely never
> going to trigger an actual unshare, yet better safe than sorry.

Does memory sharing work on pv domains at all?

> - I can wrap uses of page_lock in mm sharing in an "external"
> order-enforcing construct from mm-locks.h. And use that to scream deadlock
> between page_lock and p2m_lock.
> 
> The code that actually uses page_lock()s in the memory sharing code can be
> found in "[PATCH] x86/mm: Eliminate global lock in mem sharing code". It
> already orders locking of individual pages in ascending order.

It should be this patch to make the functions externally visible, not a
separate one (or at the very minimum the two ought to be in the same
series, back to back). Which is not to say that I'm fully convinced this
is a good idea.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel