Re: [Xen-devel] [PATCH v4 2/4] x86/mem_sharing: copy a page_lock version to be internal to memshr

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 5/3/19 2:57 PM, Jan Beulich wrote:
>>>> On 03.05.19 at 15:43, <tamas@xxxxxxxxxxxxx> wrote:
>> On Fri, May 3, 2019 at 2:27 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>
>>>>>> On 03.05.19 at 00:13, <tamas@xxxxxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>>> @@ -112,13 +112,48 @@ static inline void page_sharing_dispose(struct 
>>>> page_info *page)
>>>>
>>>>  #endif /* MEM_SHARING_AUDIT */
>>>>
>>>> -static inline int mem_sharing_page_lock(struct page_info *pg)
>>>> +/*
>>>> + * Private implementations of page_lock/unlock to bypass PV-only
>>>> + * sanity checks not applicable to mem-sharing.
>>>> + */
>>>> +static inline bool _page_lock(struct page_info *page)
>>>>  {
>>>> -    int rc;
>>>> +    unsigned long x, nx;
>>>> +
>>>> +    do {
>>>> +        while ( (x = page->u.inuse.type_info) & PGT_locked )
>>>> +            cpu_relax();
>>>> +        nx = x + (1 | PGT_locked);
>>>> +        if ( !(x & PGT_validated) ||
>>>> +             !(x & PGT_count_mask) ||
>>>> +             !(nx & PGT_count_mask) )
>>>> +            return false;
>>>
>>> Just for my own understanding: Did you verify that the PGT_validated
>>> check is indeed needed here, or did you copy it "just in case"? In the
>>> latter case a comment may be worthwhile.
>>
>> This is an exact copy of page_lock, sans the asserts that break it
>> from mem_sharing. I didn't investigate which of these flags are
>> necessary for mem_sharing. Frankly, I don't fully understand their
>> meaning and I haven't came across documentation about it yet.

I hope to add some documentation sometime in the next 6 months or so.
I've submitted a talk on the refcounting system to the XenSummit as well.

Short answer: PGT_validated means that the page in question has been
validated as safe to use as the designated type.  For PGT_writable, this
is instantaneous (i.e., the type is set to PGT_writable with the
PGT_validated bit set in the same cmpxchg operation).

Other types (such as PGT_l*_table) actually take time to verify, and so
you need to first change the type to the new type (so nobody tries to
use it as yet a different type), then verify that it's OK to use it as a
page table (by recursively checking all the entries), then set the
PGT_validated bit.

> Yes something along these lines is what I'm after. But "these flags"
> really is just PGT_validated.

Here's my take:

The sorts of "needs validation" types are PV-only I believe; if
mem_sharing is only for HVM guests, then the types should only be
PGT_writable, for which PGT_validated should always be set.

*If* we somehow do get to a situation where the type count > 0 but
PGT_validated is clear, something has probably gone terribly wrong; and
it would probably be much safer to just fail the page lock.

But perhaps that implies there should also be an ASSERT(!(x &
PGT_validated))?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel