Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Xen: Devel

[PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn

 

 

Xen devel RSS feed   Index | Next | Previous | View Threaded


andres at lagarcavilla

Apr 24, 2012, 12:34 PM

Post #1 of 3 (99 views)
Permalink
[PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn

xen/arch/x86/mm/hap/guest_walk.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)


We don't need to hold the p2m lock for the duration of the guest walk. We need
to ensure livenes of the top level page.

Signed-off-by: Andres Lagar-Cavilla <andres [at] lagarcavilla>

diff -r 34c7e6be9265 -r 58fd70123787 xen/arch/x86/mm/hap/guest_walk.c
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -52,6 +52,7 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
{
uint32_t missing;
mfn_t top_mfn;
+ struct page_info *top_page;
void *top_map;
p2m_type_t p2mt;
p2m_access_t p2ma;
@@ -85,13 +86,16 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA

/* Map the top-level table and call the tree-walker */
ASSERT(mfn_valid(mfn_x(top_mfn)));
+ top_page = mfn_to_page(mfn_x(top_mfn));
+ ASSERT(get_page(top_page, p2m->domain));
+ __put_gfn(p2m, top_gfn);
top_map = map_domain_page(mfn_x(top_mfn));
#if GUEST_PAGING_LEVELS == 3
top_map += (cr3 & ~(PAGE_MASK | 31));
#endif
missing = guest_walk_tables(v, p2m, ga, &gw, pfec[0], top_mfn, top_map);
unmap_domain_page(top_map);
- __put_gfn(p2m, top_gfn);
+ put_page(top_page);

/* Interpret the answer */
if ( missing == 0 )

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


tim at xen

Apr 25, 2012, 5:51 AM

Post #2 of 3 (94 views)
Permalink
Re: [PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn [In reply to]

At 15:34 -0400 on 24 Apr (1335281651), Andres Lagar-Cavilla wrote:
> xen/arch/x86/mm/hap/guest_walk.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
>
> We don't need to hold the p2m lock for the duration of the guest walk. We need
> to ensure livenes of the top level page.

I'm not sure I want to start taking these s/gfn-lock/get-page/ changes
piecemeal without a clear understanding of why the gfn-locking is not
needed. My understanding was that

- get_page protects against the page being freed and reused.
- gfn-lock protects against the GFN being reused, or the page
being reused within the VM.

Are there some cases where we only care about the first of these and
some where we care about both? In other words, can we replace the
gfn-locking everywhere with get_page/put_page (i.e. get rid of put_gfn)?

Also:

> @@ -85,13 +86,16 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>
> /* Map the top-level table and call the tree-walker */
> ASSERT(mfn_valid(mfn_x(top_mfn)));
> + top_page = mfn_to_page(mfn_x(top_mfn));
> + ASSERT(get_page(top_page, p2m->domain));

ASSERT(foo) is compiled out on non-debug builds, so that's definitely
not what you want.

I personally dislike this idiom with BUG_ON() as well, because even
though BUG_ON(some side-effecting statement) may be correct, my eye
tends to skip over it when skimming code.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


andres at lagarcavilla

Apr 25, 2012, 8:33 AM

Post #3 of 3 (97 views)
Permalink
Re: [PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn [In reply to]

> At 15:34 -0400 on 24 Apr (1335281651), Andres Lagar-Cavilla wrote:
>> xen/arch/x86/mm/hap/guest_walk.c | 6 +++++-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>>
>>
>> We don't need to hold the p2m lock for the duration of the guest walk.
>> We need
>> to ensure livenes of the top level page.
>
> I'm not sure I want to start taking these s/gfn-lock/get-page/ changes
> piecemeal without a clear understanding of why the gfn-locking is not
> needed. My understanding was that
>
> - get_page protects against the page being freed and reused.
> - gfn-lock protects against the GFN being reused, or the page
> being reused within the VM.

Most cases gfn_lock protects against are already taken care of by the end
of a query. From there on, get_page will protect you against paging out.

gfn_lock will further protect the caller from the gfn being replaced. That
is the risk we are taking in these code paths, and the assumption is that
the VM would not try that on its own pagetables, GDTs, or mmio targets. If
so, Xen won't misbehave, yet the VM will shoot itself in the foot.
Deservedly.

gfn/p2m lock will also prevent modifications elsewhere in the p2m, and
that is good motivation for a fine-grained lock. These code paths don't
care about modifications elsewhere in the p2m.

I agree it's rather ad hoc, and I'm not happy about it. I would prefer a
proper construct that buries all details, or finding out if rwlocks are
feasible. I'm trying to find ways to relieve contention and address Yang's
problem before the window closes. It hasn't seem that successful anyways.

Andres
>
> Are there some cases where we only care about the first of these and
> some where we care about both? In other words, can we replace the
> gfn-locking everywhere with get_page/put_page (i.e. get rid of put_gfn)?
>
> Also:
>
>> @@ -85,13 +86,16 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>>
>> /* Map the top-level table and call the tree-walker */
>> ASSERT(mfn_valid(mfn_x(top_mfn)));
>> + top_page = mfn_to_page(mfn_x(top_mfn));
>> + ASSERT(get_page(top_page, p2m->domain));
>
> ASSERT(foo) is compiled out on non-debug builds, so that's definitely
> not what you want.
>
> I personally dislike this idiom with BUG_ON() as well, because even
> though BUG_ON(some side-effecting statement) may be correct, my eye
> tends to skip over it when skimming code.
>
> Cheers,
>
> Tim.
>



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Xen devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.