[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |