Mailing List Archive: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

Nov 8, 2011, 1:42 PM

Post #1 of 8 (312 views)

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.
- the lock is always taken recursively, as there are many paths that
do get_gfn -> p2m_lock

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

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@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

tim at xen

Nov 10, 2011, 4:53 AM

Post #2 of 8 (289 views)

Permalink

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?

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@lagarcavilla.org>
>
> 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@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

andres at lagarcavilla

Nov 14, 2011, 10:03 AM

Post #3 of 8 (284 views)

Permalink

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@lagarcavilla.org>
>>
>> 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@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

tim at xen

Nov 24, 2011, 3:35 AM

Post #4 of 8 (281 views)

Permalink

At 10:03 -0800 on 14 Nov (1321264988), Andres Lagar-Cavilla wrote:
> 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.

Is sh_update_cr3() really called with p2m locks/refs held? Since it
doesn't take a frame number as an argument it might be possible to
shuffle things around at the callers.

> Any suggestions here? Other than disabling ordering of the p2m lock?

Disabling the ordering won't do, so we need to find something else.

Tim.

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

andres at lagarcavilla

Nov 24, 2011, 8:41 AM

Post #5 of 8 (282 views)

Permalink

> At 10:03 -0800 on 14 Nov (1321264988), Andres Lagar-Cavilla wrote:
>> 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.
>
> Is sh_update_cr3() really called with p2m locks/refs held? Since it
> doesn't take a frame number as an argument it might be possible to
> shuffle things around at the callers.

Ok, I've refined this a bit. There are several instances of code that
takes the paging_lock and later needs to perform a p2m lookup. Apart from
the previously mentioned example here are two others:

hap_update_paging_modes -> ... -> vmx_load_pdptrs -> get_gfn(guest_cr3)

sh_x86_emulate_* -> ... -> validate_gl?e -> get_gfn_query()

None of these functions are called with p2m locks/refs held, and likely
they shouldn't. However, they will result in a deadlock panic situation,
if get_gfn takes a lock.

Here is one solution to consider: do not lock the p2m on lookups in shadow
mode. Shadow mode does not support paging out and sharing of pages, which
are primary reasons why we want synchronized p2m lookups. The hap cases
where there is an inversion of the p2m_lock -> paging_lock order are
reasonably simple to handle.

The other option is to invert the order and place paging_lock -> p2m_lock,
but that will raise another set of potential inversions. I think this is a
no go.

Finally, one could audit all these calls and have them perform futurology,
get_gfn the gfn they will end up perhaps looking up, before taking the
paging_lock. I also think this is a no go.

All very unsavory.
Andres

>
>> Any suggestions here? Other than disabling ordering of the p2m lock?
>
> Disabling the ordering won't do, so we need to find something else.
>
> Tim.
>
>

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

tim at xen

Dec 1, 2011, 5:59 AM

Post #6 of 8 (277 views)

Permalink

At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
> Here is one solution to consider: do not lock the p2m on lookups in shadow
> mode. Shadow mode does not support paging out and sharing of pages, which
> are primary reasons why we want synchronized p2m lookups. The hap cases
> where there is an inversion of the p2m_lock -> paging_lock order are
> reasonably simple to handle.
>
> The other option is to invert the order and place paging_lock -> p2m_lock,
> but that will raise another set of potential inversions. I think this is a
> no go.

Is there scope for having the p2m lock split out into the overall one
(needed for allocations &c) and the per-record ones, and have them at
different levels in the lock hierarchy?

Tim.

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

andres at lagarcavilla

Dec 2, 2011, 8:33 AM

Post #7 of 8 (280 views)

Permalink

> At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
>> Here is one solution to consider: do not lock the p2m on lookups in
>> shadow
>> mode. Shadow mode does not support paging out and sharing of pages,
>> which
>> are primary reasons why we want synchronized p2m lookups. The hap cases
>> where there is an inversion of the p2m_lock -> paging_lock order are
>> reasonably simple to handle.
>>
>> The other option is to invert the order and place paging_lock ->
>> p2m_lock,
>> but that will raise another set of potential inversions. I think this is
>> a
>> no go.
>
> Is there scope for having the p2m lock split out into the overall one
> (needed for allocations &c) and the per-record ones, and have them at
> different levels in the lock hierarchy?
I think it's saner to go with one big lock and then shard it if we run
into performance problems. Your proposal, actually :)

Now, I'm thinking of splitting ... the paging lock. It covers way too much
ground right now, and many of these inversions would become more
tractable. Thinking of paging_alloc_lock, paging_logdirty_lock,
paging_lock.

Thanks,
Andres
>
> Tim.
>

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

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications [ In reply to ]

tim at xen

Dec 2, 2011, 9:38 AM

Post #8 of 8 (282 views)

Permalink

At 08:33 -0800 on 02 Dec (1322814798), Andres Lagar-Cavilla wrote:
> > At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
> >> Here is one solution to consider: do not lock the p2m on lookups in
> >> shadow
> >> mode. Shadow mode does not support paging out and sharing of pages,
> >> which
> >> are primary reasons why we want synchronized p2m lookups. The hap cases
> >> where there is an inversion of the p2m_lock -> paging_lock order are
> >> reasonably simple to handle.
> >>
> >> The other option is to invert the order and place paging_lock ->
> >> p2m_lock,
> >> but that will raise another set of potential inversions. I think this is
> >> a
> >> no go.
> >
> > Is there scope for having the p2m lock split out into the overall one
> > (needed for allocations &c) and the per-record ones, and have them at
> > different levels in the lock hierarchy?
> I think it's saner to go with one big lock and then shard it if we run
> into performance problems. Your proposal, actually :)
>
> Now, I'm thinking of splitting ... the paging lock. It covers way too much
> ground right now, and many of these inversions would become more
> tractable. Thinking of paging_alloc_lock, paging_logdirty_lock,
> paging_lock.

...almost exactly six months after I merged the shadow, hap and
log-dirty locks into one lock, to avoid the deadlocks that were caused
by those sections of code calling in to each other. :)

http://xenbits.xen.org/hg/staging/xen-unstable.hg/log?rev=2bbed

Tim.

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