Synchronized p2m lookups

 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.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/hap/hap.c

xen/arch/x86/mm/hap/hap.c +1 -0

     paging_lock(d);
 
     v->arch.paging.mode = hap_paging_get_mode(v);

@@ -803,6 +808,7 @@ static void hap_update_paging_modes(stru
     hap_update_cr3(v, 0);
 
     paging_unlock(d);
+    put_gfn(d, cr3_gfn);
 }
 
 #if CONFIG_PAGING_LEVELS == 3

diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/mm-locks.h

xen/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/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.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

 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.

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]>

 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.

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]>

 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]>

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_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

Quoted 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.

Are you aware of any inversions or are you just suggesting there might be some left?

Quoted 15 lines

HAP (EPT) lookups and all modifications do take the lock.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/hap/hap.c

xen/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;
 

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?

Quoted 47 lines

xen/arch/x86/mm/hap/hap.c +1 -0

     paging_lock(d);
 
     v->arch.paging.mode = hap_paging_get_mode(v);

@@ -803,6 +808,7 @@ static void hap_update_paging_modes(stru
     hap_update_cr3(v, 0);
 
     paging_unlock(d);
+    put_gfn(d, cr3_gfn);
 }
 
 #if CONFIG_PAGING_LEVELS == 3

diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/mm-locks.h

xen/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 4 lines

/* 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.

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;
 

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 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);

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.

xen/arch/x86/mm/p2m-pod.c +1 -2

@@ -114,15 +114,20 @@ p2m_pod_cache_add(struct p2m_domain *p2m
         unmap_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 +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_domai
     if ( 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_domai
     int 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 +10 -8

@@ -1012,8 +1032,6 @@ p2m_pod_demand_populate(struct p2m_domai
     if ( 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_domai
 
     BUG_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_domai
         paging_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 -r 56ceab0118cb xen/arch/x86/mm/p2m-pt.c

xen/arch/x86/mm/p2m-pt.c +1 -0

@@ -954,6 +954,7 @@ long p2m_pt_audit_p2m(struct p2m_domain
     struct 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 -r 56ceab0118cb xen/arch/x86/mm/p2m.c

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");

diff -r fb9c06df05f2 -r 56ceab0118cb xen/include/asm-x86/p2m.h

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;
 };
 

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, uint64
             nestedp2m_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:

Acked-by George Dunlap <[email protected]>

Quoted 14 lines

xen/arch/x86/mm/p2m-pod.c +1 -2

@@ -114,15 +114,20 @@ p2m_pod_cache_add(struct p2m_domain *p2m
         unmap_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);
     }
 

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 be

freed. */

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().

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 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_domai
     if ( 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_domai
     int 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) )

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 an

scenario
+ * 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 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);
     }

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_domai
     if ( 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_domai
 
     BUG_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_domai
         paging_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 -r 56ceab0118cb xen/arch/x86/mm/p2m-pt.c

xen/arch/x86/mm/p2m-pt.c +1 -0

@@ -954,6 +954,7 @@ long p2m_pt_audit_p2m(struct p2m_domain
     struct 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 -r 56ceab0118cb xen/arch/x86/mm/p2m.c

xen/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 -r 56ceab0118cb xen/include/asm-x86/p2m.h

xen/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
conjunction

xen/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_t lock; /* Locking of private pod
structs, *
+ * not relying on the p2m lock.

    */
     } pod;

};