Implement a variant locking scheme for vm maps: Access to system maps

is now synchronized by a mutex, whereas access to user maps is still
synchronized by a lockmgr()-based lock.  Why?  No single type of lock,
including sx locks, meets the requirements of both types of vm map.
Sometimes we sleep while holding the lock on a user map.  Thus, a
a mutex isn't appropriate.  On the other hand, both lockmgr()-based
and sx locks release Giant when a thread/process blocks during
contention for a lock.  This could lead to a race condition in a legacy
driver (that relies on Giant for synchronization) if it attempts to
kmem_malloc() and fails to immediately obtain the lock.  Fortunately,
we never sleep while holding a system map lock.

2 files changed, 39 insertions, 16 deletions

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 2b4c667..9b863de 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -197,7 +197,7 @@ vm_map_zfini(void *mem, int size)
 	vm_map_t map;
 
 	map = (vm_map_t)mem;
-
+	mtx_destroy(&map->system_mtx);
 	lockdestroy(&map->lock);
 }
 
@@ -210,6 +210,7 @@ vm_map_zinit(void *mem, int size)
 	map->nentries = 0;
 	map->size = 0;
 	map->infork = 0;
+	mtx_init(&map->system_mtx, "system map", NULL, MTX_DEF);
 	lockinit(&map->lock, PVM, "thrd_sleep", 0, LK_NOPAUSE);
 }
 
@@ -375,9 +376,11 @@ _vm_map_lock(vm_map_t map, const char *file, int line)
 	int error;
 
 	if (map->system_map)
-		GIANT_REQUIRED;
-	error = lockmgr(&map->lock, LK_EXCLUSIVE, NULL, curthread);
-	KASSERT(error == 0, ("%s: failed to get lock", __func__));
+		_mtx_lock_flags(&map->system_mtx, 0, file, line);
+	else {
+		error = lockmgr(&map->lock, LK_EXCLUSIVE, NULL, curthread);
+		KASSERT(error == 0, ("%s: failed to get lock", __func__));
+	}
 	map->timestamp++;
 }
 
@@ -385,7 +388,10 @@ void
 _vm_map_unlock(vm_map_t map, const char *file, int line)
 {
 
-	lockmgr(&map->lock, LK_RELEASE, NULL, curthread);
+	if (map->system_map)
+		_mtx_unlock_flags(&map->system_mtx, 0, file, line);
+	else
+		lockmgr(&map->lock, LK_RELEASE, NULL, curthread);
 }
 
 void
@@ -394,16 +400,21 @@ _vm_map_lock_read(vm_map_t map, const char *file, int line)
 	int error;
 
 	if (map->system_map)
-		GIANT_REQUIRED;
-	error = lockmgr(&map->lock, LK_EXCLUSIVE, NULL, curthread);
-	KASSERT(error == 0, ("%s: failed to get lock", __func__));
+		_mtx_lock_flags(&map->system_mtx, 0, file, line);
+	else {
+		error = lockmgr(&map->lock, LK_EXCLUSIVE, NULL, curthread);
+		KASSERT(error == 0, ("%s: failed to get lock", __func__));
+	}
 }
 
 void
 _vm_map_unlock_read(vm_map_t map, const char *file, int line)
 {
 
-	lockmgr(&map->lock, LK_RELEASE, NULL, curthread);
+	if (map->system_map)
+		_mtx_unlock_flags(&map->system_mtx, 0, file, line);
+	else
+		lockmgr(&map->lock, LK_RELEASE, NULL, curthread);
 }
 
 int
@@ -411,9 +422,9 @@ _vm_map_trylock(vm_map_t map, const char *file, int line)
 {
 	int error;
 
-	if (map->system_map)
-		GIANT_REQUIRED;
-	error = lockmgr(&map->lock, LK_EXCLUSIVE | LK_NOWAIT, NULL, curthread);
+	error = map->system_map ?
+	    !_mtx_trylock(&map->system_mtx, 0, file, line) :
+	    lockmgr(&map->lock, LK_EXCLUSIVE | LK_NOWAIT, NULL, curthread);
 	if (error == 0)
 		map->timestamp++;
 	return (error == 0);
@@ -423,8 +434,13 @@ int
 _vm_map_lock_upgrade(vm_map_t map, const char *file, int line)
 {
 
-	KASSERT(lockstatus(&map->lock, curthread) == LK_EXCLUSIVE,
-		("%s: lock not held", __func__));
+	if (map->system_map) {
+#ifdef INVARIANTS
+		_mtx_assert(&map->system_mtx, MA_OWNED, file, line);
+#endif
+	} else
+		KASSERT(lockstatus(&map->lock, curthread) == LK_EXCLUSIVE,
+		    ("%s: lock not held", __func__));
 	map->timestamp++;
 	return (0);
 }
@@ -433,8 +449,13 @@ void
 _vm_map_lock_downgrade(vm_map_t map, const char *file, int line)
 {
 
-	KASSERT(lockstatus(&map->lock, curthread) == LK_EXCLUSIVE,
-		("%s: lock not held", __func__));
+	if (map->system_map) {
+#ifdef INVARIANTS
+		_mtx_assert(&map->system_mtx, MA_OWNED, file, line);
+#endif
+	} else
+		KASSERT(lockstatus(&map->lock, curthread) == LK_EXCLUSIVE,
+		    ("%s: lock not held", __func__));
 }
 
 /*
@@ -514,6 +535,7 @@ void
 vm_map_init(vm_map_t map, vm_offset_t min, vm_offset_t max)
 {
 	_vm_map_init(map, min, max);
+	mtx_init(&map->system_mtx, "system map", NULL, MTX_DEF);
 	lockinit(&map->lock, PVM, "thrd_sleep", 0, LK_NOPAUSE);
 }
 
diff --git a/sys/vm/vm_map.h b/sys/vm/vm_map.h
index 2f629c6..d95d197 100644
--- a/sys/vm/vm_map.h
+++ b/sys/vm/vm_map.h
@@ -164,6 +164,7 @@ vm_map_entry_behavior(vm_map_entry_t entry)
 struct vm_map {
 	struct vm_map_entry header;	/* List of entries */
 	struct lock lock;		/* Lock for map data */
+	struct mtx system_mtx;
 	int nentries;			/* Number of entries */
 	vm_size_t size;			/* virtual size */
 	u_char needs_wakeup;