revert "try to simplify the locking code around busy maps"

anton@ and syzkaller have trouble with it.

2 files changed, 60 insertions, 57 deletions

diff --git a/sys/uvm/uvm_map.c b/sys/uvm/uvm_map.c
index 9de86fa2a7e..02e2c0fa0f5 100644
--- a/sys/uvm/uvm_map.c
+++ b/sys/uvm/uvm_map.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uvm_map.c,v 1.331 2024/10/20 11:28:17 dlg Exp $	*/
+/*	$OpenBSD: uvm_map.c,v 1.332 2024/10/21 06:07:33 dlg Exp $	*/
 /*	$NetBSD: uvm_map.c,v 1.86 2000/11/27 08:40:03 chs Exp $	*/
 
 /*
@@ -5201,77 +5201,68 @@ out:
 boolean_t
 vm_map_lock_try_ln(struct vm_map *map, char *file, int line)
 {
-	int rv;
+	boolean_t rv;
 
 	if (map->flags & VM_MAP_INTRSAFE) {
-		if (!mtx_enter_try(&map->mtx))
-			return FALSE;
+		rv = mtx_enter_try(&map->mtx);
 	} else {
-		struct proc *busy;
-
-		mtx_enter(&map->flags_lock);
-		busy = map->busy;
-		mtx_leave(&map->flags_lock);
-		if (busy != NULL && busy != curproc)
-			return FALSE;
-
-		rv = rw_enter(&map->lock, RW_WRITE|RW_NOSLEEP);
-		if (rv != 0)
-			return FALSE;
-
-		/* to be sure, to be sure */
 		mtx_enter(&map->flags_lock);
-		busy = map->busy;
+		if ((map->flags & VM_MAP_BUSY) && (map->busy != curproc)) {
+			mtx_leave(&map->flags_lock);
+			return (FALSE);
+		}
 		mtx_leave(&map->flags_lock);
-		if (busy != NULL && busy != curproc) {
-			rw_exit(&map->lock);
-			return FALSE;
+		rv = (rw_enter(&map->lock, RW_WRITE|RW_NOSLEEP) == 0);
+		/* check if the lock is busy and back out if we won the race */
+		if (rv) {
+			mtx_enter(&map->flags_lock);
+			if ((map->flags & VM_MAP_BUSY) &&
+			    (map->busy != curproc)) {
+				rw_exit(&map->lock);
+				rv = FALSE;
+			}
+			mtx_leave(&map->flags_lock);
 		}
 	}
 
-	map->timestamp++;
-	LPRINTF(("map   lock: %p (at %s %d)\n", map, file, line));
-	uvm_tree_sanity(map, file, line);
-	uvm_tree_size_chk(map, file, line);
+	if (rv) {
+		map->timestamp++;
+		LPRINTF(("map   lock: %p (at %s %d)\n", map, file, line));
+		uvm_tree_sanity(map, file, line);
+		uvm_tree_size_chk(map, file, line);
+	}
 
-	return TRUE;
+	return (rv);
 }
 
 void
 vm_map_lock_ln(struct vm_map *map, char *file, int line)
 {
 	if ((map->flags & VM_MAP_INTRSAFE) == 0) {
-		mtx_enter(&map->flags_lock);
-		for (;;) {
-			while (map->busy != NULL && map->busy != curproc) {
-				map->nbusy++;
-				msleep_nsec(&map->busy, &map->mtx,
+		do {
+			mtx_enter(&map->flags_lock);
+tryagain:
+			while ((map->flags & VM_MAP_BUSY) &&
+			    (map->busy != curproc)) {
+				map->flags |= VM_MAP_WANTLOCK;
+				msleep_nsec(&map->flags, &map->flags_lock,
 				    PVM, vmmapbsy, INFSLP);
-				map->nbusy--;
 			}
 			mtx_leave(&map->flags_lock);
-
-			rw_enter_write(&map->lock);
-
-			/* to be sure, to be sure */
-			mtx_enter(&map->flags_lock);
-			if (map->busy != NULL && map->busy != curproc) {
-				/* go around again */
-				rw_exit_write(&map->lock);
-			} else {
-				/* we won */
-				break;
-			}
+		} while (rw_enter(&map->lock, RW_WRITE|RW_SLEEPFAIL) != 0);
+		/* check if the lock is busy and back out if we won the race */
+		mtx_enter(&map->flags_lock);
+		if ((map->flags & VM_MAP_BUSY) && (map->busy != curproc)) {
+			rw_exit(&map->lock);
+			goto tryagain;
 		}
 		mtx_leave(&map->flags_lock);
 	} else {
 		mtx_enter(&map->mtx);
 	}
 
-	if (map->busy != curproc) {
-		KASSERT(map->busy == NULL);
+	if (map->busy != curproc)
 		map->timestamp++;
-	}
 	LPRINTF(("map   lock: %p (at %s %d)\n", map, file, line));
 	uvm_tree_sanity(map, file, line);
 	uvm_tree_size_chk(map, file, line);
@@ -5323,24 +5314,25 @@ vm_map_busy_ln(struct vm_map *map, char *file, int line)
 
 	mtx_enter(&map->flags_lock);
 	map->busy = curproc;
+	map->flags |= VM_MAP_BUSY;
 	mtx_leave(&map->flags_lock);
 }
 
 void
 vm_map_unbusy_ln(struct vm_map *map, char *file, int line)
 {
-	unsigned int nbusy;
+	int oflags;
 
 	KASSERT((map->flags & VM_MAP_INTRSAFE) == 0);
 	KASSERT(map->busy == curproc);
 
 	mtx_enter(&map->flags_lock);
-	nbusy = map->nbusy;
+	oflags = map->flags;
 	map->busy = NULL;
+	map->flags &= ~(VM_MAP_BUSY|VM_MAP_WANTLOCK);
 	mtx_leave(&map->flags_lock);
-
-	if (nbusy > 0)
-		wakeup(&map->busy);
+	if (oflags & VM_MAP_WANTLOCK)
+		wakeup(&map->flags);
 }
 
 void
diff --git a/sys/uvm/uvm_map.h b/sys/uvm/uvm_map.h
index 9f8775f74ea..0c98020d9d2 100644
--- a/sys/uvm/uvm_map.h
+++ b/sys/uvm/uvm_map.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uvm_map.h,v 1.91 2024/10/20 11:28:17 dlg Exp $	*/
+/*	$OpenBSD: uvm_map.h,v 1.92 2024/10/21 06:07:33 dlg Exp $	*/
 /*	$NetBSD: uvm_map.h,v 1.24 2001/02/18 21:19:08 chs Exp $	*/
 
 /*
@@ -214,6 +214,17 @@ RBT_PROTOTYPE(uvm_map_addr, vm_map_entry, daddrs.addr_entry,
  *					map is write-locked.  may be tested
  *					without asserting `flags_lock'.
  *
+ *		VM_MAP_BUSY		r/w; may only be set when map is
+ *					write-locked, may only be cleared by
+ *					thread which set it, map read-locked
+ *					or write-locked.  must be tested
+ *					while `flags_lock' is asserted.
+ *
+ *		VM_MAP_WANTLOCK		r/w; may only be set when the map
+ *					is busy, and thread is attempting
+ *					to write-lock.  must be tested
+ *					while `flags_lock' is asserted.
+ *
  *		VM_MAP_GUARDPAGES	r/o; must be specified at map
  *					initialization time.
  *					If set, guards will appear between
@@ -246,7 +257,6 @@ RBT_PROTOTYPE(uvm_map_addr, vm_map_entry, daddrs.addr_entry,
  *	a	atomic operations
  *	I	immutable after creation or exec(2)
  *	v	`vm_map_lock' (this map `lock' or `mtx')
- *	f	flags_lock
  */
 struct vm_map {
 	struct pmap		*pmap;		/* [I] Physical map */
@@ -256,10 +266,9 @@ struct vm_map {
 
 	vsize_t			size;		/* virtual size */
 	int			ref_count;	/* [a] Reference count */
-	int			flags;		/* [f] flags */
+	int			flags;		/* flags */
 	unsigned int		timestamp;	/* Version number */
-	struct proc		*busy;		/* [f] thread holding map busy*/
-	unsigned int		nbusy;		/* [f] waiters for busy */
+	struct proc		*busy;		/* [v] thread holding map busy*/
 
 	vaddr_t			min_offset;	/* [I] First address in map. */
 	vaddr_t			max_offset;	/* [I] Last address in map. */
@@ -314,6 +323,8 @@ struct vm_map {
 #define	VM_MAP_PAGEABLE		0x01		/* ro: entries are pageable */
 #define	VM_MAP_INTRSAFE		0x02		/* ro: interrupt safe map */
 #define	VM_MAP_WIREFUTURE	0x04		/* rw: wire future mappings */
+#define	VM_MAP_BUSY		0x08		/* rw: map is busy */
+#define	VM_MAP_WANTLOCK		0x10		/* rw: want to write-lock */
 #define	VM_MAP_GUARDPAGES	0x20		/* rw: add guard pgs to map */
 #define	VM_MAP_ISVMSPACE	0x40		/* ro: map is a vmspace */
 #define	VM_MAP_PINSYSCALL_ONCE	0x100		/* rw: pinsyscall done */