[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3 of 3] Make p2m critical sections hold a ref on the underlying mfn



Hi, 

At 16:42 -0500 on 08 Nov (1320770546), Andres Lagar-Cavilla wrote:
> For translated domains, critical sections demarcated by a
> get_gfn/put_gfn pair will hold an additional ref on the
> underlying mfn.

Remind me what this gets us again?  Is it just belt-and-braces on top of
the locking at the p2m layer?

It should be possible to do this without the extra argument - for
example, the bottom-level function that actually changes a p2m entry
must hold the p2m exclusion lock for that entry, or the master p2m lock,
right?  In either case it knows whether it has a ref on the mfn.

I suppose having these locks be recursive makes that a problem, but in
that case you have another problem -- how many mfn refs need to be
moved?

Cheers,

Tim (confused).

> This requires keeping tabs on special manipulations that
> change said mfn:
>  - physmap remove page
>  - sharing
>  - steal page
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm.c
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4128,7 +4128,7 @@ static int replace_grant_p2m_mapping(
>                   type, mfn_x(old_mfn), frame);
>          return GNTST_general_error;
>      }
> -    guest_physmap_remove_page(d, gfn, frame, 0);
> +    guest_physmap_remove_page(d, gfn, frame, 0, HOLDING_GFN);
>  
>      put_gfn(d, gfn);
>      return GNTST_okay;
> @@ -4248,8 +4248,10 @@ int donate_page(
>  }
>  
>  int steal_page(
> -    struct domain *d, struct page_info *page, unsigned int memflags)
> +    struct domain *d, struct page_info *page, unsigned int memflags,
> +    int holding_gfn)
>  {
> +    unsigned count;
>      unsigned long x, y;
>      bool_t drop_dom_ref = 0;
>  
> @@ -4261,11 +4263,14 @@ int steal_page(
>      /*
>       * We require there is just one reference (PGC_allocated). We temporarily
>       * drop this reference now so that we can safely swizzle the owner.
> +     * If, however, this is invoked by a caller holding the p2m entry, there
> +     * will be another reference.
>       */
> +    count = (holding_gfn) ? 1 : 2;
>      y = page->count_info;
>      do {
>          x = y;
> -        if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
> +        if ( (x & (PGC_count_mask|PGC_allocated)) != (count | PGC_allocated) 
> )
>              goto fail;
>          y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
>      } while ( y != x );
> @@ -4276,7 +4281,7 @@ int steal_page(
>      do {
>          x = y;
>          BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
> -    } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
> +    } while ( (y = cmpxchg(&page->count_info, x, x | count)) != x );
>  
>      /* Unlink from original owner. */
>      if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
> @@ -4779,7 +4784,8 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          {
>              if ( is_xen_heap_mfn(prev_mfn) )
>                  /* Xen heap frames are simply unhooked from this phys slot. 
> */
> -                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
> +                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0, 
> +                                            HOLDING_GFN);
>              else
>                  /* Normal domain memory is freed, to avoid leaking memory. */
>                  guest_remove_page(d, xatp.gpfn);
> @@ -4791,7 +4797,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          gpfn = get_gpfn_from_mfn(mfn);
>          ASSERT( gpfn != SHARED_M2P_ENTRY );
>          if ( gpfn != INVALID_M2P_ENTRY )
> -            guest_physmap_remove_page(d, gpfn, mfn, 0);
> +            guest_physmap_remove_page(d, gpfn, mfn, 0, (gpfn == gfn));
>  
>          /* Map at new location. */
>          rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -561,7 +561,7 @@ int mem_sharing_share_pages(shr_handle_t
>          list_del(&gfn->list);
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
> -        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn) == 0);
> +        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn, 0) == 0);
>          put_domain(d);
>          list_add(&gfn->list, &se->gfns);
>          put_page_and_type(cpage);
> @@ -670,14 +670,9 @@ gfn_found:
>      unmap_domain_page(s);
>      unmap_domain_page(t);
>  
> -    /* NOTE: set_shared_p2m_entry will switch the underlying mfn. If
> -     * we do get_page withing get_gfn, the correct sequence here
> -     * should be
> -       get_page(page);
> -       put_page(old_page);
> -     * so that the ref to the old page is dropped, and a ref to
> -     * the new page is obtained to later be dropped in put_gfn */
> -    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0);
> +    /* NOTE: set_shared_p2m_entry will swap the underlying mfn and the refs. 
> +     * It is safe to call put_gfn as usual after this. */
> +    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page), HOLDING_GFN) == 
> 0);
>      put_page_and_type(old_page);
>  
>  private_page_found:    
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -182,6 +182,13 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>      }
>  #endif
>  
> +    /* Do a get page to get one additional ref on the mfn */
> +    if ( mfn_valid(mfn) )
> +    {
> +        struct page_info *pg = mfn_to_page(mfn);
> +        BUG_ON( !page_get_owner_and_reference(pg) );
> +    }
> +
>      return mfn;
>  }
>  
> @@ -195,6 +202,21 @@ void put_gfn(struct domain *d, unsigned 
>  
>      ASSERT(p2m_locked_by_me(p2m));
>  
> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn = p2m->get_entry(p2m, gfn, &t, &a, 
> +                                    p2m_query, NULL);
> +
> +        if ( mfn_valid(mfn) )
> +        {
> +#ifdef __x86_64__
> +            if (likely( !(p2m_is_broken(t)) ))
> +#endif
> +                put_page(mfn_to_page(mfn));
> +        }
> +    }    
> +
>      p2m_unlock(p2m);
>  }
>  
> @@ -416,6 +438,28 @@ void p2m_final_teardown(struct domain *d
>      p2m_teardown_nestedp2m(d);
>  }
>  
> +/* If the caller has done get_gfn on this entry, then it has a ref on the
> + * old mfn. Swap the refs so put_gfn puts the appropriate ref */
> +static inline void __p2m_swap_entry_refs(struct p2m_domain *p2m, 
> +                                         unsigned long gfn, mfn_t mfn)
> +{
> +    p2m_type_t t;
> +    p2m_access_t a;
> +    mfn_t omfn;
> +
> +    if ( !paging_mode_translate(p2m->domain) )
> +        return;
> +
> +    ASSERT(gfn_locked_by_me(p2m, gfn));
> +
> +    omfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
> +
> +    if ( mfn_valid(mfn) )
> +        BUG_ON( !page_get_owner_and_reference(mfn_to_page(mfn)) );
> +
> +    if ( mfn_valid(omfn) )
> +        put_page(mfn_to_page(omfn));
> +}
>  
>  static void
>  p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
> @@ -451,11 +495,14 @@ p2m_remove_page(struct p2m_domain *p2m, 
>  
>  void
>  guest_physmap_remove_page(struct domain *d, unsigned long gfn,
> -                          unsigned long mfn, unsigned int page_order)
> +                          unsigned long mfn, unsigned int page_order,
> +                          int holding_gfn)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      p2m_lock(p2m);
>      audit_p2m(p2m, 1);
> +    if (holding_gfn)
> +        __p2m_swap_entry_refs(p2m, gfn, _mfn(INVALID_MFN));
>      p2m_remove_page(p2m, gfn, mfn, page_order);
>      audit_p2m(p2m, 1);
>      p2m_unlock(p2m);
> @@ -713,7 +760,8 @@ out:
>  }
>  
>  int
> -set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
> +set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, 
> +                        int holding_gfn)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      int rc = 0;
> @@ -730,6 +778,10 @@ set_shared_p2m_entry(struct domain *d, u
>       * sharable first */
>      ASSERT(p2m_is_shared(ot));
>      ASSERT(mfn_valid(omfn));
> +
> +    if ( holding_gfn )
> +        __p2m_swap_entry_refs(p2m, gfn, mfn);
> +
>      /* XXX: M2P translations have to be handled properly for shared pages */
>      set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/grant_table.c
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1497,7 +1497,7 @@ gnttab_transfer(
>              goto copyback;
>          }
>  
> -        if ( steal_page(d, page, 0) < 0 )
> +        if ( steal_page(d, page, 0, HOLDING_GFN) < 0 )
>          {
>              put_gfn(d, gop.mfn);
>              gop.status = GNTST_bad_page;
> @@ -1505,7 +1505,7 @@ gnttab_transfer(
>          }
>  
>  #ifndef __ia64__ /* IA64 implicitly replaces the old page in steal_page(). */
> -        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
> +        guest_physmap_remove_page(d, gop.mfn, mfn, 0, HOLDING_GFN);
>  #endif
>          flush_tlb_mask(d->domain_dirty_cpumask);
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/memory.c
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -165,7 +165,7 @@ int guest_remove_page(struct domain *d, 
>      mfn = mfn_x(get_gfn(d, gmfn, &p2mt)); 
>      if ( unlikely(p2m_is_paging(p2mt)) )
>      {
> -        guest_physmap_remove_page(d, gmfn, mfn, 0);
> +        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>          p2m_mem_paging_drop_page(d, gmfn);
>          put_gfn(d, gmfn);
>          return 1;
> @@ -188,7 +188,7 @@ int guest_remove_page(struct domain *d, 
>      if(p2m_is_shared(p2mt))
>      {
>          put_page_and_type(page);
> -        guest_physmap_remove_page(d, gmfn, mfn, 0);
> +        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>          put_gfn(d, gmfn);
>          return 1;
>      }
> @@ -207,7 +207,7 @@ int guest_remove_page(struct domain *d, 
>      if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>          put_page(page);
>  
> -    guest_physmap_remove_page(d, gmfn, mfn, 0);
> +    guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>      put_gfn(d, gmfn);
>  
>      put_page(page);
> @@ -387,7 +387,7 @@ static long memory_exchange(XEN_GUEST_HA
>  
>                  page = mfn_to_page(mfn);
>  
> -                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
> +                if ( unlikely(steal_page(d, page, MEMF_no_refcount, 
> HOLDING_GFN)) )
>                  {
>                      put_gfn(d, gmfn + k);
>                      rc = -EINVAL;
> @@ -427,7 +427,7 @@ static long memory_exchange(XEN_GUEST_HA
>              gfn = mfn_to_gmfn(d, mfn);
>              /* Pages were unshared above */
>              BUG_ON(SHARED_M2P(gfn));
> -            guest_physmap_remove_page(d, gfn, mfn, 0);
> +            guest_physmap_remove_page(d, gfn, mfn, 0, 0);
>              put_page(page);
>          }
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/mm.h
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -578,7 +578,8 @@ int compat_arch_memory_op(int op, XEN_GU
>  int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE(void));
>  
>  int steal_page(
> -    struct domain *d, struct page_info *page, unsigned int memflags);
> +    struct domain *d, struct page_info *page, unsigned int memflags,
> +    int holding_gfn);
>  int donate_page(
>      struct domain *d, struct page_info *page, unsigned int memflags);
>  int page_make_sharable(struct domain *d, 
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -353,6 +353,10 @@ static inline unsigned long get_gfn_unty
>  /* Will release the p2m_lock and put the page behind this mapping. */
>  void put_gfn(struct domain *d, unsigned long gfn);
>  
> +/* Operations that change the underlying mfn in a p2m entry need to be 
> + * told whether the caller is holding the current gfn */
> +#define HOLDING_GFN 1
> +
>  /* The intent is to have the caller not worry about put_gfn. They apply to 
>   * very specific situations: debug printk's, dumps during a domain crash,
>   * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
> @@ -410,7 +414,8 @@ static inline int guest_physmap_add_page
>  /* Remove a page from a domain's p2m table */
>  void guest_physmap_remove_page(struct domain *d,
>                                 unsigned long gfn,
> -                               unsigned long mfn, unsigned int page_order);
> +                               unsigned long mfn, unsigned int page_order,
> +                               int holding_gfn);
>  
>  /* Set a p2m range as populate-on-demand */
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long 
> gfn,
> @@ -471,7 +476,8 @@ p2m_pod_offline_or_broken_replace(struct
>  
>  #ifdef __x86_64__
>  /* Modify p2m table for shared gfn */
> -int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> +int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
> +                            int holding_gfn);
>  
>  /* Check if a nominated gfn is valid to be paged out */
>  int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/paging.h
> --- a/xen/include/xen/paging.h
> +++ b/xen/include/xen/paging.h
> @@ -21,7 +21,7 @@
>  #define paging_mode_translate(d)              (0)
>  #define paging_mode_external(d)               (0)
>  #define guest_physmap_add_page(d, p, m, o)    (0)
> -#define guest_physmap_remove_page(d, p, m, o) ((void)0)
> +#define guest_physmap_remove_page(d, p, m, o, h)    ((void)0)
>  
>  #endif
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/tmem_xen.h
> --- a/xen/include/xen/tmem_xen.h
> +++ b/xen/include/xen/tmem_xen.h
> @@ -198,7 +198,7 @@ static inline void _tmh_free_page_thispo
>      struct domain *d = page_get_owner(pi);
>  
>      ASSERT(IS_VALID_PAGE(pi));
> -    if ( (d == NULL) || steal_page(d,pi,0) == 0 )
> +    if ( (d == NULL) || steal_page(d,pi,0,0) == 0 )
>          tmh_page_list_put(pi);
>      else
>      {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.