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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, May 3, 2019 at 8:35 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> 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.

Great, looking forward to it!

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

In which case it does sound like it's a superfluous check but it's
also harmless.

>
> *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.

Isn't that exactly what happens?

if ( !(x & PGT_validated) ||
             !(x & PGT_count_mask) ||
             !(nx & PGT_count_mask) )
            return false;

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

Well, if the lock failed we already BUG out when it's for runtime
deduplication and return error when it's an operation from toolstack.
So I don't think we need that extra ASSERT.

Tamas

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