From: Theo Buehler <tb@openbsd.org>
Subject: Re: pgo_fault & VM_PAGER_* values
To: tech@openbsd.org, jsg@openbsd.org
Date: Thu, 5 Dec 2024 00:22:33 +0100

On Wed, Dec 04, 2024 at 10:52:59AM +0100, Martin Pieuchot wrote:
> Similar to the previous change in uvmfault_anonget(), the diff below
> gets rid of the VM_PAGER_* values and use errno values instead in all
> the pgo_fault() handlers.
> 
> The goal of this change is to reduce the differences with NetBSD's UVM
> and then to return different error codes.
> 
> I've annotated return values that are currently EACCES but could be
> something else.
> 
> While here grab the KERNEL_LOCK() after the UVM object lock to reduce
> contention.

This generally looks good to me - for a while I thought the diff was
incomplete since I looked at the wrong version of some functions under
#ifdef __linux__

I dislike that in a few handlers the vm_fault_t ret changes from holding
a VM_* code to a type holding an errno. But that's C being the terrible
type unsafe language that it is.

In udv_fault() there is a retval = VM_PAGER_OK that should be retval = 0.

In ttm_bo_vm_fault() you deduplicated the return value handling of
ttm_bo_vm_fault() with the exit path, which you didn't do in the other
functions. I would either undo that or do it for all the functions, e.g.,
in amdgpu_gem_fault() do

unlock:
	dma_resv_unlock(bo->base.resv);
out:
	switch (ret) {
	case VM_FAULT_NOPAGE:
...

I would probably go the dedup route if it was my code, but perhaps that's
an inconsistency justified by reducing the diff to an upstream.