From: Theo Buehler <tb@openbsd.org>
Subject: Re: pgo_fault & VM_PAGER_* values
To: tech@openbsd.org, jsg@openbsd.org
Date: Wed, 11 Dec 2024 09:43:14 +0100

On Mon, Dec 09, 2024 at 11:38:53AM +0100, Martin Pieuchot wrote:
> On 05/12/24(Thu) 00:22, Theo Buehler wrote:
> > 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:
> > ...
> 
> Updated diff below that addresses both issues.

It would be nice if vm_fault_ttm() was a bit less special, but I
understand why you left it the way it was.

ok tb