From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: uvmfault_anonget: return values
To: tech@openbsd.org, tb@openbsd.org
Date: Mon, 25 Nov 2024 16:19:54 +0100

On 25/11/24(Mon) 14:56, Martin Pieuchot wrote:
> Diff below changes the uvmfault_anonget() return value to avoid a
> conversion.  The idea is to return errno(2) values that are, then,
> returned by uvm_fault().  This reduces the diff with NetBSD and is
> necessary for upcoming changes.
> 
> This diff doesn't include any behavior change.  We could later return
> EIO instead of EACCES when appropriate.  But for the moment I'm
> concerned about reducing the amount of VM_PAGER_* usages.

I've been pointed out off-list that the diff was incomplete.  Updated
diff below with the corresponding uvm/uvm_anon.c change.

Index: uvm/uvm_anon.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_anon.c,v
diff -u -p -r1.58 uvm_anon.c
--- uvm/uvm_anon.c	6 Apr 2024 10:59:52 -0000	1.58
+++ uvm/uvm_anon.c	25 Nov 2024 15:17:14 -0000
@@ -170,20 +170,17 @@ uvm_anon_pagein(struct vm_amap *amap, st
 	rv = uvmfault_anonget(NULL, amap, anon);
 
 	switch (rv) {
-	case VM_PAGER_OK:
+	case 0:
+		/* Success - we have the page. */
 		KASSERT(rw_write_held(anon->an_lock));
 		break;
-
-	case VM_PAGER_ERROR:
-	case VM_PAGER_REFAULT:
-
+	case EACCES:
+	case ERESTART:
 		/*
-		 * Nothing more to do on errors.
-		 * VM_PAGER_REFAULT  means that the anon was freed.
+		 * Nothing more to do on errors.  ERESTART means that the
+		 * anon was freed.
 		 */
-
 		return FALSE;
-
 	default:
 #ifdef DIAGNOSTIC
 		panic("anon_pagein: uvmfault_anonget -> %d", rv);
Index: uvm/uvm_fault.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
diff -u -p -r1.144 uvm_fault.c
--- uvm/uvm_fault.c	25 Nov 2024 13:46:55 -0000	1.144
+++ uvm/uvm_fault.c	25 Nov 2024 15:17:14 -0000
@@ -315,7 +315,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
 			 * try again.
 			 */
 			if ((pg->pg_flags & (PG_BUSY|PG_RELEASED)) == 0)
-				return (VM_PAGER_OK);
+				return 0;
 			atomic_setbits_int(&pg->pg_flags, PG_WANTED);
 			counters_inc(uvmexp_counters, flt_pgwait);
 
@@ -404,7 +404,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
 					uvmfault_unlockall(ufi, NULL, NULL);
 				uvm_anon_release(anon);	/* frees page for us */
 				counters_inc(uvmexp_counters, flt_pgrele);
-				return (VM_PAGER_REFAULT);	/* refault! */
+				return ERESTART;	/* refault! */
 			}
 
 			if (error != VM_PAGER_OK) {
@@ -435,7 +435,12 @@ uvmfault_anonget(struct uvm_faultinfo *u
 					uvmfault_unlockall(ufi, NULL, NULL);
 				}
 				rw_exit(anon->an_lock);
-				return (VM_PAGER_ERROR);
+				/*
+				 * An error occurred while trying to bring
+				 * in the page -- this is the only error we
+				 * return right now.
+				 */
+				return EACCES;	/* XXX */
 			}
 
 			/*
@@ -457,7 +462,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
 			if (we_own) {
 				rw_exit(anon->an_lock);
 			}
-			return (VM_PAGER_REFAULT);
+			return ERESTART;
 		}
 
 		/*
@@ -468,7 +473,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
 				ufi->orig_rvaddr - ufi->entry->start) != anon) {
 
 			uvmfault_unlockall(ufi, amap, NULL);
-			return (VM_PAGER_REFAULT);
+			return ERESTART;
 		}
 
 		/*
@@ -942,25 +947,14 @@ uvm_fault_upper(struct uvm_faultinfo *uf
 	 */
 	error = uvmfault_anonget(ufi, amap, anon);
 	switch (error) {
-	case VM_PAGER_OK:
+	case 0:
 		break;
 
-	case VM_PAGER_REFAULT:
+	case ERESTART:
 		return ERESTART;
 
-	case VM_PAGER_ERROR:
-		/*
-		 * An error occurred while trying to bring in the
-		 * page -- this is the only error we return right
-		 * now.
-		 */
-		return EACCES;	/* XXX */
 	default:
-#ifdef DIAGNOSTIC
-		panic("uvm_fault: uvmfault_anonget -> %d", error);
-#else
-		return EACCES;
-#endif
+		return error;
 	}
 
 	KASSERT(rw_write_held(amap->am_lock));