[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH 14 of 17] x86/mm: Make MM locks recursive



# HG changeset patch
# User Tim Deegan <Tim.Deegan@xxxxxxxxxx>
# Date 1307017012 -3600
# Node ID 64398d14dcd6e720ac6908ee5ae284b03832e8bc
# Parent  d6518e8670ab15d5a9ec49b500ecf6e67442d3a8
x86/mm: Make MM locks recursive.

This replaces a lot of open coded 'if (!locked) {lock()}' instances
by making the mm locks recursive locks, but only allowing them
to be taken recursively in the places that they were before.

Signed-off-by: Tim Deegan <Tim.Deegan@xxxxxxxxxx>

diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/hap/hap.c Thu Jun 02 13:16:52 2011 +0100
@@ -277,13 +277,10 @@ static void hap_free(struct domain *d, m
 static struct page_info *hap_alloc_p2m_page(struct domain *d)
 {
     struct page_info *pg;
-    int do_locking;
 
     /* This is called both from the p2m code (which never holds the 
      * hap lock) and the log-dirty code (which sometimes does). */
-    do_locking = !hap_locked_by_me(d);
-    if ( do_locking )
-        hap_lock(d);
+    hap_lock_recursive(d);
     pg = hap_alloc(d);
 
 #if CONFIG_PAGING_LEVELS == 3
@@ -314,20 +311,15 @@ static struct page_info *hap_alloc_p2m_p
         pg->count_info |= 1;
     }
 
-    if ( do_locking )
-        hap_unlock(d);
+    hap_unlock(d);
     return pg;
 }
 
 static void hap_free_p2m_page(struct domain *d, struct page_info *pg)
 {
-    int do_locking;
-
     /* This is called both from the p2m code (which never holds the 
      * hap lock) and the log-dirty code (which sometimes does). */
-    do_locking = !hap_locked_by_me(d);
-    if ( do_locking )
-        hap_lock(d);
+    hap_lock_recursive(d);
 
     ASSERT(page_get_owner(pg) == d);
     /* Should have just the one ref we gave it in alloc_p2m_page() */
@@ -345,8 +337,7 @@ static void hap_free_p2m_page(struct dom
     hap_free(d, page_to_mfn(pg));
     ASSERT(d->arch.paging.hap.p2m_pages >= 0);
 
-    if ( do_locking )
-        hap_unlock(d);
+    hap_unlock(d);
 }
 
 /* Return the size of the pool, rounded up to the nearest MB */
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h        Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/mm-locks.h        Thu Jun 02 13:16:52 2011 +0100
@@ -37,42 +37,44 @@ static inline void mm_lock_init(mm_lock_
     l->unlock_level = 0;
 }
 
-static inline void _mm_lock(mm_lock_t *l, const char *func, int level)
+static inline int mm_locked_by_me(mm_lock_t *l) 
 {
-    if ( unlikely(l->locker == current->processor) )
-        panic("mm lock held by %s\n", l->locker_function);
+    return (l->lock.recurse_cpu == current->processor);
+}
+
+static inline void _mm_lock(mm_lock_t *l, const char *func, int level, int rec)
+{
     /* If you see this crash, the numbers printed are lines in this file 
      * where the offending locks are declared. */
-    if ( unlikely(this_cpu(mm_lock_level) >= level) )
-        panic("mm locking order violation: %i >= %i\n", 
+    if ( unlikely(this_cpu(mm_lock_level) > level) )
+        panic("mm locking order violation: %i > %i\n", 
               this_cpu(mm_lock_level), level);
-    spin_lock(&l->lock);
-    ASSERT(l->locker == -1);
-    l->locker = current->processor;
-    l->locker_function = func;
-    l->unlock_level = this_cpu(mm_lock_level);
+    spin_lock_recursive(&l->lock);
+    if ( l->lock.recurse_cnt == 1 )
+    {
+        l->locker_function = func;
+        l->unlock_level = this_cpu(mm_lock_level);
+    }
+    else if ( (unlikely(!rec)) )
+        panic("mm lock already held by %s\n", l->locker_function);
     this_cpu(mm_lock_level) = level;
 }
 /* This wrapper uses the line number to express the locking order below */
-#define declare_mm_lock(name)                                             \
-  static inline void mm_lock_##name(mm_lock_t *l, const char *func)       \
-  { _mm_lock(l, func, __LINE__); }
-/* This one captures the name of the calling function */
-#define mm_lock(name, l) mm_lock_##name(l, __func__)
+#define declare_mm_lock(name)                                                 \
+    static inline void mm_lock_##name(mm_lock_t *l, const char *func, int rec)\
+    { _mm_lock(l, func, __LINE__, rec); }
+/* These capture the name of the calling function */
+#define mm_lock(name, l) mm_lock_##name(l, __func__, 0)
+#define mm_lock_recursive(name, l) mm_lock_##name(l, __func__, 1)
 
 static inline void mm_unlock(mm_lock_t *l)
 {
-    ASSERT(l->locker == current->processor);
-    l->locker = -1;
-    l->locker_function = "nobody";
-    this_cpu(mm_lock_level) = l->unlock_level;
-    l->unlock_level = -1;
-    spin_unlock(&l->lock);
-}
-
-static inline int mm_locked_by_me(mm_lock_t *l) 
-{
-    return (current->processor == l->locker);
+    if ( l->lock.recurse_cnt == 1 )
+    {
+        l->locker_function = "nobody";
+        this_cpu(mm_lock_level) = l->unlock_level;
+    }
+    spin_unlock_recursive(&l->lock);
 }
 
 /************************************************************************
@@ -107,9 +109,10 @@ declare_mm_lock(nestedp2m)
  * 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_unlock(p)       mm_unlock(&(p)->lock)
-#define p2m_locked_by_me(p) mm_locked_by_me(&(p)->lock)
+#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)
 
 /* Shadow lock (per-domain)
  *
@@ -126,6 +129,8 @@ declare_mm_lock(p2m)
 
 declare_mm_lock(shadow)
 #define shadow_lock(d)         mm_lock(shadow, &(d)->arch.paging.shadow.lock)
+#define shadow_lock_recursive(d) \
+                     mm_lock_recursive(shadow, &(d)->arch.paging.shadow.lock)
 #define shadow_unlock(d)       mm_unlock(&(d)->arch.paging.shadow.lock)
 #define shadow_locked_by_me(d) mm_locked_by_me(&(d)->arch.paging.shadow.lock)
 
@@ -136,6 +141,8 @@ declare_mm_lock(shadow)
 
 declare_mm_lock(hap)
 #define hap_lock(d)         mm_lock(hap, &(d)->arch.paging.hap.lock)
+#define hap_lock_recursive(d) \
+                  mm_lock_recursive(hap, &(d)->arch.paging.hap.lock)
 #define hap_unlock(d)       mm_unlock(&(d)->arch.paging.hap.lock)
 #define hap_locked_by_me(d) mm_locked_by_me(&(d)->arch.paging.hap.lock)
 
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/p2m-ept.c Thu Jun 02 13:16:52 2011 +0100
@@ -47,26 +47,22 @@ static int ept_pod_check_and_populate(st
                                       ept_entry_t *entry, int order,
                                       p2m_query_t q)
 {
-    /* Only take the lock if we don't already have it.  Otherwise it
-     * wouldn't be safe to do p2m lookups with the p2m lock held */
-    int do_locking = !p2m_locked_by_me(p2m);
     int r;
 
-    if ( do_locking )
-        p2m_lock(p2m);
+    /* 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 )
     {
-        if ( do_locking )
-            p2m_unlock(p2m);
+        p2m_unlock(p2m);
         return 0;
     }
 
     r = p2m_pod_demand_populate(p2m, gfn, order, q);
 
-    if ( do_locking )
-        p2m_unlock(p2m);
+    p2m_unlock(p2m);
 
     return r;
 }
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/p2m-pt.c
--- a/xen/arch/x86/mm/p2m-pt.c  Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/p2m-pt.c  Thu Jun 02 13:16:52 2011 +0100
@@ -478,29 +478,24 @@ static int p2m_pod_check_and_populate(st
                                       l1_pgentry_t *p2m_entry, int order,
                                       p2m_query_t q)
 {
-    /* Only take the lock if we don't already have it.  Otherwise it
-     * wouldn't be safe to do p2m lookups with the p2m lock held */
-    int do_locking = !p2m_locked_by_me(p2m);
     int r;
 
-    if ( do_locking )
-        p2m_lock(p2m);
-
+    /* This is called from the p2m lookups, which can happen with or 
+     * without the lock hed. */
+    p2m_lock_recursive(p2m);
     audit_p2m(p2m, 1);
 
     /* Check to make sure this is still PoD */
     if ( p2m_flags_to_type(l1e_get_flags(*p2m_entry)) != 
p2m_populate_on_demand )
     {
-        if ( do_locking )
-            p2m_unlock(p2m);
+        p2m_unlock(p2m);
         return 0;
     }
 
     r = p2m_pod_demand_populate(p2m, gfn, order, q);
 
     audit_p2m(p2m, 1);
-    if ( do_locking )
-        p2m_unlock(p2m);
+    p2m_unlock(p2m);
 
     return r;
 }
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c   Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/shadow/common.c   Thu Jun 02 13:16:52 2011 +0100
@@ -906,7 +906,7 @@ static int sh_skip_sync(struct vcpu *v, 
  * will be made safe (TLB flush semantics); pages unsynced by this vcpu
  * are brought back into sync and write-protected.  If skip != 0, we try
  * to avoid resyncing at all if we think we can get away with it. */
-void sh_resync_all(struct vcpu *v, int skip, int this, int others, int 
do_locking)
+void sh_resync_all(struct vcpu *v, int skip, int this, int others)
 {
     int idx;
     struct vcpu *other;
@@ -916,14 +916,11 @@ void sh_resync_all(struct vcpu *v, int s
 
     SHADOW_PRINTK("d=%d, v=%d\n", v->domain->domain_id, v->vcpu_id);
 
-    ASSERT(do_locking || shadow_locked_by_me(v->domain));
+    ASSERT(shadow_locked_by_me(v->domain));
 
     if ( !this )
         goto resync_others;
 
-    if ( do_locking )
-        shadow_lock(v->domain);
-
     /* First: resync all of this vcpu's oos pages */
     for ( idx = 0; idx < SHADOW_OOS_PAGES; idx++ ) 
         if ( mfn_valid(oos[idx]) )
@@ -933,9 +930,6 @@ void sh_resync_all(struct vcpu *v, int s
             oos[idx] = _mfn(INVALID_MFN);
         }
 
-    if ( do_locking )
-        shadow_unlock(v->domain);
-
  resync_others:
     if ( !others )
         return;
@@ -946,9 +940,6 @@ void sh_resync_all(struct vcpu *v, int s
         if ( v == other ) 
             continue;
 
-        if ( do_locking )
-            shadow_lock(v->domain);
-
         oos = other->arch.paging.shadow.oos;
         oos_fixup = other->arch.paging.shadow.oos_fixup;
         oos_snapshot = other->arch.paging.shadow.oos_snapshot;
@@ -972,10 +963,7 @@ void sh_resync_all(struct vcpu *v, int s
                 _sh_resync(other, oos[idx], &oos_fixup[idx], 
oos_snapshot[idx]);
                 oos[idx] = _mfn(INVALID_MFN);
             }
-        }
-        
-        if ( do_locking )
-            shadow_unlock(v->domain);
+        }        
     }
 }
 
@@ -1623,19 +1611,15 @@ static struct page_info *
 shadow_alloc_p2m_page(struct domain *d)
 {
     struct page_info *pg;
-    int do_locking;
 
     /* This is called both from the p2m code (which never holds the 
      * shadow lock) and the log-dirty code (which sometimes does). */
-    do_locking = !shadow_locked_by_me(d);
-    if ( do_locking )
-        shadow_lock(d);
+    shadow_lock_recursive(d);
 
     if ( d->arch.paging.shadow.total_pages 
          < shadow_min_acceptable_pages(d) + 1 )
     {
-        if ( do_locking )
-            shadow_unlock(d);
+        shadow_unlock(d);
         return NULL;
     }
  
@@ -1644,8 +1628,7 @@ shadow_alloc_p2m_page(struct domain *d)
     d->arch.paging.shadow.p2m_pages++;
     d->arch.paging.shadow.total_pages--;
 
-    if ( do_locking )
-        shadow_unlock(d);
+    shadow_unlock(d);
 
     /* Unlike shadow pages, mark p2m pages as owned by the domain.
      * Marking the domain as the owner would normally allow the guest to
@@ -1660,8 +1643,6 @@ shadow_alloc_p2m_page(struct domain *d)
 static void
 shadow_free_p2m_page(struct domain *d, struct page_info *pg)
 {
-    int do_locking;
-
     ASSERT(page_get_owner(pg) == d);
     /* Should have just the one ref we gave it in alloc_p2m_page() */
     if ( (pg->count_info & PGC_count_mask) != 1 )
@@ -1675,16 +1656,13 @@ shadow_free_p2m_page(struct domain *d, s
 
     /* This is called both from the p2m code (which never holds the 
      * shadow lock) and the log-dirty code (which sometimes does). */
-    do_locking = !shadow_locked_by_me(d);
-    if ( do_locking )
-        shadow_lock(d);
+    shadow_lock_recursive(d);
 
     shadow_free(d, page_to_mfn(pg));
     d->arch.paging.shadow.p2m_pages--;
     d->arch.paging.shadow.total_pages++;
 
-    if ( do_locking )
-        shadow_unlock(d);
+    shadow_unlock(d);
 }
 
 #if CONFIG_PAGING_LEVELS == 3
@@ -2489,7 +2467,7 @@ int sh_remove_write_access_from_sl1p(str
 int sh_remove_all_mappings(struct vcpu *v, mfn_t gmfn)
 {
     struct page_info *page = mfn_to_page(gmfn);
-    int expected_count, do_locking;
+    int expected_count;
 
     /* Dispatch table for getting per-type functions */
     static const hash_callback_t callbacks[SH_type_unused] = {
@@ -2531,10 +2509,8 @@ int sh_remove_all_mappings(struct vcpu *
 
     /* Although this is an externally visible function, we do not know
      * whether the shadow lock will be held when it is called (since it
-     * can be called via put_page_type when we clear a shadow l1e).
-     * If the lock isn't held, take it for the duration of the call. */
-    do_locking = !shadow_locked_by_me(v->domain);
-    if ( do_locking ) shadow_lock(v->domain);
+     * can be called via put_page_type when we clear a shadow l1e).*/
+    shadow_lock_recursive(v->domain);
 
     /* XXX TODO: 
      * Heuristics for finding the (probably) single mapping of this gmfn */
@@ -2560,7 +2536,7 @@ int sh_remove_all_mappings(struct vcpu *
         }
     }
 
-    if ( do_locking ) shadow_unlock(v->domain);
+    shadow_unlock(v->domain);
 
     /* We killed at least one mapping, so must flush TLBs. */
     return 1;
@@ -2638,7 +2614,6 @@ void sh_remove_shadows(struct vcpu *v, m
 {
     struct page_info *pg = mfn_to_page(gmfn);
     mfn_t smfn;
-    int do_locking;
     unsigned char t;
     
     /* Dispatch table for getting per-type functions: each level must
@@ -2696,10 +2671,8 @@ void sh_remove_shadows(struct vcpu *v, m
 
     /* Although this is an externally visible function, we do not know
      * whether the shadow lock will be held when it is called (since it
-     * can be called via put_page_type when we clear a shadow l1e).
-     * If the lock isn't held, take it for the duration of the call. */
-    do_locking = !shadow_locked_by_me(v->domain);
-    if ( do_locking ) shadow_lock(v->domain);
+     * can be called via put_page_type when we clear a shadow l1e).*/
+    shadow_lock_recursive(v->domain);
 
     SHADOW_PRINTK("d=%d, v=%d, gmfn=%05lx\n",
                    v->domain->domain_id, v->vcpu_id, mfn_x(gmfn));
@@ -2707,7 +2680,7 @@ void sh_remove_shadows(struct vcpu *v, m
     /* Bail out now if the page is not shadowed */
     if ( (pg->count_info & PGC_page_table) == 0 )
     {
-        if ( do_locking ) shadow_unlock(v->domain);
+        shadow_unlock(v->domain);
         return;
     }
 
@@ -2769,7 +2742,7 @@ void sh_remove_shadows(struct vcpu *v, m
      * take a fault. */
     flush_tlb_mask(v->domain->domain_dirty_cpumask);
 
-    if ( do_locking ) shadow_unlock(v->domain);
+    shadow_unlock(v->domain);
 }
 
 static void
@@ -2907,7 +2880,7 @@ static void sh_update_paging_modes(struc
         /* Need to resync all our pages now, because if a page goes out
          * of sync with paging enabled and is resynced with paging
          * disabled, the resync will go wrong. */
-        shadow_resync_all(v, 0);
+        shadow_resync_all(v);
 #endif /* OOS */
 
         if ( !hvm_paging_enabled(v) )
@@ -3181,8 +3154,7 @@ void shadow_teardown(struct domain *d)
     ASSERT(d->is_dying);
     ASSERT(d != current->domain);
 
-    if ( !shadow_locked_by_me(d) )
-        shadow_lock(d); /* Keep various asserts happy */
+    shadow_lock(d);
 
     if ( shadow_mode_enabled(d) )
     {
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c    Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/shadow/multi.c    Thu Jun 02 13:16:52 2011 +0100
@@ -1979,7 +1979,7 @@ static shadow_l1e_t * shadow_get_and_cre
     /* All pages walked are now pagetables. Safe to resync pages
        in case level 4 or 3 shadows were set. */
     if ( resync )
-        shadow_resync_all(v, 0);
+        shadow_resync_all(v);
 #endif
 
     /* Now follow it down a level.  Guaranteed to succeed. */
@@ -2273,7 +2273,7 @@ static int validate_gl4e(struct vcpu *v,
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC )
         if ( mfn_valid(sl3mfn) )
-            shadow_resync_all(v, 0);
+            shadow_resync_all(v);
 #endif
     }
     l4e_propagate_from_guest(v, new_gl4e, sl3mfn, &new_sl4e, ft_prefetch);
@@ -2330,7 +2330,7 @@ static int validate_gl3e(struct vcpu *v,
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC )
         if ( mfn_valid(sl2mfn) )
-            shadow_resync_all(v, 0);
+            shadow_resync_all(v);
 #endif
     }
     l3e_propagate_from_guest(v, new_gl3e, sl2mfn, &new_sl3e, ft_prefetch);
@@ -4172,15 +4172,15 @@ sh_update_cr3(struct vcpu *v, int do_loc
         return;
     }
 
+    if ( do_locking ) shadow_lock(v->domain);
+
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     /* Need to resync all the shadow entries on a TLB flush.  Resync
      * current vcpus OOS pages before switching to the new shadow
      * tables so that the VA hint is still valid.  */
-    shadow_resync_current_vcpu(v, do_locking);
+    shadow_resync_current_vcpu(v);
 #endif
 
-    if ( do_locking ) shadow_lock(v->domain);
-
     ASSERT(shadow_locked_by_me(v->domain));
     ASSERT(v->arch.paging.mode);
 
@@ -4406,17 +4406,16 @@ sh_update_cr3(struct vcpu *v, int do_loc
     v->arch.paging.last_write_emul_ok = 0;
 #endif
 
-    /* Release the lock, if we took it (otherwise it's the caller's problem) */
-    if ( do_locking ) shadow_unlock(v->domain);
-
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     /* Need to resync all the shadow entries on a TLB flush. We only
      * update the shadows, leaving the pages out of sync. Also, we try
      * to skip synchronization of shadows not mapped in the new
      * tables. */
-    shadow_sync_other_vcpus(v, do_locking);
+    shadow_sync_other_vcpus(v);
 #endif
 
+    /* Release the lock, if we took it (otherwise it's the caller's problem) */
+    if ( do_locking ) shadow_unlock(v->domain);
 }
 
 
diff -r d6518e8670ab -r 64398d14dcd6 xen/arch/x86/mm/shadow/private.h
--- a/xen/arch/x86/mm/shadow/private.h  Thu Jun 02 13:16:52 2011 +0100
+++ b/xen/arch/x86/mm/shadow/private.h  Thu Jun 02 13:16:52 2011 +0100
@@ -388,36 +388,24 @@ int sh_remove_write_access_from_sl1p(str
 /* Pull all out-of-sync shadows back into sync.  If skip != 0, we try
  * to avoid resyncing where we think we can get away with it. */
 
-void sh_resync_all(struct vcpu *v, int skip, int this, int others, int 
do_locking);
+void sh_resync_all(struct vcpu *v, int skip, int this, int others);
 
 static inline void
-shadow_resync_all(struct vcpu *v, int do_locking)
+shadow_resync_all(struct vcpu *v)
 {
-    sh_resync_all(v,
-                  0 /* skip */,
-                  1 /* this */,
-                  1 /* others */,
-                  do_locking);
+    sh_resync_all(v, 0 /* skip */, 1 /* this */, 1 /* others */);
 }
 
 static inline void
-shadow_resync_current_vcpu(struct vcpu *v, int do_locking)
+shadow_resync_current_vcpu(struct vcpu *v)
 {
-    sh_resync_all(v,
-                  0 /* skip */,
-                  1 /* this */, 
-                  0 /* others */,
-                  do_locking);
+    sh_resync_all(v, 0 /* skip */, 1 /* this */, 0 /* others */);
 }
 
 static inline void
-shadow_sync_other_vcpus(struct vcpu *v, int do_locking)
+shadow_sync_other_vcpus(struct vcpu *v)
 {
-    sh_resync_all(v,
-                  1 /* skip */, 
-                  0 /* this */,
-                  1 /* others */,
-                  do_locking);
+    sh_resync_all(v, 1 /* skip */, 0 /* this */, 1 /* others */);
 }
 
 void oos_audit_hash_is_present(struct domain *d, mfn_t gmfn);

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.