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 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 can
certainly add a comment saying TODO: figure out which of these flags
are actually needed.

> Furthermore, are there any mem-sharing specific checks reasonable
> to do here in place of the PV ones you want to avoid? Like pages
> making it here only ever being of PGT_shared_page type?

There are checks already in place after the lock is taken where
necessary. Those checks can't be moved in here because they don't
apply to all cases uniformly - for example, we also take the lock for
when the page type is being converted between shared and not shared.

> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -356,24 +356,12 @@ struct platform_bad_page {
> >  const struct platform_bad_page *get_platform_badpages(unsigned int
> > *array_size);
> >
> >  /* Per page locks:
> > - * page_lock() is used for two purposes: pte serialization, and memory 
> > sharing.
> > + * page_lock() is used for pte serialization.
> >   *
> >   * 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 
> > ithin
> >   * 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. 
> > Page_lock
> > - * is used in memory sharing to protect addition (share) and removal 
> > (unshare)
> > - * of (gfn,domain) tupples to a list of gfn's that the shared page is 
> > currently
> > - * backing. Nesting may happen when sharing (and locking) two pages -- 
> > deadlock
> > - * is avoided by locking pages in increasing order.
> > - * All memory sharing code paths take the p2m lock of the affected gfn 
> > before
> > - * taking the lock for the underlying page. We enforce ordering between 
> > page_lock
> > - * and p2m_lock using an mm-locks.h construct.
> > - *
> > - * These two users (pte serialization and memory sharing) do not collide, 
> > since
> > - * sharing is only supported for hvm guests, which do not perform pv pte 
> > updates.
> >   */
> >  int page_lock(struct page_info *page);
> >  void page_unlock(struct page_info *page);
>
> I think it would be helpful to retain (in a slightly adjusted form) the last
> sentence of the comment above, to clarify that the PGT_locked uses
> are now what does not end up colliding. At this occasion "which do not
> perform pv pte updates" would also better be re-worded to e.g.
> "which do not have PV PTEs updated" (as PVH Dom0 is very much
> expected to issue PV page table operations for PV DomU-s).

Sure.

Thanks,
Tamas

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