WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [PATCH 2 of 5] Fix up the synchronisation around grant table

To: <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] [PATCH 2 of 5] Fix up the synchronisation around grant table map track handles
From: <steven.smith@xxxxxxxxxxxxx>
Date: Tue, 19 May 2009 12:27:04 +0100
Delivery-date: Tue, 19 May 2009 04:26:32 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <patchbomb.1242732422@xxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User Steven Smith <steven.smith@xxxxxxxxxxxxx>
# Date 1242731493 -3600
# Node ID 5c6214b1f6003bfb0db95ad8f02a1665a98a21ba
# Parent  1954d5a3a1f5977b6d22c545cdc352bad27af528
Fix up the synchronisation around grant table map track handles.
At present, we're not doing any at all, so if a domain e.g. tries to
do two map operations at the same time from different vcpus then you
could end up with both operations getting back the same maptrack
handle.

Fix this problem by just shoving an enormous lock around grant table
operations.  This is unlikely to be heavily contended, because netback
and blkback both restrict themselves to mapping on a single vcpu at a
time (globally for netback, and per-device for blkback), and most of
the interesting bits are already protected by the remote domain's
grant table lock anyway.

The unconteded acquisition cost might be significant for some
workloads.  If that were the case, it might be worth only acquiring
the lock only for multi-vcpu domains, since we only manipulate the
maptrack table in the context of one of the domain's vcpus.  I've not
done that optimisation here, because I didn't want to think about what
would happen if e.g. a cpu got hot-unplugged from a domain while it
was performing a map operation.

Signed-off-by: Steven Smith <steven.smith@xxxxxxxxxx>

diff -r 1954d5a3a1f5 -r 5c6214b1f600 xen/common/grant_table.c
--- a/xen/common/grant_table.c  Tue May 19 12:11:33 2009 +0100
+++ b/xen/common/grant_table.c  Tue May 19 12:11:33 2009 +0100
@@ -111,6 +111,26 @@
 #define active_entry(t, e) \
     ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])
 
+/* Technically, we only really need to acquire the lock for SMP
+   guests, because you only ever touch the maptrack tables from the
+   context of the guest which owns them, so if it's uniproc then the
+   lock can't be contended, and is therefore pointless.  Don't bother
+   with that optimisation for now, though, because it's scary and
+   confusing. */
+/* The maptrack lock is top-level: you're not allowed to be holding
+   any other locks when you acquire it. */
+static void
+maptrack_lock(struct grant_table *lgt)
+{
+    spin_lock(&lgt->maptrack_lock);
+}
+
+static void
+maptrack_unlock(struct grant_table *lgt)
+{
+    spin_unlock(&lgt->maptrack_lock);
+}
+
 static inline int
 __get_maptrack_handle(
     struct grant_table *t)
@@ -141,43 +161,30 @@
 
     if ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
     {
-        spin_lock(&lgt->lock);
+        nr_frames = nr_maptrack_frames(lgt);
+        if ( nr_frames >= max_nr_maptrack_frames() )
+            return -1;
 
-        if ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
+        new_mt = alloc_xenheap_page();
+        if ( new_mt == NULL )
+            return -1;
+
+        clear_page(new_mt);
+
+        new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
+
+        for ( i = lgt->maptrack_limit; i < new_mt_limit; i++ )
         {
-            nr_frames = nr_maptrack_frames(lgt);
-            if ( nr_frames >= max_nr_maptrack_frames() )
-            {
-                spin_unlock(&lgt->lock);
-                return -1;
-            }
-
-            new_mt = alloc_xenheap_page();
-            if ( new_mt == NULL )
-            {
-                spin_unlock(&lgt->lock);
-                return -1;
-            }
-
-            clear_page(new_mt);
-
-            new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
-
-            for ( i = lgt->maptrack_limit; i < new_mt_limit; i++ )
-            {
-                new_mt[i % MAPTRACK_PER_PAGE].ref = i+1;
-                new_mt[i % MAPTRACK_PER_PAGE].flags = 0;
-            }
-
-            lgt->maptrack[nr_frames] = new_mt;
-            lgt->maptrack_limit      = new_mt_limit;
-
-            gdprintk(XENLOG_INFO,
-                    "Increased maptrack size to %u frames.\n", nr_frames + 1);
-            handle = __get_maptrack_handle(lgt);
+            new_mt[i % MAPTRACK_PER_PAGE].ref = i+1;
+            new_mt[i % MAPTRACK_PER_PAGE].flags = 0;
         }
 
-        spin_unlock(&lgt->lock);
+        lgt->maptrack[nr_frames] = new_mt;
+        lgt->maptrack_limit      = new_mt_limit;
+
+        gdprintk(XENLOG_INFO,
+                 "Increased maptrack size to %u frames.\n", nr_frames + 1);
+        handle = __get_maptrack_handle(lgt);
     }
     return handle;
 }
@@ -1506,7 +1513,9 @@
             guest_handle_cast(uop, gnttab_map_grant_ref_t);
         if ( unlikely(!guest_handle_okay(map, count)) )
             goto out;
+        maptrack_lock(current->domain->grant_table);
         rc = gnttab_map_grant_ref(map, count);
+        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_unmap_grant_ref:
@@ -1515,7 +1524,9 @@
             guest_handle_cast(uop, gnttab_unmap_grant_ref_t);
         if ( unlikely(!guest_handle_okay(unmap, count)) )
             goto out;
+        maptrack_lock(current->domain->grant_table);
         rc = gnttab_unmap_grant_ref(unmap, count);
+        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_unmap_and_replace:
@@ -1527,7 +1538,9 @@
         rc = -ENOSYS;
         if ( unlikely(!replace_grant_supported()) )
             goto out;
+        maptrack_lock(current->domain->grant_table);
         rc = gnttab_unmap_and_replace(unmap, count);
+        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_setup_table:
@@ -1598,6 +1611,7 @@
     /* Simple stuff. */
     memset(t, 0, sizeof(*t));
     spin_lock_init(&t->lock);
+    spin_lock_init(&t->maptrack_lock);
     t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
 
     /* Active grant table. */
@@ -1678,6 +1692,7 @@
 
     for ( handle = 0; handle < gt->maptrack_limit; handle++ )
     {
+        /* Domain is dying, so don't need maptrack lock */
         map = &maptrack_entry(gt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
             continue;
diff -r 1954d5a3a1f5 -r 5c6214b1f600 xen/include/xen/grant_table.h
--- a/xen/include/xen/grant_table.h     Tue May 19 12:11:33 2009 +0100
+++ b/xen/include/xen/grant_table.h     Tue May 19 12:11:33 2009 +0100
@@ -91,6 +91,8 @@
     struct grant_mapping **maptrack;
     unsigned int          maptrack_head;
     unsigned int          maptrack_limit;
+    /* Lock protecting maptrack-related fields. */
+    spinlock_t            maptrack_lock;
     /* Lock protecting updates to active and shared grant tables. */
     spinlock_t            lock;
 };
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>