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

[Keir Fraser <keir@xen.org>]

On 09/12/2011 14:57, "Tim Deegan" <tim@xen.org> wrote:

> At 18:54 -0800 on 08 Dec (1323370449), Andres Lagar-Cavilla 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@lagarcavilla.org>
>>> 
>>> 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."
> 
> Thanks.  Extracting from that and from Keir's response:
> 
> It serves both as a spinlock and to exclude any to the page-type of the
> page in question (by causing the get_page_type() functions to spin).

It does not cause the get_page_type() functions to spin. An attempt to get
another reference to the current type will succeed; an attempt to change the
type will immediately fail. From the p.o.v. of the type-tracking logic,
page_lock() simply takes a reference to the current type.

 -- Keir

> What it currently protects is all modifications to pages that have
> pagetable type.  This serialises PV PTE validations, both against other
> validations of the same PTE and against concurrent changes of the
> enclosing page's type.
> 
> Your planned use is to protect updates to the page-sharing state
> associated with a page.  Can you be clear about what exactly is protected?
> 
> The proposed locking discipline is that
> - page locks must be taken in increasing order of MFN, yes?
> - and that you must always take page locks before the p2m lock?
> 
> Is that about right?
> 
> Tim.