On Thu, Dec 05, 2024 at 12:22:33AM +0100, 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__

In fault handlers, function arguments and various other things need to
be different to linux.  Sometimes comparing after unifdef helps.

> 
> 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.

We don't have a vm_fault_t typedef, so any code using vm_fault_t
is not used.

> 
> 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.

VM_FAULT_* are defines used by linux code
VM_PAGER_* are defines used by uvm

The case statements mapping between the two will be local changes.
Keeping the local changes small and consolidated is the goal.