Synchronized p2m lookups
PATCH 1/1 Synchronized p2m lookups
Up until now, p2m lookups (get_gfn*) in the x86 architecture were not synchronized against concurrent updates. With the addition of sharing and paging subsystems that can dynamically modify the p2m, the need for synchronized lookups becomes more pressing.
Without synchronized lookups, we've encountered host crashes triggered by race conditions, particularly when exercising the sharing subsystem.
In this patch series we make p2m lookups fully synchronized. We still use a single per-domain p2m lock (profiling work tbd may or may not indicate the need for fine-grained locking).
We only enforce locking of p2m queries for hap-assisted domains. These are the domains (in the case of EPT) that can exercise the paging and sharing subsystems and thus most in need of synchronized lookups.
While the code *should* work for domains relying on shadow paging, it has not been tested, hence we do not enable this at the moment.
Finally, the locking discipline in the PoD subsystem has been updated, since PoD relied on some assumptions about the way the p2m lock is used.
xen/arch/x86/mm/hap/hap.c | 6 ++ xen/arch/x86/mm/mm-locks.h | 40 ++++++++----- xen/arch/x86/mm/p2m.c | 21 ++++++- xen/include/asm-x86/p2m.h | 47 ++++++++++------ xen/arch/x86/mm/mem_sharing.c | 2 - xen/arch/x86/mm/mm-locks.h | 4 +- xen/arch/x86/mm/p2m-ept.c | 33 +---------- xen/arch/x86/mm/p2m-pod.c | 14 +++- xen/arch/x86/mm/p2m-pt.c | 45 ++------------ xen/arch/x86/mm/p2m.c | 64 +++++++++++---------- xen/arch/x86/mm/mm-locks.h | 10 +++ xen/arch/x86/mm/p2m-pod.c | 112 +++++++++++++++++++++++--------------- xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 8 ++- xen/include/asm-x86/p2m.h | 27 ++------ xen/arch/x86/hvm/emulate.c | 2 +- xen/arch/x86/hvm/hvm.c | 25 +++++-- xen/arch/x86/mm.c | 10 +- xen/arch/x86/mm/guest_walk.c | 2 +- xen/arch/x86/mm/hap/guest_walk.c | 6 +- xen/arch/x86/mm/mem_access.c | 10 +++ xen/arch/x86/mm/mem_event.c | 5 + xen/arch/x86/mm/p2m.c | 52 ++++++++++-------- xen/common/grant_table.c | 2 +- xen/common/memory.c | 2 +- xen/include/asm-x86/mem_access.h | 1 + xen/include/asm-x86/mem_event.h | 3 + xen/include/asm-x86/p2m.h | 6 +- xen/arch/x86/mm/p2m.c | 4 +- 29 files changed, 312 insertions(+), 252 deletions(-)
PATCH 1/1 Make p2m lookups fully synchronized wrt modifications
xen/arch/x86/mm/hap/hap.c | 6 +++++ xen/arch/x86/mm/mm-locks.h | 40 ++++++++++++++++++++++++-------------- xen/arch/x86/mm/p2m.c | 21 ++++++++++++++++++- xen/include/asm-x86/p2m.h | 47 ++++++++++++++++++++++++++++----------------- 4 files changed, 79 insertions(+), 35 deletions(-)
We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.
The lock is always taken recursively, as there are many paths that do get_gfn, and later, another attempt at grabbing the p2m_lock
The lock is not taken for shadow lookups, as the testing needed to rule out the possibility of locking inversions and deadlock has not yet been carried out for shadow-paging. This is tolerable as long as shadows do not gain support for paging or sharing.
HAP (EPT) lookups and all modifications do take the lock.
xen/arch/x86/mm/hap/hap.c +6 -0
@@ -786,7 +786,12 @@ hap_paging_get_mode(struct vcpu *v)static void hap_update_paging_modes (struct vcpu *v) {struct domain *d = v->domain; +unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3 ]; +p2m_type_t t; +/* We hold onto the cr3 as it may be modified later, and +* we need to respect lock ordering */ + (void )get_gfn (d, cr3_gfn, &t);paging_lock (d); v->arch.paging.mode =hap_paging_get_mode (v);
@@ -803,6 +808,7 @@ static void hap_update_paging_modes(struhap_update_cr3 (v,0 );paging_unlock (d); +put_gfn (d, cr3_gfn); }#if CONFIG_PAGING_LEVELS ==3
xen/arch/x86/mm/mm-locks.h +25 -15
@@ -144,6 +144,31 @@ static inline void mm_enforce_order_unlo* * ************************************************************************/ +declare_mm_lock(nestedp2m) +#define nestedp2m_lock (d) mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock) +#define nestedp2m_unlock (d) mm_unlock(&(d)->arch.nested_p2m_lock) + +/* P2M lock (per-p2m-table) +* +* This protects all queries and updates to the p2m table. +* +* A note about ordering: +* The order established here is enforced on all mutations of a p2m. +* For lookups, the order established here is enforced only for hap +* domains (1. shadow domains present a few nasty inversions; +* 2. shadow domains do not support paging and sharing, +* the main sources of dynamic p2m mutations) +* +* The lock is recursive as it is common for a code path to look up a gfn +* and later mutate it. +*/ + +declare_mm_lock(p2m) +#define p2m_lock (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_lock_recursive (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_unlock (p) mm_unlock(&(p)->lock) +#define p2m_locked_by_me (p) mm_locked_by_me(&(p)->lock) +/* Sharing per page lock * * This is an external lock, not represented by an mm_lock_t. The memory
@@ -167,21 +192,6 @@ declare_mm_order_constraint(per_page_sha* - setting the "cr3" field of any p2m table to a non-CR3_EADDR value. * (i.e. assigning a p2m table to be the shadow of that cr3 */ -declare_mm_lock(nestedp2m) -#define nestedp2m_lock (d) mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock) -#define nestedp2m_unlock (d) mm_unlock(&(d)->arch.nested_p2m_lock) - -/* 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. */ - -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_locked_by_me (p) mm_locked_by_me(&(p)->lock) -/* Page alloc lock (per-domain) * * This is an external lock, not represented by an mm_lock_t. However,
xen/arch/x86/mm/p2m.c +19 -2
@@ -144,9 +144,9 @@ void p2m_change_entry_type_global(structp2m_unlock (p2m); } -mfn_t get_gfn_type_access (struct p2m_domain *p2m,unsigned long gfn, +mfn_t __get_gfn_type_access (struct p2m_domain *p2m,unsigned long gfn,p2m_type_t *t,p2m_access_t *a,p2m_query_t q, -unsigned int *page_order) +unsigned int *page_order,bool_t locked) {mfn_t mfn;
@@ -158,6 +158,11 @@ mfn_t get_gfn_type_access(struct p2m_domreturn _mfn (gfn); } +/* For now only perform locking on hap domains */ +if ( locked && (hap_enabled (p2m->domain)) ) +/* 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 +187,18 @@ mfn_t get_gfn_type_access(struct p2m_domreturn mfn; } +void __put_gfn (struct p2m_domain *p2m,unsigned long gfn) +{ +if ( !p2m || !paging_mode_translate (p2m->domain) + || !hap_enabled (p2m->domain) ) +/* 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) {
xen/include/asm-x86/p2m.h +29 -18
68 lines not on this page · raw message
Re: [PATCH 1 of 5] Make p2m lookups fully synchronized wrt modifications
Quoted 9 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/hap/hap.c | 6 +++++ xen/arch/x86/mm/mm-locks.h | 40 ++++++++++++++++++++++++-------------- xen/arch/x86/mm/p2m.c | 21 ++++++++++++++++++- xen/include/asm-x86/p2m.h | 47 ++++++++++++++++++++++++++++----------------- 4 files changed, 79 insertions(+), 35 deletions(-)We achieve this by locking/unlocking the global
p2m_lockin get/put_gfn.
Quoted 7 lines
The lock is always taken recursively, as there are many paths that do
get_gfn, and later, another attempt at grabbing thep2m_lockThe lock is not taken for shadow lookups, as the testing needed to rule out the possibility of locking inversions and deadlock has not yet been carried out for shadow-paging. This is tolerable as long as shadows do not gain support for paging or sharing.
Are you aware of any inversions or are you just suggesting there might be some left?
Quoted 8 lines
HAP (EPT) lookups and all modifications do take the lock.
Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
9a55109e4d7e-r223512f9fb5bxen/arch/x86/mm/hap/hap.c
xen/arch/x86/mm/hap/hap.c hap_paging_get_mode(struct vcpu *v) · line 794
Quoted 9 lines
xen/arch/x86/mm/hap/hap.c +5
-0@@ -786,7 +786,12 @@ hap_paging_get_mode(struct vcpu *v)static void hap_update_paging_modes (struct vcpu *v) {struct domain *d = v->domain; +unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3 ]; +p2m_type_t t; +/* We hold onto the cr3 as it may be modified later, and +* we need to respect lock ordering */ + (void )get_gfn (d, cr3_gfn, &t);
Don't you need to check for errors?
Quoted 36 lines
xen/arch/x86/mm/hap/hap.c +1
-0paging_lock (d); v->arch.paging.mode =hap_paging_get_mode (v);@@ -803,6 +808,7 @@ static void hap_update_paging_modes(struhap_update_cr3 (v,0 );paging_unlock (d); +put_gfn (d, cr3_gfn); }#if CONFIG_PAGING_LEVELS ==3 diff -r
9a55109e4d7e-r223512f9fb5bxen/arch/x86/mm/mm-locks.hxen/arch/x86/mm/mm-locks.h +18
-0@@ -144,6 +144,31 @@ static inline void mm_enforce_order_unlo* * ************************************************************************/ +declare_mm_lock(nestedp2m) +#define nestedp2m_lock (d) mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock) +#define nestedp2m_unlock (d) mm_unlock(&(d)->arch.nested_p2m_lock) + +/* P2M lock (per-p2m-table) +* +* This protects all queries and updates to the p2m table. +* +* A note about ordering: +* The order established here is enforced on all mutations of a p2m. +* For lookups, the order established here is enforced only for hap +* domains (1. shadow domains present a few nasty inversions; +* 2. shadow domains do not support paging and sharing, +* the main sources of dynamic p2m mutations) +* +* The lock is recursive as it is common for a code path to look up a gfn +* and later mutate it. +*/
xen/arch/x86/mm/mm-locks.h static inline void mm_enforce_order_unlo · line 174
Quoted 10 lines
xen/arch/x86/mm/mm-locks.h +7
-0+ +declare_mm_lock(p2m) +#define p2m_lock (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_lock_recursive (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_unlock (p) mm_unlock(&(p)->lock) +#define p2m_locked_by_me (p) mm_locked_by_me(&(p)->lock) +/* Sharing per page lock * * This is an external lock, not represented by an mm_lock_t. The memory
Since you've just reversed the locking order between this page-sharing lock and the p2m lock, you need to update the comment that describes it. Also, presumably, the code that it describes?
Cheers,
Tim.
Re: [PATCH 1 of 5] Make p2m lookups fully synchronized wrt modifications
Quoted 22 lines
Quoted 13 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/hap/hap.c | 6 +++++ xen/arch/x86/mm/mm-locks.h | 40 ++++++++++++++++++++++++-------------- xen/arch/x86/mm/p2m.c | 21 ++++++++++++++++++- xen/include/asm-x86/p2m.h | 47 ++++++++++++++++++++++++++++----------------- 4 files changed, 79 insertions(+), 35 deletions(-)We achieve this by locking/unlocking the global
p2m_lockin get/put_gfn.The lock is always taken recursively, as there are many paths that do
get_gfn, and later, another attempt at grabbing thep2m_lockQuoted 7 lines
The lock is not taken for shadow lookups, as the testing needed to rule out the
possibility of locking inversions and deadlock has not yet been carried out for
shadow-paging. This is tolerable as long as shadows do not gain support for
paging or sharing.
Quoted 2 lines
Are you aware of any inversions or are you just suggesting there might be some left?
I'm not aware of any inversions. A previously posted patch cleans up all that I could see. But I have not tested shadows, so I don't feel comfortable about enabling it just yet.
Quoted 15 lines
Quoted 15 lines
HAP (EPT) lookups and all modifications do take the lock.
Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
9a55109e4d7e-r223512f9fb5bxen/arch/x86/mm/hap/hap.cxen/arch/x86/mm/hap/hap.c +2
-0@@ -786,7 +786,12 @@ hap_paging_get_mode(struct vcpu *v)static void hap_update_paging_modes (struct vcpu *v) {struct domain *d = v->domain; +unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3 ]; +p2m_type_t t;
Quoted 5 lines
xen/arch/x86/mm/hap/hap.c hap_paging_get_mode(struct vcpu *v) · line 794
Quoted 3 lines
xen/arch/x86/mm/hap/hap.c +3
-0+/* We hold onto the cr3 as it may be modified later, and +* we need to respect lock ordering */ + (void )get_gfn (d, cr3_gfn, &t);Don't you need to check for errors?
Good to have, yes. As in, bail out if !p2m_is_ram || !mfn_valid.
Quoted 47 lines
Quoted 47 lines
xen/arch/x86/mm/hap/hap.c +1
-0paging_lock (d); v->arch.paging.mode =hap_paging_get_mode (v);@@ -803,6 +808,7 @@ static void hap_update_paging_modes(struhap_update_cr3 (v,0 );paging_unlock (d); +put_gfn (d, cr3_gfn); }#if CONFIG_PAGING_LEVELS ==3 diff -r
9a55109e4d7e-r223512f9fb5bxen/arch/x86/mm/mm-locks.hxen/arch/x86/mm/mm-locks.h +16
-0@@ -144,6 +144,31 @@ static inline void mm_enforce_order_unlo* * ************************************************************************/ +declare_mm_lock(nestedp2m) +#define nestedp2m_lock (d) mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock) +#define nestedp2m_unlock (d) mm_unlock(&(d)->arch.nested_p2m_lock) + +/* P2M lock (per-p2m-table) +* +* This protects all queries and updates to the p2m table. +* +* A note about ordering: +* The order established here is enforced on all mutations of a p2m. +* For lookups, the order established here is enforced only for hap +* domains (1. shadow domains present a few nasty inversions; +* 2. shadow domains do not support paging and sharing, +* the main sources of dynamic p2m mutations) +* +* The lock is recursive as it is common for a code path to look up a gfn
+ * and later mutate it.
+ */xen/arch/x86/mm/mm-locks.h +7
-0+ +declare_mm_lock(p2m) +#define p2m_lock (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_lock_recursive (p) mm_lock_recursive(p2m, &(p)->lock) +#define p2m_unlock (p) mm_unlock(&(p)->lock) +#define p2m_locked_by_me (p) mm_locked_by_me(&(p)->lock) +
Quoted 8 lines
Quoted 4 lines
/* Sharing per page lock
*
* This is an external lock, not represented by anmm_lock_t. The
memorySince you've just reversed the locking order between this page-sharing lock and the p2m lock, you need to update the comment that describes it. Also, presumably, the code that it describes?
The comment might be stale, certainly, will look into it.
The code is just fine, per testing. The next patch does a proper cleanup:
the "complicated condition" is grabbing a p2m lock nested inside a page-sharing critical section. But thanks to this patch, the p2m lock will have been taken before, so no race (hope that makes sense).
Bottomline is, correct with this patch, clean with next.
Thanks!
Andres
Quoted 5 lines
Cheers,
Tim.
PATCH 1/1 Clean up locking now that p2m lockups are fully synchronized
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/mem_sharing.c | 2 - xen/arch/x86/mm/mm-locks.h | 4 ++- xen/arch/x86/mm/p2m-ept.c | 33 ++------------------- xen/arch/x86/mm/p2m-pod.c | 14 ++++++--- xen/arch/x86/mm/p2m-pt.c | 45 +++++------------------------ xen/arch/x86/mm/p2m.c | 64 ++++++++++++++++++++++-------------------- 6 files changed, 58 insertions(+), 104 deletions(-)
With p2m lookups fully synchronized, many routines need not call p2m_lock any longer. Also, many routines can logically assert holding the p2m for a specific gfn.
xen/arch/x86/mm/mem_sharing.c +0 -2
@@ -882,9 +882,7 @@ int mem_sharing_add_to_physmap(struct do goto err_unlock; } -p2m_lock (p2m); ret =set_p2m_entry (p2m, cgfn, smfn,PAGE_ORDER_4K , p2m_ram_shared, a); -p2m_unlock (p2m);/* Tempted to turn this into an assert */ if ( !ret )
xen/arch/x86/mm/mm-locks.h +3 -1
@@ -165,9 +165,11 @@ declare_mm_lock(nestedp2m) declare_mm_lock(p2m)#define p2m_lock (p) mm_lock_recursive(p2m, &(p)->lock) -#define p2m_lock_recursive (p) mm_lock_recursive(p2m, &(p)->lock) +#define gfn_lock (p,g,o) mm_lock_recursive(p2m, &(p)->lock)#define p2m_unlock (p) mm_unlock(&(p)->lock) +#define gfn_unlock (p,g,o) mm_unlock(&(p)->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)/* Sharing per page lock *
xen/arch/x86/mm/p2m-ept.c +4 -29
@@ -46,31 +46,6 @@ static inline bool_t is_epte_valid(ept_ereturn (e->epte !=0 && e->sa_p2mt != p2m_invalid); } -/* Non-ept "lock-and-check" wrapper */ -static int ept_pod_check_and_populate (struct p2m_domain *p2m,unsigned long gfn, -ept_entry_t *entry,int order, -p2m_query_t q) -{ -int r; - -/* This is called from the p2m lookups, which can happen with or -* without the lock hed. */ -p2m_lock_recursive (p2m); - -/* Check to make sure this is still PoD */ -if ( entry->sa_p2mt != p2m_populate_on_demand ) - { -p2m_unlock (p2m); -return 0 ; - } - - r =p2m_pod_demand_populate (p2m, gfn, order, q); - -p2m_unlock (p2m); - -return r; -} -static void ept_p2m_type_to_flags (ept_entry_t *entry,p2m_type_t type,p2m_access_t access) {/* First apply type permissions */
@@ -551,8 +526,8 @@ static mfn_t ept_get_entry(struct p2m_do index = gfn_remainder >> ( i *EPT_TABLE_ORDER ); ept_entry = table + index; -if ( !ept_pod_check_and_populate (p2m, gfn, - ept_entry,9 , q) ) +if ( !p2m_pod_demand_populate (p2m, gfn, +PAGE_ORDER_2M , q) ) goto retry;else goto out;
@@ -574,8 +549,8 @@ static mfn_t ept_get_entry(struct p2m_doASSERT (i ==0 ); -if (ept_pod_check_and_populate (p2m, gfn, - ept_entry,0 , q) ) +if (p2m_pod_demand_populate (p2m, gfn, +PAGE_ORDER_4K , q) ) goto out; }
xen/arch/x86/mm/p2m-pod.c +9 -5
@@ -521,7 +521,7 @@ p2m_pod_decrease_reservation(struct doma/* Figure out if we need to steal some freed memory for our cache */ steal_for_cache = ( p2m->pod.entry_count > p2m->pod.count ); -p2m_lock (p2m); +gfn_lock (p2m, gpfn, order);if (unlikely (d->is_dying) ) goto out_unlock;
@@ -615,7 +615,7 @@ out_entry_check: } out_unlock: -p2m_unlock (p2m); +gfn_unlock (p2m, gpfn, order); out:return ret;
@@ -964,7 +964,7 @@ p2m_pod_demand_populate(struct p2m_domaimfn_t mfn;int i; -ASSERT (p2m_locked_by_me (p2m)); +ASSERT (gfn_locked_by_me (p2m, gfn));/* This check is done with the p2m lock held. This will make sure that * even if d->is_dying changes under our feet, p2m_pod_empty_cache()
@@ -972,6 +972,7 @@ p2m_pod_demand_populate(struct p2m_domaiif (unlikely (d->is_dying) ) goto out_fail; +/* Because PoD does not have cache list for 1GB pages, it has to remap * 1GB region to 2MB chunks for a retry. */ if ( order ==PAGE_ORDER_1G )
@@ -981,6 +982,9 @@ p2m_pod_demand_populate(struct p2m_domai* split 1GB into 512 2MB pages here. But We only do once here because * set_p2m_entry() should automatically shatter the 1GB page into * 512 2MB pages. The rest of 511 calls are unnecessary. +* +* NOTE: In a fine-grained p2m locking scenario this operation +* may need to promote its locking from gfn->1g superpage */ set_p2m_entry (p2m, gfn_aligned,_mfn (0 ),PAGE_ORDER_2M , p2m_populate_on_demand, p2m->default_access);
@@ -1104,7 +1108,7 @@ guest_physmap_mark_populate_on_demand(stif ( rc !=0 )return rc; -p2m_lock (p2m); +gfn_lock (p2m, gfn, order);P2M_DEBUG ("mark pod gfn=%#lx\n" , gfn);
@@ -1138,7 +1142,7 @@ guest_physmap_mark_populate_on_demand(stBUG_ON (p2m->pod.entry_count <0 ); } -p2m_unlock (p2m); +gfn_unlock (p2m, gfn, order); out:return rc;
xen/arch/x86/mm/p2m-pt.c +8 -37
87 lines not on this page · raw message
xen/arch/x86/mm/p2m.c +34 -30
240 lines not on this page · raw message
Re: [PATCH 2 of 5] Clean up locking now that p2m lockups are fully synchronized
Quoted 9 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/mem_sharing.c | 2 - xen/arch/x86/mm/mm-locks.h | 4 ++- xen/arch/x86/mm/p2m-ept.c | 33 ++------------------- xen/arch/x86/mm/p2m-pod.c | 14 ++++++--- xen/arch/x86/mm/p2m-pt.c | 45 +++++------------------------ xen/arch/x86/mm/p2m.c | 64 ++++++++++++++++++++++-------------------- 6 files changed, 58 insertions(+), 104 deletions(-)
Quoted 5 lines
With p2m lookups fully synchronized, many routines need not call
p2m_lockany longer. Also, many routines can logically assert holding the p2m for a specific gfn.Signed-off-by Andres Lagar-Cavilla <[email protected]>
This looks OK to me, but I'd like an Ack from George as well for the PoD changes.
Cheers,
Tim.
Re: [PATCH 2 of 5] Clean up locking now that p2m lockups are fully synchronized
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/p2m.c p2m_get_nestedp2m(struct vcpu *v, uint64 · line 1453
Quoted 7 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/p2m.c +1
-1@@ -1445,7 +1449,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64nestedp2m_unlock (d);return p2m; } -p2m_unlock (p2m); +gfn_unlock (p2m, gfn,0 ); }
It looks like maybe you forgot to change the corresponding p2m_lock() to gfn_lock(), here in p2m.c:p2m_get_nestedp2m()?
Other than that, I think the PoD stuff looks fine. So regarding PoD stuff:
Re: [PATCH 2 of 5] Clean up locking now that p2m lockups are fully synchronized
Quoted 11 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/p2m.c p2m_get_nestedp2m(struct vcpu *v, uint64 · line 1453
Quoted 7 lines
xen/arch/x86/mm/p2m.c +1
-1@@ -1445,7 +1449,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64nestedp2m_unlock (d);return p2m; } -p2m_unlock (p2m); +gfn_unlock (p2m, gfn,0 ); }It looks like maybe you forgot to change the corresponding
p2m_lock()togfn_lock(), here in p2m.c:p2m_get_nestedp2m()?
Good catch! (evil macro argument squashing!)
Thanks
Andres
Quoted 6 lines
Other than that, I think the PoD stuff looks fine. So regarding PoD stuff:
Acked-by George Dunlap <[email protected]>
PATCH 1/1 Rework locking in the PoD layer
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/mm-locks.h | 10 ++++ xen/arch/x86/mm/p2m-pod.c | 112 ++++++++++++++++++++++++++------------------ xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 8 ++- xen/include/asm-x86/p2m.h | 27 +++------- 5 files changed, 93 insertions(+), 65 deletions(-)
The PoD layer has a comples locking discipline. It relies on the p2m being globally locked, and it also relies on the page alloc lock to protect some of its data structures. Replace this all by an explicit pod lock: per p2m, order enforced.
Three consequences:
- Critical sections in the pod code protected by the page alloc lock are now reduced to modifications of the domain page list.
- When the p2m lock becomes fine-grained, there are no assumptions broken in the PoD layer.
- The locking is easier to understand.
xen/arch/x86/mm/mm-locks.h +10 -0
@@ -194,6 +194,16 @@ declare_mm_order_constraint(per_page_sha* - setting the "cr3" field of any p2m table to a non-CR3_EADDR value. * (i.e. assigning a p2m table to be the shadow of that cr3 */ +/* PoD lock (per-p2m-table) +* +* Protects private PoD data structs: entry and cache +* counts, page lists, sweep parameters. */ + +declare_mm_lock(pod) +#define pod_lock (p) mm_lock(pod, &(p)->pod.lock) +#define pod_unlock (p) mm_unlock(&(p)->pod.lock) +#define pod_locked_by_me (p) mm_locked_by_me(&(p)->pod.lock) +/* Page alloc lock (per-domain) * * This is an external lock, not represented by an mm_lock_t. However,
xen/arch/x86/mm/p2m-pod.c +67 -45
374 lines not on this page · raw message
xen/arch/x86/mm/p2m-pt.c +1 -0
@@ -954,6 +954,7 @@ long p2m_pt_audit_p2m(struct p2m_domainstruct domain *d = p2m->domain;ASSERT (p2m_locked_by_me (p2m)); +ASSERT (pod_locked_by_me (p2m)); test_linear = ( (d == current->domain) && !pagetable_is_null (current->arch.monitor_table) );
xen/arch/x86/mm/p2m.c +7 -1
@@ -72,6 +72,7 @@ boolean_param("hap_2mb", opt_hap_2mb);static void p2m_initialise (struct domain *d,struct p2m_domain *p2m) {mm_lock_init (&p2m->lock); +mm_lock_init (&p2m->pod.lock);INIT_LIST_HEAD (&p2m->np2m_list);INIT_PAGE_LIST_HEAD (&p2m->pages);INIT_PAGE_LIST_HEAD (&p2m->pod.super);
@@ -587,8 +588,10 @@ guest_physmap_add_entry(struct domain *d rc = -EINVAL ; else { - p2m->pod.entry_count -= pod_count;/* Lock: p2m */ +pod_lock (p2m); + p2m->pod.entry_count -= pod_count;BUG_ON (p2m->pod.entry_count <0 ); +pod_unlock (p2m); } }
@@ -1372,6 +1375,7 @@ p2m_flush_table(struct p2m_domain *p2m)/* "Host" p2m tables can have shared entries &c that need a bit more * care when discarding them */ ASSERT (p2m_is_nestedp2m (p2m)); +/* Nested p2m's do not do pod, hence the asserts (and no pod lock)*/ ASSERT (page_list_empty (&p2m->pod.super));ASSERT (page_list_empty (&p2m->pod.single));
@@ -1529,6 +1533,7 @@ void audit_p2m(struct domain *d,P2M_PRINTK ("p2m audit starts\n" );p2m_lock (p2m); +pod_lock (p2m);if (p2m->audit_p2m) pmbad = p2m->audit_p2m (p2m);
@@ -1589,6 +1594,7 @@ void audit_p2m(struct domain *d, }spin_unlock (&d->page_alloc_lock); +pod_unlock (p2m);p2m_unlock (p2m);P2M_PRINTK ("p2m audit complete\n" );
xen/include/asm-x86/p2m.h +8 -19
@@ -261,25 +261,12 @@ struct p2m_domain {unsigned long max_mapped_pfn;/* Populate-on-demand variables -* NB on locking. {super,single,count} are -* covered by d->page_alloc_lock, since they're almost always used in -* conjunction with that functionality. {entry_count} is covered by -* the domain p2m lock, since it's almost always used in conjunction -* with changing the p2m tables. -* -* At this point, both locks are held in two places. In both, -* the order is [p2m,page_alloc]: -* + p2m_pod_decrease_reservation() calls p2m_pod_cache_add(), -* which grabs page_alloc -* + p2m_pod_demand_populate() grabs both; the p2m lock to avoid -* double-demand-populating of pages, the page_alloc lock to -* protect moving stuff from the PoD cache to the domain page list. -* -* We enforce this lock ordering through a construct in mm-locks.h. -* This demands, however, that we store the previous lock-ordering -* level in effect before grabbing the page_alloc lock. The unlock -* level is stored in the arch section of the domain struct. -*/ +* All variables are protected with the pod lock. We cannot rely on +* the p2m lock if it's turned into a fine-grained lock. +* We only use the domain page_alloc lock for additions and +* deletions to the domain's page list. Because we use it nested +* within the PoD lock, we enforce it's ordering (by remembering +* the unlock level in the arch_domain sub struct). */ struct {struct page_list_head super,/* List of superpages */ single;/* Non-super lists */
@@ -288,6 +275,8 @@ struct p2m_domain {unsigned reclaim_super;/* Last gpfn of a scan */ unsigned reclaim_single;/* Last gpfn of a scan */ unsigned max_guest;/* gpfn of max guest demand-populate */ +mm_lock_t lock;/* Locking of private pod structs, * +* not relying on the p2m lock. */ } pod; };
Re: [PATCH 3 of 5] Rework locking in the PoD layer
Quoted 13 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/mm-locks.h | 10 ++++ xen/arch/x86/mm/p2m-pod.c | 112 ++++++++++++++++++++++++++------------------ xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 8 ++- xen/include/asm-x86/p2m.h | 27 +++------- 5 files changed, 93 insertions(+), 65 deletions(-)The PoD layer has a comples locking discipline. It relies on the p2m being globally locked, and it also relies on the page alloc lock to protect some of its data structures. Replace this all by an explicit pod lock: per p2m, order enforced.
Quoted 8 lines
Three consequences:
- Critical sections in the pod code protected by the page alloc lock are now reduced to modifications of the domain page list.
- When the p2m lock becomes fine-grained, there are no assumptions broken in the PoD layer.
- The locking is easier to understand.Signed-off-by Andres Lagar-Cavilla <[email protected]>
This needs an Ack from George, too. Also:
xen/arch/x86/mm/p2m-pod.c p2m_pod_emergency_sweep(struct p2m_domai · line 937
Quoted 10 lines
xen/arch/x86/mm/p2m-pod.c +6
-0@@ -922,6 +929,12 @@ p2m_pod_emergency_sweep(struct p2m_domai limit = (start >POD_SWEEP_LIMIT ) ? (start -POD_SWEEP_LIMIT ) :0 ;/* FIXME: Figure out how to avoid superpages */ +/* NOTE: Promote to globally locking the p2m. This will get complicated +* in a fine-grained scenario. Even if we're to lock each gfn +* individually we must be careful about recursion limits and +* POD_SWEEP_STRIDE. This is why we don't enforce deadlock constraints +* between p2m and pod locks */ +p2m_lock (p2m);
That's a scary comment. It looks to me as if the mm-locks.h mechanism _does_ enforce those constraints - am I missing something?
Cheers,
Tim.
Re: [PATCH 3 of 5] Rework locking in the PoD layer
Quoted 29 lines
Quoted 14 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/mm-locks.h | 10 ++++ xen/arch/x86/mm/p2m-pod.c | 112 ++++++++++++++++++++++++++------------------ xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 8 ++- xen/include/asm-x86/p2m.h | 27 +++------- 5 files changed, 93 insertions(+), 65 deletions(-)The PoD layer has a comples locking discipline. It relies on the p2m being globally locked, and it also relies on the page alloc lock to protect some of its data structures. Replace this all by an explicit pod lock: per p2m, order enforced.
Quoted 8 lines
Three consequences:
- Critical sections in the pod code protected by the page alloc lock are now reduced to modifications of the domain page list.
- When the p2m lock becomes fine-grained, there are no assumptions broken in the PoD layer.
- The locking is easier to understand.Signed-off-by Andres Lagar-Cavilla <[email protected]>This needs an Ack from George, too. Also:
Quoted 3 lines
xen/arch/x86/mm/p2m-pod.c +0
-0@@ -922,6 +929,12 @@ p2m_pod_emergency_sweep(struct p2m_domai limit = (start >POD_SWEEP_LIMIT ) ? (start -POD_SWEEP_LIMIT ) :0 ;
Quoted 12 lines
xen/arch/x86/mm/p2m-pod.c p2m_pod_emergency_sweep(struct p2m_domai · line 937
Quoted 9 lines
(quoted diff) +1
-0/* FIXME: Figure out how to avoid superpages */ + /* NOTE: Promote to globally locking the p2m. This will getcomplicated
xen/arch/x86/mm/p2m-pod.c +3
-0+* in a fine-grained scenario. Even if we're to lock each gfn +* individually we must be careful about recursion limits and +* POD_SWEEP_STRIDE. This is why we don't enforce deadlock constraints
xen/arch/x86/mm/p2m-pod.c +2
-0+* between p2m and pod locks */ +p2m_lock (p2m);That's a scary comment. It looks to me as if the
mm-locks.hmechanism _does_ enforce those constraints - am I missing something?
The problem is that the recurse count of a spinlock is not particularly wide. So if you have a loop that does a lot of nested get_gfn*, you may overflow.
The funny bit is that we do enforce ordering, so that part of the comment is stale. Will update.
Andres
Quoted 5 lines
Cheers,
Tim.
Re: [PATCH 3 of 5] Rework locking in the PoD layer
In reply to Andres Lagar-CavillaQuoted 10 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/p2m-pod.c +1
-2@@ -114,15 +114,20 @@ p2m_pod_cache_add(struct p2m_domain *p2munmap_domain_page (b); } +/* First, take all pages off the domain list */ lock_page_alloc (p2m); - -/* First, take all pages off the domain list */ for (i=0 ; i <1 << order ; i++) {
xen/arch/x86/mm/p2m-pod.c p2m_pod_cache_add(struct p2m_domain *p2m · line 130
Quoted 10 lines
xen/arch/x86/mm/p2m-pod.c +6
-0p = page + i;page_list_del (p, &d->page_list); } +/* Ensure that the PoD cache has never been emptied. +* This may cause "zombie domains" since the page will never be freed. */ +BUG_ON ( d->arch.relmem != RELMEM_not_started ); + +unlock_page_alloc (p2m); +
This assert should stay where it is. The idea is to verify after-the-fact that the page_list_add_tail()s *have not* raced with emptying the PoD cache. Having the assert before cannot guarantee that they *will not* race with emptying the PoD cache. Alternately, if we're positive that condition can never happen again, we should just remove the BUG_ON().
If I recall correctly, I put this in here because something ended up calling cache_add after empty_cache(), potentially with the p2m lock already held; that's why I put the BUG_ON() there to begin with.
Quoted 55 lines
xen/arch/x86/mm/p2m-pod.c +13
-2@@ -922,6 +929,12 @@ p2m_pod_emergency_sweep(struct p2m_domai limit = (start >POD_SWEEP_LIMIT ) ? (start -POD_SWEEP_LIMIT ) :0 ;/* FIXME: Figure out how to avoid superpages */ +/* NOTE: Promote to globally locking the p2m. This will get complicated +* in a fine-grained scenario. Even if we're to lock each gfn +* individually we must be careful about recursion limits and +* POD_SWEEP_STRIDE. This is why we don't enforce deadlock constraints +* between p2m and pod locks */ +p2m_lock (p2m);for ( i=p2m->pod.reclaim_single; i >0 ; i-- ) {p2m_access_t a;@@ -940,7 +953,7 @@ p2m_pod_emergency_sweep(struct p2m_domai/* Stop if we're past our limit and we have found *something*. * * NB that this is a zero-sum game; we're increasing our cache size -* by re-increasing our 'debt'. Since we hold the p2m lock, +* by re-increasing our 'debt'. Since we hold the pod lock, * (entry_count - count) must remain the same. */ if ( p2m->pod.count >0 && i < limit )break ;@@ -949,6 +962,7 @@ p2m_pod_emergency_sweep(struct p2m_domaiif ( j )p2m_pod_zero_check (p2m, gfns, j); +p2m_unlock (p2m); p2m->pod.reclaim_single = i ? i -1 : i; }@@ -965,8 +979,9 @@ p2m_pod_demand_populate(struct p2m_domaiint i;ASSERT (gfn_locked_by_me (p2m, gfn)); +pod_lock (p2m); -/* This check is done with the p2m lock held. This will make sure that +/* This check is done with the pod lock held. This will make sure that * even if d->is_dying changes under our feet, p2m_pod_empty_cache() * won't start until we're done. */ if (unlikely (d->is_dying) )@@ -977,6 +992,7 @@ p2m_pod_demand_populate(struct p2m_domai* 1GB region to 2MB chunks for a retry. */ if ( order ==PAGE_ORDER_1G ) { +pod_unlock (p2m); gfn_aligned = (gfn >> order) << order;/* Note that we are supposed to call set_p2m_entry() 512 times to * split 1GB into 512 2MB pages here. But We only do once here because @@ -1000,11 +1016,15 @@ p2m_pod_demand_populate(struct p2m_domai/* If we're low, start a sweep */ if ( order ==PAGE_ORDER_2M &&page_list_empty (&p2m->pod.super) ) +/* Note that sweeps scan other ranges in the p2m. In an scenario +* in which p2m locks are order-enforced wrt pod lock and p2m
xen/arch/x86/mm/p2m-pod.c p2m_pod_demand_populate(struct p2m_domai · line 1030
Quoted 10 lines
xen/arch/x86/mm/p2m-pod.c +2
-0+* locks are fine grained, this will result in deadlock panic */ p2m_pod_emergency_sweep_super (p2m);if (page_list_empty (&p2m->pod.single) && ( ( order ==PAGE_ORDER_4K ) || (order ==PAGE_ORDER_2M &&page_list_empty (&p2m->pod.super) ) ) ) +/* Same comment regarding deadlock applies */ p2m_pod_emergency_sweep (p2m); }
Regarding locking on emergency sweeps: I think it should suffice if we could do the equivalent of a spin_trylock() on each gpfn, and just move on (not checking that gfn) if it fails. What do you think?
Other than that, I don't see anything wrong with this locking at first glance; but it's complicated enough that I don't think I've quite grokked it yet. I'll look at it again tomorrow and see if things are more clear. :-)
-George
Quoted 169 lines
xen/arch/x86/mm/p2m-pod.c +10
-8@@ -1012,8 +1032,6 @@ p2m_pod_demand_populate(struct p2m_domaiif ( q == p2m_guest && gfn > p2m->pod.max_guest ) p2m->pod.max_guest = gfn; -lock_page_alloc (p2m); -if ( p2m->pod.count ==0 ) goto out_of_memory;@@ -1026,8 +1044,6 @@ p2m_pod_demand_populate(struct p2m_domaiBUG_ON ((mfn_x (mfn) & ((1 << order)-1 )) !=0 ); -unlock_page_alloc (p2m); - gfn_aligned = (gfn >> order) << order;set_p2m_entry (p2m, gfn_aligned, mfn, order, p2m_ram_rw, p2m->default_access);@@ -1038,7 +1054,7 @@ p2m_pod_demand_populate(struct p2m_domaipaging_mark_dirty (d,mfn_x (mfn) + i); } - p2m->pod.entry_count -= (1 << order);/* Lock: p2m */ + p2m->pod.entry_count -= (1 << order);BUG_ON (p2m->pod.entry_count <0 );if ( tb_init_done )@@ -1056,20 +1072,24 @@ p2m_pod_demand_populate(struct p2m_domai__trace_var (TRC_MEM_POD_POPULATE ,0 ,sizeof (t), &t); } +pod_unlock (p2m);return 0 ; out_of_memory: -unlock_page_alloc (p2m); +pod_unlock (p2m);printk ("%s: Out of populate-on-demand memory! tot_pages %" PRIu32" pod_entries %" PRIi32"\n" , __func__, d->tot_pages, p2m->pod.entry_count);domain_crash (d); out_fail: +pod_unlock (p2m);return -1 ; remap_and_retry:BUG_ON (order !=PAGE_ORDER_2M ); -unlock_page_alloc (p2m); +pod_unlock (p2m);/* Remap this 2-meg region in singleton chunks */ +/* NOTE: In a p2m fine-grained lock scenario this might +* need promoting the gfn lock from gfn->2M superpage */ gfn_aligned = (gfn>>order)<<order;for (i=0 ; i<(1 <<order); i++) set_p2m_entry(p2m, gfn_aligned+i, _mfn(0), PAGE_ORDER_4K,@@ -1137,9 +1157,11 @@ guest_physmap_mark_populate_on_demand(st rc = -EINVAL ; else { - p2m->pod.entry_count +=1 << order;/* Lock: p2m */ +pod_lock (p2m); + p2m->pod.entry_count +=1 << order; p2m->pod.entry_count -= pod_count;BUG_ON (p2m->pod.entry_count <0 ); +pod_unlock (p2m); }gfn_unlock (p2m, gfn, order);diff -r
fb9c06df05f2-r56ceab0118cbxen/arch/x86/mm/p2m-pt.cxen/arch/x86/mm/p2m-pt.c +1
-0@@ -954,6 +954,7 @@ long p2m_pt_audit_p2m(struct p2m_domainstruct domain *d = p2m->domain;ASSERT (p2m_locked_by_me (p2m)); +ASSERT (pod_locked_by_me (p2m)); test_linear = ( (d == current->domain) && !pagetable_is_null (current->arch.monitor_table) );diff -r
fb9c06df05f2-r56ceab0118cbxen/arch/x86/mm/p2m.cxen/arch/x86/mm/p2m.c +7
-1@@ -72,6 +72,7 @@ boolean_param("hap_2mb", opt_hap_2mb);static void p2m_initialise (struct domain *d,struct p2m_domain *p2m) {mm_lock_init (&p2m->lock); +mm_lock_init (&p2m->pod.lock);INIT_LIST_HEAD (&p2m->np2m_list);INIT_PAGE_LIST_HEAD (&p2m->pages);INIT_PAGE_LIST_HEAD (&p2m->pod.super);@@ -587,8 +588,10 @@ guest_physmap_add_entry(struct domain *d rc = -EINVAL ; else { - p2m->pod.entry_count -= pod_count;/* Lock: p2m */ +pod_lock (p2m); + p2m->pod.entry_count -= pod_count;BUG_ON (p2m->pod.entry_count <0 ); +pod_unlock (p2m); } }@@ -1372,6 +1375,7 @@ p2m_flush_table(struct p2m_domain *p2m)/* "Host" p2m tables can have shared entries &c that need a bit more * care when discarding them */ ASSERT (p2m_is_nestedp2m (p2m)); +/* Nested p2m's do not do pod, hence the asserts (and no pod lock)*/ ASSERT (page_list_empty (&p2m->pod.super));ASSERT (page_list_empty (&p2m->pod.single));@@ -1529,6 +1533,7 @@ void audit_p2m(struct domain *d,P2M_PRINTK ("p2m audit starts\n" );p2m_lock (p2m); +pod_lock (p2m);if (p2m->audit_p2m) pmbad = p2m->audit_p2m (p2m);@@ -1589,6 +1594,7 @@ void audit_p2m(struct domain *d, }spin_unlock (&d->page_alloc_lock); +pod_unlock (p2m);p2m_unlock (p2m);P2M_PRINTK ("p2m audit complete\n" );diff -r
fb9c06df05f2-r56ceab0118cbxen/include/asm-x86/p2m.hxen/include/asm-x86/p2m.h +8
-19@@ -261,25 +261,12 @@ struct p2m_domain {unsigned long max_mapped_pfn;/* Populate-on-demand variables -* NB on locking. {super,single,count} are -* covered by d->page_alloc_lock, since they're almost always used in -* conjunction with that functionality. {entry_count} is covered by -* the domain p2m lock, since it's almost always used in conjunction -* with changing the p2m tables. -* -* At this point, both locks are held in two places. In both, -* the order is [p2m,page_alloc]: -* + p2m_pod_decrease_reservation() calls p2m_pod_cache_add(), -* which grabs page_alloc -* + p2m_pod_demand_populate() grabs both; the p2m lock to avoid -* double-demand-populating of pages, the page_alloc lock to -* protect moving stuff from the PoD cache to the domain page list. -* -* We enforce this lock ordering through a construct in mm-locks.h. -* This demands, however, that we store the previous lock-ordering -* level in effect before grabbing the page_alloc lock. The unlock -* level is stored in the arch section of the domain struct. -*/ +* All variables are protected with the pod lock. We cannot rely on +* the p2m lock if it's turned into a fine-grained lock. +* We only use the domain page_alloc lock for additions and +* deletions to the domain's page list. Because we use it nested +* within the PoD lock, we enforce it's ordering (by remembering +* the unlock level in the arch_domain sub struct). */ struct {struct page_list_head super,/* List of superpages */ single;/* Non-super lists */ @@ -288,6 +275,8 @@ struct p2m_domain {unsigned reclaim_super;/* Last gpfn of a scan */ unsigned reclaim_single;/* Last gpfn of a scan */ unsigned max_guest;/* gpfn of max guest demand-populate */ +mm_lock_t lock;/* Locking of private pod structs, * +* not relying on the p2m lock. */ } pod; };
Re: [PATCH 3 of 5] Rework locking in the PoD layer
Quoted 15 lines · Andres Lagar-Cavilla
Quoted 14 lines
xen/arch/x86/mm/p2m-pod.c +1
-2@@ -114,15 +114,20 @@ p2m_pod_cache_add(struct p2m_domain *p2munmap_domain_page (b); } +/* First, take all pages off the domain list */ lock_page_alloc (p2m); - -/* First, take all pages off the domain list */ for (i=0 ; i <1 << order ; i++) { p = page + i;page_list_del (p, &d->page_list); }
Quoted 14 lines
xen/arch/x86/mm/p2m-pod.c p2m_pod_cache_add(struct p2m_domain *p2m · line 130
Quoted 7 lines
(quoted diff) +2
-0+ /* Ensure that the PoD cache has never been emptied. + * This may cause "zombie domains" since the page will never befreed. */
xen/arch/x86/mm/p2m-pod.c +4
-0+BUG_ON ( d->arch.relmem != RELMEM_not_started ); + +unlock_page_alloc (p2m); +This assert should stay where it is. The idea is to verify after-the-fact that the page_list_add_tail()s *have not* raced with emptying the PoD cache. Having the assert before cannot guarantee that they *will not* race with emptying the PoD cache. Alternately, if we're positive that condition can never happen again, we should just remove the
BUG_ON().
As is, the code in cache_add mutually excludes p2m_pod_empty_cache until pod_lock is released by the caller. So, the page_list_add_tails()s cannot race with emptying the PoD cache.
On further look, the BUG_ON can go. relmem moves out of _not_started only after p2m_pod_empty_cache has finished. So, transitively, we're safe.
Emptying the pod cache does the spin barrier. Question: should it not take the lock after the spin barrier, to be super safe?
Thanks!
Andres
Quoted 75 lines
If I recall correctly, I put this in here because something ended up calling
cache_addafterempty_cache(), potentially with the p2m lock already held; that's why I put theBUG_ON()there to begin with.Quoted 70 lines
xen/arch/x86/mm/p2m-pod.c +1
-0@@ -922,6 +929,12 @@ p2m_pod_emergency_sweep(struct p2m_domai limit = (start >POD_SWEEP_LIMIT ) ? (start -POD_SWEEP_LIMIT ) :0 ;/* FIXME: Figure out how to avoid superpages */ +/* NOTE: Promote to globally locking the p2m. This will get complicated
xen/arch/x86/mm/p2m-pod.c +3
-0+* in a fine-grained scenario. Even if we're to lock each gfn +* individually we must be careful about recursion limits and +* POD_SWEEP_STRIDE. This is why we don't enforce deadlock constraints
xen/arch/x86/mm/p2m-pod.c +2
-0+* between p2m and pod locks */ +p2m_lock (p2m);for ( i=p2m->pod.reclaim_single; i >0 ; i-- ) {p2m_access_t a;@@ -940,7 +953,7 @@ p2m_pod_emergency_sweep(struct p2m_domai/* Stop if we're past our limit and we have found *something*. * * NB that this is a zero-sum game; we're increasing our cache size
xen/arch/x86/mm/p2m-pod.c +3
-2-* by re-increasing our 'debt'. Since we hold the p2m lock, +* by re-increasing our 'debt'. Since we hold the pod lock, * (entry_count - count) must remain the same. */ if ( p2m->pod.count >0 && i < limit )break ;@@ -949,6 +962,7 @@ p2m_pod_emergency_sweep(struct p2m_domaiif ( j )p2m_pod_zero_check (p2m, gfns, j); +p2m_unlock (p2m); p2m->pod.reclaim_single = i ? i -1 : i; }@@ -965,8 +979,9 @@ p2m_pod_demand_populate(struct p2m_domaiint i;ASSERT (gfn_locked_by_me (p2m, gfn)); +pod_lock (p2m); -/* This check is done with the p2m lock held. This will make sure that
+ /* This check is done with the pod lock held. This will make sure
that
* even ifd->is_dyingchanges under our feet,p2m_pod_empty_cache()
* won't start until we're done. */
if (unlikely(d->is_dying))xen/arch/x86/mm/p2m-pod.c +1
-0@@ -977,6 +992,7 @@ p2m_pod_demand_populate(struct p2m_domai* 1GB region to 2MB chunks for a retry. */ if ( order ==PAGE_ORDER_1G ) { +pod_unlock (p2m); gfn_aligned = (gfn >> order) << order;/* Note that we are supposed to call set_p2m_entry() 512 times to
* split 1GB into 512 2MB pages here. But We only do once here
because(quoted diff) +1
-0@@ -1000,11 +1016,15 @@ p2m_pod_demand_populate(struct p2m_domai /* If we're low, start a sweep */ if ( order == PAGE_ORDER_2M && page_list_empty(&p2m->pod.super) ) + /* Note that sweeps scan other ranges in the p2m. In anscenario
+ * in which p2m locks are order-enforced wrt pod lock and
p2m
+ * locks are fine grained, this will result in deadlock
panic */p2m_pod_emergency_sweep_super(p2m);
Quoted 9 lines
Quoted 8 lines
if ( page_list_empty(&p2m->pod.single) && ( ( order == PAGE_ORDER_4K ) || (order == PAGE_ORDER_2M && page_list_empty(&p2m->pod.super) ) ) ) + /* Same comment regarding deadlock applies */ p2m_pod_emergency_sweep(p2m); }
You know what, these comments are *old*. I really need to cut down on the FUD factor :) As is, there is no risk of deadlock.
With a (hypothetical as of today) fine-grained p2m locking approach, I want to leave a "be careful" reminder. And document your trylock suggestion, which I believe to be accurate.
Thanks for the detailed look!
Andres
Quoted 198 lines
Regarding locking on emergency sweeps: I think it should suffice if we could do the equivalent of a
spin_trylock()on each gpfn, and just move on (not checking that gfn) if it fails. What do you think?Other than that, I don't see anything wrong with this locking at first glance; but it's complicated enough that I don't think I've quite grokked it yet. I'll look at it again tomorrow and see if things are more clear. :-)
-George
Quoted 183 lines
xen/arch/x86/mm/p2m-pod.c +0
-4@@ -1012,8 +1032,6 @@ p2m_pod_demand_populate(struct p2m_domaiif ( q == p2m_guest && gfn > p2m->pod.max_guest ) p2m->pod.max_guest = gfn; -lock_page_alloc (p2m); -if ( p2m->pod.count ==0 ) goto out_of_memory;@@ -1026,8 +1044,6 @@ p2m_pod_demand_populate(struct p2m_domaiBUG_ON ((mfn_x (mfn) & ((1 << order)-1 )) !=0 ); -unlock_page_alloc (p2m); - gfn_aligned = (gfn >> order) << order; set_p2m_entry(p2m, gfn_aligned, mfn, order, p2m_ram_rw,
p2m->default_access);xen/arch/x86/mm/p2m-pod.c +7
-3@@ -1038,7 +1054,7 @@ p2m_pod_demand_populate(struct p2m_domaipaging_mark_dirty (d,mfn_x (mfn) + i); } - p2m->pod.entry_count -= (1 << order);/* Lock: p2m */ + p2m->pod.entry_count -= (1 << order);BUG_ON (p2m->pod.entry_count <0 );if ( tb_init_done )@@ -1056,20 +1072,24 @@ p2m_pod_demand_populate(struct p2m_domai__trace_var (TRC_MEM_POD_POPULATE ,0 ,sizeof (t), &t); } +pod_unlock (p2m);return 0 ; out_of_memory: -unlock_page_alloc (p2m); +pod_unlock (p2m); printk("%s: Out of populate-on-demand memory! tot_pages %" PRIu32 " pod_entries %" PRIi32 "\n", __func__, d->tot_pages, p2m->pod.entry_count);domain_crash (d); out_fail: +pod_unlock (p2m);return -1 ; remap_and_retry:BUG_ON (order !=PAGE_ORDER_2M ); -unlock_page_alloc (p2m); +pod_unlock (p2m);/* Remap this 2-meg region in singleton chunks */ +/* NOTE: In a p2m fine-grained lock scenario this might +* need promoting the gfn lock from gfn->2M superpage */ gfn_aligned = (gfn>>order)<<order;for (i=0 ; i<(1 <<order); i++)set_p2m_entry(p2m, gfn_aligned+i, _mfn(0), PAGE_ORDER_4K,xen/arch/x86/mm/p2m-pod.c +3
-1@@ -1137,9 +1157,11 @@ guest_physmap_mark_populate_on_demand(st rc = -EINVAL ; else { - p2m->pod.entry_count +=1 << order;/* Lock: p2m */ +pod_lock (p2m); + p2m->pod.entry_count +=1 << order; p2m->pod.entry_count -= pod_count;BUG_ON (p2m->pod.entry_count <0 ); +pod_unlock (p2m); }gfn_unlock (p2m, gfn, order);diff -r
fb9c06df05f2-r56ceab0118cbxen/arch/x86/mm/p2m-pt.cxen/arch/x86/mm/p2m-pt.c +1
-0@@ -954,6 +954,7 @@ long p2m_pt_audit_p2m(struct p2m_domainstruct domain *d = p2m->domain;ASSERT (p2m_locked_by_me (p2m)); +ASSERT (pod_locked_by_me (p2m)); test_linear = ( (d == current->domain) && !pagetable_is_null(current->arch.monitor_table));
diff -rfb9c06df05f2-r56ceab0118cbxen/arch/x86/mm/p2m.cxen/arch/x86/mm/p2m.c +4
-1@@ -72,6 +72,7 @@ boolean_param("hap_2mb", opt_hap_2mb);static void p2m_initialise (struct domain *d,struct p2m_domain *p2m) {mm_lock_init (&p2m->lock); +mm_lock_init (&p2m->pod.lock);INIT_LIST_HEAD (&p2m->np2m_list);INIT_PAGE_LIST_HEAD (&p2m->pages);INIT_PAGE_LIST_HEAD (&p2m->pod.super);@@ -587,8 +588,10 @@ guest_physmap_add_entry(struct domain *d rc = -EINVAL ; else { - p2m->pod.entry_count -= pod_count;/* Lock: p2m */ +pod_lock (p2m); + p2m->pod.entry_count -= pod_count;BUG_ON (p2m->pod.entry_count <0 ); +pod_unlock (p2m); } }@@ -1372,6 +1375,7 @@ p2m_flush_table(struct p2m_domain *p2m)/* "Host" p2m tables can have shared entries &c that need a bit more
* care when discarding them */
ASSERT(p2m_is_nestedp2m(p2m));
+ /* Nested p2m's do not do pod, hence the asserts (and no pod
lock)*/ASSERT(page_list_empty(&p2m->pod.super)); ASSERT(page_list_empty(&p2m->pod.single));xen/arch/x86/mm/p2m.c +2
-0@@ -1529,6 +1533,7 @@ void audit_p2m(struct domain *d,P2M_PRINTK ("p2m audit starts\n" );p2m_lock (p2m); +pod_lock (p2m);if (p2m->audit_p2m) pmbad = p2m->audit_p2m (p2m);@@ -1589,6 +1594,7 @@ void audit_p2m(struct domain *d, }spin_unlock (&d->page_alloc_lock); +pod_unlock (p2m);p2m_unlock (p2m);P2M_PRINTK ("p2m audit complete\n" );diff -r
fb9c06df05f2-r56ceab0118cbxen/include/asm-x86/p2m.hxen/include/asm-x86/p2m.h +0
-2@@ -261,25 +261,12 @@ struct p2m_domain {unsigned long max_mapped_pfn;/* Populate-on-demand variables -* NB on locking. {super,single,count} are -* covered by d->page_alloc_lock, since they're almost always used in
- * conjunction with that functionality. {entry_count} is covered
by
- * the domain p2m lock, since it's almost always used in
conjunctionxen/include/asm-x86/p2m.h +0
-9-* with changing the p2m tables. -* -* At this point, both locks are held in two places. In both, -* the order is [p2m,page_alloc]: -* + p2m_pod_decrease_reservation() calls p2m_pod_cache_add(), -* which grabs page_alloc -* + p2m_pod_demand_populate() grabs both; the p2m lock to avoid -* double-demand-populating of pages, the page_alloc lock to -* protect moving stuff from the PoD cache to the domain page list.
xen/include/asm-x86/p2m.h +6
-6-* -* We enforce this lock ordering through a construct in mm-locks.h. -* This demands, however, that we store the previous lock-ordering -* level in effect before grabbing the page_alloc lock. The unlock -* level is stored in the arch section of the domain struct. -*/ +* All variables are protected with the pod lock. We cannot rely on +* the p2m lock if it's turned into a fine-grained lock. +* We only use the domain page_alloc lock for additions and +* deletions to the domain's page list. Because we use it nested +* within the PoD lock, we enforce it's ordering (by remembering +* the unlock level in the arch_domain sub struct). */ struct {struct page_list_head super,/* List of superpages */ single;/* Non-super lists */ @@ -288,6 +275,8 @@ struct p2m_domain {unsigned reclaim_super;/* Last gpfn of a scan */ unsigned reclaim_single;/* Last gpfn of a scan */ unsigned max_guest;/* gpfn of max guest demand-populate */
+mm_lock_tlock; /* Locking of private pod
structs, *
+ * not relying on the p2m lock.*/ } pod;};
PATCH 1/1 Re-order calls to put_gfn() around wait queu invocations
In reply to Andres Lagar-Cavillaxen/arch/x86/hvm/emulate.c | 2 +- xen/arch/x86/hvm/hvm.c | 25 +++++++++++++------ xen/arch/x86/mm.c | 10 +++--- xen/arch/x86/mm/guest_walk.c | 2 +- xen/arch/x86/mm/hap/guest_walk.c | 6 +--- xen/arch/x86/mm/mem_access.c | 10 +++++++ xen/arch/x86/mm/mem_event.c | 5 +++ xen/arch/x86/mm/p2m.c | 52 ++++++++++++++++++++++----------------- xen/common/grant_table.c | 2 +- xen/common/memory.c | 2 +- xen/include/asm-x86/mem_access.h | 1 + xen/include/asm-x86/mem_event.h | 3 ++ xen/include/asm-x86/p2m.h | 6 +++- 13 files changed, 80 insertions(+), 46 deletions(-)
Since we use wait queues to handle potential ring congestion cases, code paths that try to generate a mem event while holding a gfn lock would go to sleep in non-preemptible mode.
Most such code paths can be fixed by simply postponing event generation until locks are released.
xen/arch/x86/hvm/emulate.c +1 -1
@@ -67,8 +67,8 @@ static int hvmemul_do_io( ram_mfn =get_gfn_unshare (curr->domain, ram_gfn, &p2mt);if (p2m_is_paging (p2mt) ) { +put_gfn (curr->domain, ram_gfn);p2m_mem_paging_populate (curr->domain, ram_gfn); -put_gfn (curr->domain, ram_gfn);return X86EMUL_RETRY ; }if (p2m_is_shared (p2mt) )
xen/arch/x86/hvm/hvm.c +17 -8
@@ -64,6 +64,7 @@#include <public/version.h> #include <public/memory.h> #include <asm/mem_event.h> +#include <asm/mem_access.h> #include <public/mem_event.h> bool_t __read_mostly hvm_enabled;
@@ -363,8 +364,8 @@ static int hvm_set_ioreq_page( }if (p2m_is_paging (p2mt) ) { +put_gfn (d, gmfn);p2m_mem_paging_populate (d, gmfn); -put_gfn (d, gmfn);return -ENOENT ; }if (p2m_is_shared (p2mt) )
@@ -1195,7 +1196,8 @@ int hvm_hap_nested_page_fault(unsigned lmfn_t mfn;struct vcpu *v = current;struct p2m_domain *p2m; -int rc, fall_through =0 ; +int rc, fall_through =0 , paged =0 ; +mem_event_request_t *req_ptr =NULL ;/* On Nested Virtualization, walk the guest page table. * If this succeeds, all is fine.
@@ -1270,7 +1272,7 @@ int hvm_hap_nested_page_fault(unsigned lif ( violation ) {if (p2m_mem_access_check (gpa, gla_valid, gla, access_r, - access_w, access_x) ) + access_w, access_x, &req_ptr) ) { fall_through =1 ; }else {
@@ -1297,7 +1299,7 @@ int hvm_hap_nested_page_fault(unsigned l#ifdef __x86_64__/* Check if the page has been paged out */ if (p2m_is_paged (p2mt) || (p2mt == p2m_ram_paging_out) ) -p2m_mem_paging_populate (v->domain, gfn); + paged =1 ;/* Mem sharing: unshare the page and try again */ if ( access_w && (p2mt == p2m_ram_shared) )
@@ -1343,6 +1345,13 @@ int hvm_hap_nested_page_fault(unsigned l out_put_gfn:put_gfn (p2m->domain, gfn); +if ( paged ) +p2m_mem_paging_populate (v->domain, gfn); +if ( req_ptr ) + { +mem_access_send_req (v->domain, req_ptr); +xfree (req_ptr); + }return rc; }
@@ -1849,8 +1858,8 @@ static void *__hvm_map_guest_frame(unsig }if (p2m_is_paging (p2mt) ) { +put_gfn (d, gfn);p2m_mem_paging_populate (d, gfn); -put_gfn (d, gfn);return NULL ; }
@@ -2325,8 +2334,8 @@ static enum hvm_copy_result __hvm_copy(if (p2m_is_paging (p2mt) ) { +put_gfn (curr->domain, gfn);p2m_mem_paging_populate (curr->domain, gfn); -put_gfn (curr->domain, gfn);return HVMCOPY_gfn_paged_out; }if (p2m_is_shared (p2mt) )
@@ -3923,8 +3932,8 @@ long do_hvm_op(unsigned long op, XEN_GUEmfn_t mfn =get_gfn_unshare (d, pfn, &t);if (p2m_is_paging (t) ) { +put_gfn (d, pfn);p2m_mem_paging_populate (d, pfn); -put_gfn (d, pfn); rc = -EINVAL ; goto param_fail3; }
@@ -4040,8 +4049,8 @@ long do_hvm_op(unsigned long op, XEN_GUE mfn =get_gfn_unshare (d, pfn, &t);if (p2m_is_paging (t) ) { +put_gfn (d, pfn);p2m_mem_paging_populate (d, pfn); -put_gfn (d, pfn); rc = -EINVAL ; goto param_fail4; }
xen/arch/x86/mm.c +5 -5
@@ -3536,8 +3536,8 @@ int do_mmu_update(if (p2m_is_paged (p2mt) ) { +put_gfn (pt_owner, gmfn);p2m_mem_paging_populate (pg_owner, gmfn); -put_gfn (pt_owner, gmfn); rc = -ENOENT ;break ; }
@@ -3568,8 +3568,8 @@ int do_mmu_update(if (p2m_is_paged (l1e_p2mt) ) { +put_gfn (pg_owner, l1egfn);p2m_mem_paging_populate (pg_owner,l1e_get_pfn (l1e)); -put_gfn (pg_owner, l1egfn); rc = -ENOENT ;break ; }
@@ -3617,8 +3617,8 @@ int do_mmu_update(if (p2m_is_paged (l2e_p2mt) ) { +put_gfn (pg_owner, l2egfn);p2m_mem_paging_populate (pg_owner, l2egfn); -put_gfn (pg_owner, l2egfn); rc = -ENOENT ;break ; }
@@ -3652,8 +3652,8 @@ int do_mmu_update(if (p2m_is_paged (l3e_p2mt) ) { +put_gfn (pg_owner, l3egfn);p2m_mem_paging_populate (pg_owner, l3egfn); -put_gfn (pg_owner, l3egfn); rc = -ENOENT ;break ; }
@@ -3687,8 +3687,8 @@ int do_mmu_update(if (p2m_is_paged (l4e_p2mt) ) { +put_gfn (pg_owner, l4egfn);p2m_mem_paging_populate (pg_owner, l4egfn); -put_gfn (pg_owner, l4egfn); rc = -ENOENT ;break ; }
xen/arch/x86/mm/guest_walk.c +1 -1
@@ -102,8 +102,8 @@ static inline void *map_domain_gfn(strucif (p2m_is_paging (*p2mt) ) {ASSERT (!p2m_is_nestedp2m (p2m)); +__put_gfn (p2m,gfn_x (gfn));p2m_mem_paging_populate (p2m->domain,gfn_x (gfn)); -__put_gfn (p2m,gfn_x (gfn)); *rc = _PAGE_PAGED;return NULL ; }
xen/arch/x86/mm/hap/guest_walk.c +2 -4
@@ -64,10 +64,9 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAif (p2m_is_paging (p2mt) ) {ASSERT (!p2m_is_nestedp2m (p2m)); -p2m_mem_paging_populate (p2m->domain, cr3 >>PAGE_SHIFT ); - pfec[0 ] = PFEC_page_paged;__put_gfn (p2m, top_gfn); +p2m_mem_paging_populate (p2m->domain, cr3 >>PAGE_SHIFT );return INVALID_GFN ; }if (p2m_is_shared (p2mt) )
@@ -101,10 +100,9 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAif (p2m_is_paging (p2mt) ) {ASSERT (!p2m_is_nestedp2m (p2m)); -p2m_mem_paging_populate (p2m->domain,gfn_x (gfn)); - pfec[0 ] = PFEC_page_paged;__put_gfn (p2m,gfn_x (gfn)); +p2m_mem_paging_populate (p2m->domain,gfn_x (gfn));return INVALID_GFN ; }if (p2m_is_shared (p2mt) )
xen/arch/x86/mm/mem_access.c +10 -0
@@ -47,6 +47,16 @@ int mem_access_domctl(struct domain *d,return rc; } +int mem_access_send_req (struct domain *d,mem_event_request_t *req) +{ +int rc =mem_event_claim_slot (d, &d->mem_event->access); +if ( rc <0 ) +return rc; + +mem_event_put_request (d, &d->mem_event->access, req); + +return 0 ; +}/* * Local variables:
xen/arch/x86/mm/mem_event.c +5 -0
@@ -423,6 +423,11 @@ static int mem_event_wait_slot(struct mereturn rc; } +bool_t mem_event_check_ring (struct mem_event_domain *med) +{ +return (med->ring_page !=NULL ); +} +/* * Determines whether or not the current vCPU belongs to the target domain, * and calls the appropriate wait function. If it is a guest vCPU, then we
xen/arch/x86/mm/p2m.c +29 -23
82 lines not on this page · raw message
xen/common/grant_table.c +1 -1
@@ -164,8 +164,8 @@ static int __get_paged_frame(unsigned lo *frame =mfn_x (mfn);if (p2m_is_paging (p2mt) ) { +put_gfn (rd, gfn);p2m_mem_paging_populate (rd, gfn); -put_gfn (rd, gfn); rc = GNTST_eagain; } }else {
xen/common/memory.c +1 -1
@@ -166,8 +166,8 @@ int guest_remove_page(struct domain *d,if (unlikely (p2m_is_paging (p2mt)) ) {guest_physmap_remove_page (d, gmfn, mfn,0 ); +put_gfn (d, gmfn);p2m_mem_paging_drop_page (d, gmfn, p2mt); -put_gfn (d, gmfn);return 1 ; }#else
xen/include/asm-x86/mem_access.h +1 -0
@@ -23,6 +23,7 @@int mem_access_domctl (struct domain *d,xen_domctl_mem_event_op_t *mec,XEN_GUEST_HANDLE (void ) u_domctl); +int mem_access_send_req (struct domain *d,mem_event_request_t *req);/*
xen/include/asm-x86/mem_event.h +3 -0
@@ -24,6 +24,9 @@#ifndef __MEM_EVENT_H__#define __MEM_EVENT_H__ +/* Returns whether a ring has been set up */ +bool_t mem_event_check_ring (struct mem_event_domain *med); +/* Returns 0 on success, -ENOSYS if there is no ring, -EBUSY if there is no * available space. For success or -EBUSY, the vCPU may be left blocked * temporarily to ensure that the ring does not lose future events. In
xen/include/asm-x86/p2m.h +4 -2
@@ -587,7 +587,8 @@ static inline void p2m_mem_paging_popula* the rw2rx conversion. Boolean return value indicates if access rights have * been promoted with no underlying vcpu pause. */ bool_t p2m_mem_access_check (unsigned long gpa,bool_t gla_valid,unsigned long gla, -bool_t access_r,bool_t access_w,bool_t access_x); +bool_t access_r,bool_t access_w,bool_t access_x, +mem_event_request_t **req_ptr);/* Resumes the running of the VCPU, restarting the last instruction */ void p2m_mem_access_resume (struct domain *d);
@@ -604,7 +605,8 @@ int p2m_get_mem_access(struct domain *d,#else static inline bool_t p2m_mem_access_check (unsigned long gpa,bool_t gla_valid,unsigned long gla,bool_t access_r, -bool_t access_w,bool_t access_x) +bool_t access_w,bool_t access_x, +mem_event_request_t **req_ptr); {return 1 ; } static inline int p2m_set_mem_access(struct domain *d, unsigned long start_pfn,
Re: [PATCH 4 of 5] Re-order calls to put_gfn() around wait queu invocations
Quoted 20 lines · Andres Lagar-Cavilla
xen/arch/x86/hvm/emulate.c | 2 +- xen/arch/x86/hvm/hvm.c | 25 +++++++++++++------ xen/arch/x86/mm.c | 10 +++--- xen/arch/x86/mm/guest_walk.c | 2 +- xen/arch/x86/mm/hap/guest_walk.c | 6 +--- xen/arch/x86/mm/mem_access.c | 10 +++++++ xen/arch/x86/mm/mem_event.c | 5 +++ xen/arch/x86/mm/p2m.c | 52 ++++++++++++++++++++++----------------- xen/common/grant_table.c | 2 +- xen/common/memory.c | 2 +- xen/include/asm-x86/mem_access.h | 1 + xen/include/asm-x86/mem_event.h | 3 ++ xen/include/asm-x86/p2m.h | 6 +++- 13 files changed, 80 insertions(+), 46 deletions(-)Since we use wait queues to handle potential ring congestion cases, code paths that try to generate a mem event while holding a gfn lock would go to sleep in non-preemptible mode.
Quoted 5 lines
Most such code paths can be fixed by simply postponing event generation until locks are released.
Signed-off-by Adin Scannell <[email protected]>Signed-off-by Andres Lagar-Cavilla <[email protected]>
Looks OK, once the preceding patches are sorted out. If you add some comments to p2m_mem_access_check() describing its new argument and what the caller's supposed to do about it, you can put my Ack on this next time.
Cheers,
Tim.
PATCH 1/1 x86/mm: Revert changeset 24582:f6c33cfe7333
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/p2m.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
With synchronized p2m lookups this is no longer needed, and we can lock the p2m up-front.
xen/arch/x86/mm/p2m.c +2 -2
@@ -361,6 +361,8 @@ void p2m_teardown(struct p2m_domain *p2mif (p2m ==NULL )return ; +p2m_lock (p2m); +#ifdef __x86_64__for ( gfn=0 ; gfn < p2m->max_mapped_pfn; gfn++ ) {
@@ -374,8 +376,6 @@ void p2m_teardown(struct p2m_domain *p2m }#endif -p2m_lock (p2m); - p2m->phys_table =pagetable_null ();while ( (pg =page_list_remove_head (&p2m->pages)) )
Re: [PATCH 5 of 5] x86/mm: Revert changeset 24582:f6c33cfe7333
Quoted 8 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/p2m.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)With synchronized p2m lookups this is no longer needed, and we can lock the p2m up-front.
Signed-off-by Andres Lagar-Cavilla <[email protected]>
(once the earlier patches are in, of course!)
Cheers,
Tim.