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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

>>>> On 09.12.11 at 15:53, "Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>
>>>> wrote:
>>>>>> 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.
>> I'm not sure I follow. Unshare would do its work, and then pte
>> serialization would start. The two pieces of code will be disjoint,
>> locking-wise.
>
> But your original mail said "Memory sharing may take the p2m_lock
> within a page_lock/unlock critical section" - see above. That's what
> I'm referring to.
>
>> Now it is true that during unshare we need to take the p2m lock to
>> change
>> the p2m entry. That's a very strong reason to make the p2m lock
>> fine-grained. But I need to start somewhere, so I'm breaking up the
>> global
>> shr_lock first.
>
> I don't really think that it'll be reasonable to split up the p2m lock.
>
>>>> 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?
>> Not. At. All :)
>>
>> I can _not_ add the unshare. It's idempotent to pv.
>
> Perhaps I should have clarified why I was asking: The pte handling is
> a pv-only thing, and if memory sharing is hvm only, then the two can't
> ever conflict.

Grant mapping uses the page_lock. I believe there is work trying to make
pv backends work in hvm?

If so, best to add unshare of the page table page now. Should a
free-wheeling user-space sharer try to share page table pages....

Andres
>
>>>> - 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.
>> Whichever you prefer. I'm of the mind of making shorter patches when
>> possible, that do one thing, to ease readability. But I can collapse the
>> two.
>
> In quite a few (recent) cases your patches did something where the user
> of the change wasn't obvious at all (in some cases - I tried to point
> these
> out - there was no user even at the end of a series). While I agree that
> shorter patches are easier to review, trivial changes like the one here
> should imo really be logically grouped with what requires them.
>
> Jan
>



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