[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications
Hi there, > At 16:42 -0500 on 08 Nov (1320770545), Andres Lagar-Cavilla wrote: >> xen/arch/x86/mm/mm-locks.h | 13 +++++++------ >> xen/arch/x86/mm/p2m.c | 18 +++++++++++++++++- >> xen/include/asm-x86/p2m.h | 39 >> ++++++++++++++++++++++++--------------- >> 3 files changed, 48 insertions(+), 22 deletions(-) >> >> >> We achieve this by locking/unlocking the global p2m_lock in get/put_gfn. >> This brings about a few consequences for the p2m_lock: >> >> - not ordered anymore in mm-locks.h: unshare does get_gfn -> shr_lock, >> there are code paths that do paging_lock -> get_gfn. All of these >> would cause mm-locks.h to panic. > > In that case there's a potential deadlock in the sharing code, and > turning off the safety catches is not an acceptable response to that. :) > > ISTR you had a plan to get rid of the shr_lock entirely, or am > I misremembering? Sharing is actually fine, I can reorder those safely until I get rid of the shr_lock. Except for sharing audits, which basically lock the whole hypervisor, and _no one is using at all_. I have a more fundamental problem with the paging lock. sh_update_cr3 can be called from a variety of situations. It will walk the four top level PAE mappings, acquiring the p2m entry for each, with the paging lock held. This is an inversion & deadlock, if I try to synchronize p2m lookups with the p2m lock. Any suggestions here? Other than disabling ordering of the p2m lock? Thanks Andres > > Tim. > >> - the lock is always taken recursively, as there are many paths that >> do get_gfn -> p2m_lock >> >> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx> >> >> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/mm-locks.h >> --- a/xen/arch/x86/mm/mm-locks.h >> +++ b/xen/arch/x86/mm/mm-locks.h >> @@ -165,14 +165,15 @@ declare_mm_lock(nestedp2m) >> >> /* P2M lock (per-p2m-table) >> * >> - * This protects all updates to the p2m table. Updates are expected to >> - * be safe against concurrent reads, which do *not* require the lock. >> */ >> + * This protects all queries and updates to the p2m table. Because >> queries >> + * can happen interleaved with other locks in here, we do not enforce >> + * ordering, and make all locking recursive. */ >> >> -declare_mm_lock(p2m) >> -#define p2m_lock(p) mm_lock(p2m, &(p)->lock) >> -#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock) >> -#define p2m_unlock(p) mm_unlock(&(p)->lock) >> +#define p2m_lock(p) spin_lock_recursive(&(p)->lock.lock) >> +#define p2m_lock_recursive(p) spin_lock_recursive(&(p)->lock.lock) >> +#define p2m_unlock(p) spin_unlock_recursive(&(p)->lock.lock) >> #define p2m_locked_by_me(p) mm_locked_by_me(&(p)->lock) >> +#define gfn_locked_by_me(p,g) mm_locked_by_me(&(p)->lock) >> >> /* Page alloc lock (per-domain) >> * >> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/p2m.c >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -158,6 +158,9 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom >> return _mfn(gfn); >> } >> >> + /* Grab the lock here, don't release until put_gfn */ >> + p2m_lock(p2m); >> + >> mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order); >> >> #ifdef __x86_64__ >> @@ -182,6 +185,19 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom >> return mfn; >> } >> >> +void put_gfn(struct domain *d, unsigned long gfn) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + >> + if ( !p2m || !paging_mode_translate(d) ) >> + /* Nothing to do in this case */ >> + return; >> + >> + ASSERT(p2m_locked_by_me(p2m)); >> + >> + p2m_unlock(p2m); >> +} >> + >> int set_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >> unsigned int page_order, p2m_type_t p2mt, >> p2m_access_t p2ma) >> { >> @@ -190,7 +206,7 @@ int set_p2m_entry(struct p2m_domain *p2m >> unsigned int order; >> int rc = 1; >> >> - ASSERT(p2m_locked_by_me(p2m)); >> + ASSERT(gfn_locked_by_me(p2m, gfn)); >> >> while ( todo ) >> { >> diff -r a0c55cc5d696 -r 6203a0549d8a xen/include/asm-x86/p2m.h >> --- a/xen/include/asm-x86/p2m.h >> +++ b/xen/include/asm-x86/p2m.h >> @@ -305,9 +305,10 @@ struct p2m_domain *p2m_get_p2m(struct vc >> >> #define p2m_get_pagetable(p2m) ((p2m)->phys_table) >> >> -/**** p2m query accessors. After calling any of the variants below, you >> - * need to call put_gfn(domain, gfn). If you don't, you'll lock the >> - * hypervisor. ****/ >> +/**** p2m query accessors. They lock p2m_lock, and thus serialize >> + * lookups wrt modifications. They _do not_ release the lock on exit. >> + * After calling any of the variants below, caller needs to use >> + * put_gfn. ****/ >> >> /* Read a particular P2M table, mapping pages as we go. Most callers >> * should _not_ call this directly; use the other get_gfn* functions >> @@ -349,19 +350,27 @@ static inline unsigned long get_gfn_unty >> return INVALID_MFN; >> } >> >> -/* This is a noop for now. */ >> -static inline void put_gfn(struct domain *d, unsigned long gfn) >> -{ >> -} >> +/* Will release the p2m_lock and put the page behind this mapping. */ >> +void put_gfn(struct domain *d, unsigned long gfn); >> >> -/* These are identical for now. The intent is to have the caller not >> worry >> - * about put_gfn. To only be used in printk's, crash situations, or to >> - * peek at a type. You're not holding the p2m entry exclsively after >> calling >> - * this. */ >> -#define get_gfn_unlocked(d, g, t) get_gfn_type((d), (g), (t), >> p2m_alloc) >> -#define get_gfn_query_unlocked(d, g, t) get_gfn_type((d), (g), (t), >> p2m_query) >> -#define get_gfn_guest_unlocked(d, g, t) get_gfn_type((d), (g), (t), >> p2m_guest) >> -#define get_gfn_unshare_unlocked(d, g, t) get_gfn_type((d), (g), (t), >> p2m_unshare) >> +/* 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 >> + * exclusively after calling this. */ >> +#define build_unlocked_accessor(name) >> \ >> + static inline mfn_t get_gfn ## name ## _unlocked(struct domain *d, >> \ >> + unsigned long gfn, >> \ >> + p2m_type_t *t) >> \ >> + { >> \ >> + mfn_t mfn = get_gfn ##name (d, gfn, t); >> \ >> + put_gfn(d, gfn); >> \ >> + return mfn; >> \ >> + } >> + >> +build_unlocked_accessor() >> +build_unlocked_accessor(_query) >> +build_unlocked_accessor(_guest) >> +build_unlocked_accessor(_unshare) >> >> /* General conversion function from mfn to gfn */ >> static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) >> >> _______________________________________________ >> 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 |