thr3ads.net - Xen devel - [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes [Jun 2011]

If this information is useful, please help other people find it:
Share via:

Tim Deegan

2011-Jun-27 10:46 UTC

[Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

This patch series tidies up a few bits ofthe nested p2m code.
The main thing it does is reorganize the locking so that most of the
changes to nested p2m tables happen only under the p2m lock, and the
nestedp2m lock is only needed to reassign p2m tables to new cr3 values.

Changes since v1:

 - a few minor fixes
 - more sensible flushing policy in p2m_get_nestedp2m()
 - smoke-tested this time!

Tim.

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

Tim Deegan

2011-Jun-27 10:46 UTC

head link

[Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 44c7b977302663487922a5d822d1d4032badfebc
# Parent  b240183197720129a8d83847bc5592d6dff3d530
Nested p2m: implement "flush" as a first-class action
rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all
pfns that''s only needed for non-nested p2ms.  It also moves the
tlb flush into the proper place in the flush logic, avoiding a
possible race against other CPUs.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r b24018319772 -r 44c7b9773026 xen/arch/x86/hvm/nestedhvm.c
--- a/xen/arch/x86/hvm/nestedhvm.c	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/arch/x86/hvm/nestedhvm.c	Fri Jun 24 16:24:44 2011 +0100
@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
     cpus_clear(p2m->p2m_dirty_cpumask);
 }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
 bool_t
 nestedhvm_is_n2(struct vcpu *v)
 {
diff -r b24018319772 -r 44c7b9773026 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
     return lrup2m;
 }
 
-static int 
+/* Reset this p2m table to be empty */
+static void
 p2m_flush_locked(struct p2m_domain *p2m)
 {
-    ASSERT(p2m);
-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * => about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;
 
-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+
+    /* "Host" p2m tables can have shared entries &c that need a
bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+    
+    /* Zap the top level of the trie */
+    top = mfn_to_page(pagetable_get_mfn(p2m_get_pagetable(p2m)));
+    p = __map_domain_page(top);
+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top ) 
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top, &p2m->pages);
+
+    p2m_unlock(p2m);
 }
 
 void
@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
     ASSERT(v->domain == d);
     vcpu_nestedhvm(v).nv_p2m = NULL;
     nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
     hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
     nestedp2m_unlock(d);
 }
 
@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
     int i;
 
     nestedp2m_lock(d);
-    for (i = 0; i < MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i < MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
     nestedp2m_unlock(d);
 }
 
@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     volatile struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct domain *d;
     struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
     if (cr3 == 0 || cr3 == CR3_EADDR)
         cr3 = v->arch.hvm_vcpu.guest_cr[3];
@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
      */
     for (i = 0; i < MAX_NESTEDP2M; i++) {
         p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
     }
     nv->nv_p2m = p2m;
     p2m->cr3 = cr3;
diff -r b24018319772 -r 44c7b9773026 xen/include/asm-x86/hvm/nestedhvm.h
--- a/xen/include/asm-x86/hvm/nestedhvm.h	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/include/asm-x86/hvm/nestedhvm.h	Fri Jun 24 16:24:44 2011 +0100
@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
     (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
 void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
 bool_t nestedhvm_is_n2(struct vcpu *v);
 

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

Tim Deegan

2011-Jun-27 10:46 UTC

head link

[Xen-devel] [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 3c756243b74302daee24fa77b9000e4039c897e3
# Parent  44c7b977302663487922a5d822d1d4032badfebc
Nested p2m: remove bogus check of CR3 value.

0 is a valid CR3 value; CR3_EADDR isn''t but there''s nothing
stopping a
guest from putting it in its VMCB.  The special case was broken anyway
since AFAICT "p2m->cr3" is a nester-cr3 (i.e. p2m-table) value and
guest_cr[3] is an actual-cr3 (pagetable) value.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 44c7b9773026 -r 3c756243b743 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1122,8 +1122,8 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     struct p2m_domain *p2m;
     int i;
 
-    if (cr3 == 0 || cr3 == CR3_EADDR)
-        cr3 = v->arch.hvm_vcpu.guest_cr[3];
+    /* Mask out low bits; this avoids collisions with CR3_EADDR */
+    cr3 &= ~(0xfffull);
 
     if (nv->nv_flushp2m && nv->nv_p2m) {
         nv->nv_p2m = NULL;

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

Tim Deegan

2011-Jun-27 10:46 UTC

head link

[Xen-devel] [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m()

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 97e15368260c093078e1f1bc04521de30c1792cc
# Parent  3c756243b74302daee24fa77b9000e4039c897e3
Nested p2m: clarify logic in p2m_get_nestedp2m()

This just makes the behaviour of this function a bit more explicit.  It
may be that it also needs to be changed. :)

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 3c756243b743 -r 97e15368260c xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1131,11 +1131,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
 
     d = v->domain;
     nestedp2m_lock(d);
-    for (i = 0; i < MAX_NESTEDP2M; i++) {
-        p2m = d->arch.nested_p2m[i];
-        if ((p2m->cr3 != cr3 && p2m->cr3 != CR3_EADDR) || (p2m !=
nv->nv_p2m))
-            continue;
-
+    p2m = nv->nv_p2m;
+    if ( p2m && (p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR) )
+    {
         nv->nv_flushp2m = 0;
         p2m_getlru_nestedp2m(d, p2m);
         nv->nv_p2m = p2m;

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

Tim Deegan

2011-Jun-27 10:46 UTC

head link

[Xen-devel] [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
# Parent  97e15368260c093078e1f1bc04521de30c1792cc
Nested p2m: flush only one p2m table when reallocating.
It''s unhelpful to flush all of them when we only need one.

Reported-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     volatile struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct domain *d;
     struct p2m_domain *p2m;
-    int i;
 
     /* Mask out low bits; this avoids collisions with CR3_EADDR */
     cr3 &= ~(0xfffull);
@@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     }
 
     /* All p2m''s are or were in use. Take the least recent used one,
-     * flush it and reuse.
-     */
-    for (i = 0; i < MAX_NESTEDP2M; i++) {
-        p2m = p2m_getlru_nestedp2m(d, NULL);
-        p2m_flush_locked(p2m);
-    }
+     * flush it and reuse. */
+    p2m = p2m_getlru_nestedp2m(d, NULL);
+    p2m_flush_locked(p2m);
     nv->nv_p2m = p2m;
     p2m->cr3 = cr3;
     nv->nv_flushp2m = 0;

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

Tim Deegan

2011-Jun-27 10:46 UTC

head link

[Xen-devel] [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929085 -3600
# Node ID c82cebcfec2546260e4c3b75bb6a47cfdf8bc162
# Parent  0753351afbbe1c3fdde3a72dfb5a67105524f813
Nested p2m: rework locking around nested-p2m flushes and updates.

The nestedp2m_lock now only covers the mapping from nested-cr3 to
nested-p2m; the tables themselves may be updated or flushed using only
the relevant p2m lock.

This means that the nested-p2m lock is only taken on one path, and
always before any p2m locks.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/hap/nested_hap.c
--- a/xen/arch/x86/mm/hap/nested_hap.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/hap/nested_hap.c	Fri Jun 24 16:24:45 2011 +0100
@@ -96,17 +96,23 @@ nestedp2m_write_p2m_entry(struct p2m_dom
 /*          NESTED VIRT FUNCTIONS           */
 /********************************************/
 static void
-nestedhap_fix_p2m(struct p2m_domain *p2m, paddr_t L2_gpa, paddr_t L0_gpa,
-    p2m_type_t p2mt, p2m_access_t p2ma)
+nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m, 
+                  paddr_t L2_gpa, paddr_t L0_gpa,
+                  p2m_type_t p2mt, p2m_access_t p2ma)
 {
-    int rv;
+    int rv = 1;
     ASSERT(p2m);
     ASSERT(p2m->set_entry);
 
     p2m_lock(p2m);
-    rv = set_p2m_entry(p2m, L2_gpa >> PAGE_SHIFT,
-                         page_to_mfn(maddr_to_page(L0_gpa)),
-                         0 /*4K*/, p2mt, p2ma);
+
+    /* If this p2m table has been flushed or recycled under our feet, 
+     * leave it alone.  We''ll pick up the right one as we try to 
+     * vmenter the guest. */
+    if ( p2m->cr3 == nhvm_vcpu_hostcr3(v) )
+         rv = set_p2m_entry(p2m, L2_gpa >> PAGE_SHIFT,
+                            page_to_mfn(maddr_to_page(L0_gpa)),
+                            0 /*4K*/, p2mt, p2ma);
     p2m_unlock(p2m);
 
     if (rv == 0) {
@@ -211,12 +217,10 @@ nestedhvm_hap_nested_page_fault(struct v
         break;
     }
 
-    nestedp2m_lock(d);
     /* fix p2m_get_pagetable(nested_p2m) */
-    nestedhap_fix_p2m(nested_p2m, L2_gpa, L0_gpa,
+    nestedhap_fix_p2m(v, nested_p2m, L2_gpa, L0_gpa,
         p2m_ram_rw,
         p2m_access_rwx /* FIXME: Should use same permission as l1 guest */);
-    nestedp2m_unlock(d);
 
     return NESTEDHVM_PAGEFAULT_DONE;
 }
diff -r 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/mm-locks.h	Fri Jun 24 16:24:45 2011 +0100
@@ -96,8 +96,11 @@ declare_mm_lock(shr)
 
 /* Nested P2M lock (per-domain)
  *
- * A per-domain lock that protects some of the nested p2m datastructures.
- * TODO: find out exactly what needs to be covered by this lock */
+ * A per-domain lock that protects the mapping from nested-CR3 to 
+ * nested-p2m.  In particular it covers:
+ * - the array of nested-p2m tables, and all LRU activity therein; and
+ * - 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)
diff -r 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:45 2011 +0100
@@ -1052,7 +1052,7 @@ p2m_getlru_nestedp2m(struct domain *d, s
 
 /* Reset this p2m table to be empty */
 static void
-p2m_flush_locked(struct p2m_domain *p2m)
+p2m_flush_table(struct p2m_domain *p2m)
 {
     struct page_info *top, *pg;
     struct domain *d = p2m->domain;
@@ -1094,21 +1094,16 @@ p2m_flush(struct vcpu *v, struct p2m_dom
 
     ASSERT(v->domain == d);
     vcpu_nestedhvm(v).nv_p2m = NULL;
-    nestedp2m_lock(d);
-    p2m_flush_locked(p2m);
+    p2m_flush_table(p2m);
     hvm_asid_flush_vcpu(v);
-    nestedp2m_unlock(d);
 }
 
 void
 p2m_flush_nestedp2m(struct domain *d)
 {
     int i;
-
-    nestedp2m_lock(d);
     for ( i = 0; i < MAX_NESTEDP2M; i++ )
-        p2m_flush_locked(d->arch.nested_p2m[i]);
-    nestedp2m_unlock(d);
+        p2m_flush_table(d->arch.nested_p2m[i]);
 }
 
 struct p2m_domain *
@@ -1131,29 +1126,37 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     d = v->domain;
     nestedp2m_lock(d);
     p2m = nv->nv_p2m;
-    if ( p2m && (p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR) )
+    if ( p2m ) 
     {
-        nv->nv_flushp2m = 0;
-        p2m_getlru_nestedp2m(d, p2m);
-        nv->nv_p2m = p2m;
-        if (p2m->cr3 == CR3_EADDR)
-            hvm_asid_flush_vcpu(v);
-        p2m->cr3 = cr3;
-        cpu_set(v->processor, p2m->p2m_dirty_cpumask);
-        nestedp2m_unlock(d);
-        return p2m;
+        p2m_lock(p2m);
+        if ( p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR )
+        {
+            nv->nv_flushp2m = 0;
+            p2m_getlru_nestedp2m(d, p2m);
+            nv->nv_p2m = p2m;
+            if (p2m->cr3 == CR3_EADDR)
+                hvm_asid_flush_vcpu(v);
+            p2m->cr3 = cr3;
+            cpu_set(v->processor, p2m->p2m_dirty_cpumask);
+            p2m_unlock(p2m);
+            nestedp2m_unlock(d);
+            return p2m;
+        }
+        p2m_unlock(p2m);
     }
 
     /* All p2m''s are or were in use. Take the least recent used one,
      * flush it and reuse. */
     p2m = p2m_getlru_nestedp2m(d, NULL);
-    p2m_flush_locked(p2m);
+    p2m_flush_table(p2m);
+    p2m_lock(p2m);
     nv->nv_p2m = p2m;
     p2m->cr3 = cr3;
     nv->nv_flushp2m = 0;
     hvm_asid_flush_vcpu(v);
     nestedhvm_vmcx_flushtlb(nv->nv_p2m);
     cpu_set(v->processor, p2m->p2m_dirty_cpumask);
+    p2m_unlock(p2m);
     nestedp2m_unlock(d);
 
     return p2m;
diff -r 0753351afbbe -r c82cebcfec25 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/include/asm-x86/p2m.h	Fri Jun 24 16:24:45 2011 +0100
@@ -201,8 +201,13 @@ struct p2m_domain {
     cpumask_t          p2m_dirty_cpumask;
 
     struct domain     *domain;   /* back pointer to domain */
+
+    /* Nested p2ms only: nested-CR3 value that this p2m shadows. 
+     * This can be cleared to CR3_EADDR under the per-p2m lock but
+     * needs both the per-p2m lock and the per-domain nestedp2m lock
+     * to set it to any other value. */
 #define CR3_EADDR     (~0ULL)
-    uint64_t           cr3;      /* to identify this p2m for re-use */
+    uint64_t           cr3;
 
     /* Pages used to construct the p2m */
     struct page_list_head pages;

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

Tim Deegan

2011-Jun-27 10:56 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 11:46 +0100 on 27 Jun (1309175170), Tim Deegan wrote:> This patch series tidies up a few bits ofthe nested p2m code.
> The main thing it does is reorganize the locking so that most of the
> changes to nested p2m tables happen only under the p2m lock, and the
> nestedp2m lock is only needed to reassign p2m tables to new cr3 values.
There are still a few things I''m not convinced about in the nested NPT
code:

 - The function that allocates new nested p2ms probably needs an
   overhaul, as I said in my last email. 
 - The flushing policy is a bit confusing: e.g., what exactly ought to 
   happen when the guest sets the tlb-control bits?  AFAICS the nested-p2ms 
   are already kept in sync with host-p2m changes, and we flush all 
   TLBs when we update nested-p2ms, so can we skip this extra flush?
 - Why is there a 10x increase in IPIs after this series?  I don''t see
   what sequence of events sets the relevant cpumask bits to make this
   happen.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Christoph Egger

2011-Jun-27 12:23 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

On 06/27/11 12:56, Tim Deegan wrote:> At 11:46 +0100 on 27 Jun (1309175170), Tim Deegan wrote:
>> This patch series tidies up a few bits ofthe nested p2m code.
>> The main thing it does is reorganize the locking so that most of the
>> changes to nested p2m tables happen only under the p2m lock, and the
>> nestedp2m lock is only needed to reassign p2m tables to new cr3 values.
>
> There are still a few things I''m not convinced about in the nested
NPT
> code:
>
>   - The function that allocates new nested p2ms probably needs an
>     overhaul, as I said in my last email.
Ack.
>   - The flushing policy is a bit confusing: e.g., what exactly ought to
>     happen when the guest sets the tlb-control bits?  AFAICS the
nested-p2ms
>     are already kept in sync with host-p2m changes, and we flush all
>     TLBs when we update nested-p2ms, so can we skip this extra flush?
Yes, we can.
>   - Why is there a 10x increase in IPIs after this series?  I
don''t see
>     what sequence of events sets the relevant cpumask bits to make this
>     happen.
In patch 1 the code that sends the IPIs was outside of the loop and 
moved into the loop.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Tim Deegan

2011-Jun-27 13:15 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger
wrote:> >  - Why is there a 10x increase in IPIs after this series?  I
don''t see
> >    what sequence of events sets the relevant cpumask bits to make this
> >    happen.
> 
> In patch 1 the code that sends the IPIs was outside of the loop and
> moved into the loop.
Well, yes, but I don''t see what that causes 10x IPIs, unless the vcpus
are burning through np2m tables very quickly indeed.  Maybe removing the
extra flushes for TLB control will do the trick.  I''ll make a patch...

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Tim Deegan

2011-Jun-27 13:20 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > >  - Why is there a 10x increase in IPIs after this series?  I
don''t see
> > >    what sequence of events sets the relevant cpumask bits to make
this
> > >    happen.
> > 
> > In patch 1 the code that sends the IPIs was outside of the loop and
> > moved into the loop.
> 
> Well, yes, but I don''t see what that causes 10x IPIs, unless the
vcpus
> are burning through np2m tables very quickly indeed.  Maybe removing the
> extra flushes for TLB control will do the trick.  I''ll make a
patch...
Hmmm, on second thoughts, we can''t remove those flushes after all. 
The np2m is in sync with the p2m but not with the guest-supplied p2m, 
so we do need to flush it when the guest asks for that to happen.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Christoph Egger

2011-Jun-27 13:24 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

On 06/27/11 15:20, Tim Deegan wrote:> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I
don''t see
>>>>     what sequence of events sets the relevant cpumask bits to
make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don''t see what that causes 10x IPIs, unless
the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing
the
>> extra flushes for TLB control will do the trick.  I''ll make a
patch...
>
> Hmmm, on second thoughts, we can''t remove those flushes after all.
> The np2m is in sync with the p2m but not with the guest-supplied p2m,
> so we do need to flush it when the guest asks for that to happen.
Right. I thought on that when I developped the code but then I forgot.
We should add a comment.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Christoph Egger

2011-Jun-27 13:44 UTC

head link

[Xen-devel] Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

I think, this patch can be folded into the first one.
Otherwise:

Ack-by: Christoph Egger <Christoph.Egger@amd.com>

On 06/27/11 12:46, Tim Deegan wrote:> # HG changeset patch
> # User Tim Deegan<Tim.Deegan@citrix.com>
> # Date 1308929084 -3600
> # Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
> # Parent  97e15368260c093078e1f1bc04521de30c1792cc
> Nested p2m: flush only one p2m table when reallocating.
> It''s unhelpful to flush all of them when we only need one.
>
> Reported-by: Christoph Egger<Christoph.Egger@amd.com>
> Signed-off-by: Tim Deegan<Tim.Deegan@citrix.com>
>
> diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> @@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
>       volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
>       struct domain *d;
>       struct p2m_domain *p2m;
> -    int i;
>
>       /* Mask out low bits; this avoids collisions with CR3_EADDR */
>       cr3&= ~(0xfffull);
> @@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
>       }
>
>       /* All p2m''s are or were in use. Take the least recent used
one,
> -     * flush it and reuse.
> -     */
> -    for (i = 0; i<  MAX_NESTEDP2M; i++) {
> -        p2m = p2m_getlru_nestedp2m(d, NULL);
> -        p2m_flush_locked(p2m);
> -    }
> +     * flush it and reuse. */
> +    p2m = p2m_getlru_nestedp2m(d, NULL);
> +    p2m_flush_locked(p2m);
>       nv->nv_p2m = p2m;
>       p2m->cr3 = cr3;
>       nv->nv_flushp2m = 0;
>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Tim Deegan

2011-Jun-27 13:55 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 15:24 +0200 on 27 Jun (1309188245), Christoph Egger
wrote:> On 06/27/11 15:20, Tim Deegan wrote:
> >At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> >>At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> >>>>  - Why is there a 10x increase in IPIs after this series? 
I don''t see
> >>>>    what sequence of events sets the relevant cpumask bits
to make this
> >>>>    happen.
> >>>
> >>>In patch 1 the code that sends the IPIs was outside of the loop
and
> >>>moved into the loop.
> >>
> >>Well, yes, but I don''t see what that causes 10x IPIs,
unless the vcpus
> >>are burning through np2m tables very quickly indeed.  Maybe
removing the
> >>extra flushes for TLB control will do the trick.  I''ll
make a patch...
> >
> >Hmmm, on second thoughts, we can''t remove those flushes after
all.
> >The np2m is in sync with the p2m but not with the guest-supplied p2m,
> >so we do need to flush it when the guest asks for that to happen.
And futhermore we can''t share np2ms between vcpus because that could 
violate the TLB''s coherence rules. E.g.:
 - vcpu 1 uses ncr3 A, gets np2m A'', A'' is populated from A;
 - vcpu 1 switches to ncr3 B;
 - guest updates p2m @ A, knows there are no users so doesn''t flush it;
 - vcpu 2 uses ncr3 A, gets np2m A'' with stale data.

So in fact we have to have per-vcpu np2ms, unless we want a lot of
implicit flushes.  Which means we can discard the ''cr3'' field
in the
nested p2m.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Tim Deegan

2011-Jun-27 14:01 UTC

head link

Re: [Xen-devel] Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

At 15:44 +0200 on 27 Jun (1309189494), Christoph Egger
wrote:> 
> I think, this patch can be folded into the first one.
I don''t see which patch it would be folded into.  It''s a
distinct change
from the change to the flush implementation. 
> Otherwise:
> 
> Ack-by: Christoph Egger <Christoph.Egger@amd.com>
Thank you.  I''ll apply them all this afternoon.

Tim.
> On 06/27/11 12:46, Tim Deegan wrote:
> ># HG changeset patch
> ># User Tim Deegan<Tim.Deegan@citrix.com>
> ># Date 1308929084 -3600
> ># Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
> ># Parent  97e15368260c093078e1f1bc04521de30c1792cc
> >Nested p2m: flush only one p2m table when reallocating.
> >It''s unhelpful to flush all of them when we only need one.
> >
> >Reported-by: Christoph Egger<Christoph.Egger@amd.com>
> >Signed-off-by: Tim Deegan<Tim.Deegan@citrix.com>
> >
> >diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
> >--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> >+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> >@@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
> >      volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
> >      struct domain *d;
> >      struct p2m_domain *p2m;
> >-    int i;
> >
> >      /* Mask out low bits; this avoids collisions with CR3_EADDR */
> >      cr3&= ~(0xfffull);
> >@@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
> >      }
> >
> >      /* All p2m''s are or were in use. Take the least recent
used one,
> >-     * flush it and reuse.
> >-     */
> >-    for (i = 0; i<  MAX_NESTEDP2M; i++) {
> >-        p2m = p2m_getlru_nestedp2m(d, NULL);
> >-        p2m_flush_locked(p2m);
> >-    }
> >+     * flush it and reuse. */
> >+    p2m = p2m_getlru_nestedp2m(d, NULL);
> >+    p2m_flush_locked(p2m);
> >      nv->nv_p2m = p2m;
> >      p2m->cr3 = cr3;
> >      nv->nv_flushp2m = 0;
> >
> 
> 
> -- 
> ---to satisfy European Law for business letters:
> Advanced Micro Devices GmbH
> Einsteinring 24, 85689 Dornach b. Muenchen
> Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> Registergericht Muenchen, HRB Nr. 43632
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Tim Deegan

2011-Jun-27 15:48 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > >  - Why is there a 10x increase in IPIs after this series?  I
don''t see
> > >    what sequence of events sets the relevant cpumask bits to make
this
> > >    happen.
> > 
> > In patch 1 the code that sends the IPIs was outside of the loop and
> > moved into the loop.
> 
> Well, yes, but I don''t see what that causes 10x IPIs, unless the
vcpus
> are burning through np2m tables very quickly indeed.  Maybe removing the
> extra flushes for TLB control will do the trick.  I''ll make a
patch...
I think I get it - it''s a race between p2m_flush_nestedp2m() on one CPU
flushing all the nested P2M tables and a VCPU on another CPU repeatedly
getting fresh ones.  Try the attached patch, which should cut back the
major source of p2m_flush_nestedp2m() calls. 

Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
isn''t safe because it can run in parallel with p2m_get_nestedp2m, which
reorders the array it walks.  I''ll have to make the LRU-fu independent
of the array order; should be easy enough but I''ll hold off committing
the current series until I''ve done it. 

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)


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

Christoph Egger

2011-Jun-28 11:04 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

On 06/27/11 17:48, Tim Deegan wrote:> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I
don''t see
>>>>     what sequence of events sets the relevant cpumask bits to
make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don''t see what that causes 10x IPIs, unless
the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing
the
>> extra flushes for TLB control will do the trick.  I''ll make a
patch...
>
> I think I get it - it''s a race between p2m_flush_nestedp2m() on
one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls.
This patch is great! It gives the speed up the XP mode needs to continue 
booting. We talked about this at the Xen-Hackathon you remember?
> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn''t safe because it can run in parallel with p2m_get_nestedp2m,
which
> reorders the array it walks.  I''ll have to make the LRU-fu
independent
> of the array order; should be easy enough but I''ll hold off
committing
> the current series until I''ve done it.
Ok.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Christoph Egger

2011-Jun-28 13:47 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

On 06/27/11 17:48, Tim Deegan wrote:> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I
don''t see
>>>>     what sequence of events sets the relevant cpumask bits to
make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don''t see what that causes 10x IPIs, unless
the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing
the
>> extra flushes for TLB control will do the trick.  I''ll make a
patch...
>
> I think I get it - it''s a race between p2m_flush_nestedp2m() on
one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls.
>
> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn''t safe because it can run in parallel with p2m_get_nestedp2m,
which
> reorders the array it walks.  I''ll have to make the LRU-fu
independent
> of the array order; should be easy enough but I''ll hold off
committing
> the current series until I''ve done it.

With Windows 7 XP mode I see a xen crash where p2m_get_nestedp2m() calls
nestedhvm_vmcx_flushtlb(nv->nv_p2m) with nv->nv_p2m being NULL.

nestedhvm_vmcx_flushtlb() assumes the parameter is not NULL.

(XEN) Xen call trace:
(XEN)    [<ffff82c4801b1d19>] nestedhvm_vmcx_flushtlb+0xf/0x52
(XEN)    [<ffff82c4801d270c>] p2m_get_nestedp2m+0x406/0x497
(XEN)    [<ffff82c4801bad7a>] nestedsvm_vmcb_set_nestedp2m+0x54/0x6a
(XEN)    [<ffff82c4801bc602>] nsvm_vcpu_vmrun+0x984/0xf14
(XEN)    [<ffff82c4801bcd53>] nsvm_vcpu_switch+0x1c1/0x226

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Tim Deegan

2011-Jun-30 09:49 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

At 16:48 +0100 on 27 Jun (1309193311), Tim Deegan wrote:> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> > At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > > >  - Why is there a 10x increase in IPIs after this series?  I
don''t see
> > > >    what sequence of events sets the relevant cpumask bits to
make this
> > > >    happen.
> > > 
> > > In patch 1 the code that sends the IPIs was outside of the loop
and
> > > moved into the loop.
> > 
> > Well, yes, but I don''t see what that causes 10x IPIs, unless
the vcpus
> > are burning through np2m tables very quickly indeed.  Maybe removing
the
> > extra flushes for TLB control will do the trick.  I''ll make a
patch...
> 
> I think I get it - it''s a race between p2m_flush_nestedp2m() on
one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls. 
> 
> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn''t safe because it can run in parallel with p2m_get_nestedp2m,
which
> reorders the array it walks.  I''ll have to make the LRU-fu
independent
> of the array order; should be easy enough but I''ll hold off
committing
> the current series until I''ve done it. 
I''ve just pushed 23633 - 26369, which is this series plus the change to
the LRU code (and a fix to the NULL deref you reported is folded in).
Hopefully that puts nested SVM back in at least as good a state as it
was before my locking-order patch broke it! :)

Cheers,

Tim

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Olaf Hering

2011-Jun-30 09:51 UTC

head link

Re: [Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

On Mon, Jun 27, Tim Deegan wrote:
> diff -r b24018319772 -r 44c7b9773026 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Thu Jun 23 18:34:55 2011 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> @@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
>      return lrup2m;
>  }
>  
> -static int 
> +/* Reset this p2m table to be empty */
> +static void
>  p2m_flush_locked(struct p2m_domain *p2m)
>  {
> -    ASSERT(p2m);
> -    if (p2m->cr3 == CR3_EADDR)
> -        /* Microoptimisation: p2m is already empty.
> -         * => about 0.3% speedup of overall system performance.
> -         */
> -        return 0;
> +    struct page_info *top, *pg;
> +    struct domain *d = p2m->domain;
Tim,

one of these changes causes a build error:
p2m.c:1105:20: error: unused variable ''d''
[-Werror=unused-variable]

Olaf

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

Tim Deegan

2011-Jun-30 10:02 UTC

head link

Re: [Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

At 11:51 +0200 on 30 Jun (1309434665), Olaf Hering
wrote:> one of these changes causes a build error:
> p2m.c:1105:20: error: unused variable ''d''
[-Werror=unused-variable]
My apologies; I left a variable that''s only used in an ASSERT() and
didn''t check the non-debug build.   Fixed in 23640:4a1f7441095d.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Christoph Egger

2011-Jul-01 10:00 UTC

head link

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

On 06/30/11 11:49, Tim Deegan wrote:> At 16:48 +0100 on 27 Jun (1309193311), Tim Deegan wrote:
>> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>>   - Why is there a 10x increase in IPIs after this series? 
I don''t see
>>>>>     what sequence of events sets the relevant cpumask bits
to make this
>>>>>     happen.
>>>>
>>>> In patch 1 the code that sends the IPIs was outside of the loop
and
>>>> moved into the loop.
>>>
>>> Well, yes, but I don''t see what that causes 10x IPIs,
unless the vcpus
>>> are burning through np2m tables very quickly indeed.  Maybe
removing the
>>> extra flushes for TLB control will do the trick.  I''ll
make a patch...
>>
>> I think I get it - it''s a race between p2m_flush_nestedp2m()
on one CPU
>> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
>> getting fresh ones.  Try the attached patch, which should cut back the
>> major source of p2m_flush_nestedp2m() calls.
>>
>> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
>> isn''t safe because it can run in parallel with
p2m_get_nestedp2m, which
>> reorders the array it walks.  I''ll have to make the LRU-fu
independent
>> of the array order; should be easy enough but I''ll hold off
committing
>> the current series until I''ve done it.
>
> I''ve just pushed 23633 - 26369, which is this series plus the
change to
> the LRU code (and a fix to the NULL deref you reported is folded in).
> Hopefully that puts nested SVM back in at least as good a state as it
> was before my locking-order patch broke it! :)
I run some tests and nested SVM is now in an even better state as
it was before. Performance is a lot better now, particularly MMIO 
performance. The e1000 driver needed several minutes to read the
e1000 mac address before and now it takes less than a second.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Apparently Analagous Threads

Search for more maybe matching threads

Xen devel - Jun 2011 - [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

[Xen-devel] [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value

[Xen-devel] [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m()

[Xen-devel] [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

[Xen-devel] [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Xen-devel] Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Re: [Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

Re: [Xen-devel] [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

Re: [Xen-devel] [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

Apparently Analagous Threads