From: Martin Pieuchot Subject: Re: pgo_fault & VM_PAGER_* values To: Theo Buehler Cc: tech@openbsd.org, jsg@openbsd.org Date: Mon, 9 Dec 2024 11:38:53 +0100 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. ok? Index: dev/pci/drm/drm_gem.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/drm_gem.c,v diff -u -p -r1.23 drm_gem.c --- dev/pci/drm/drm_gem.c 16 Jan 2024 23:37:51 -0000 1.23 +++ dev/pci/drm/drm_gem.c 9 Dec 2024 09:56:24 -0000 @@ -102,10 +102,9 @@ drm_fault(struct uvm_faultinfo *ufi, vad * we do not allow device mappings to be mapped copy-on-write * so we kill any attempt to do so here. */ - if (UVM_ET_ISCOPYONWRITE(entry)) { uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj); - return(VM_PAGER_ERROR); + return EACCES; } /* @@ -125,7 +124,7 @@ drm_fault(struct uvm_faultinfo *ufi, vad PZERO, "drmflt", INFSLP); } mtx_leave(&dev->quiesce_mtx); - return(VM_PAGER_REFAULT); + return ERESTART; } dev->quiesce_count++; mtx_leave(&dev->quiesce_mtx); @@ -141,7 +140,7 @@ drm_fault(struct uvm_faultinfo *ufi, vad wakeup(&dev->quiesce_count); mtx_leave(&dev->quiesce_mtx); - return (ret); + return ret; } boolean_t Index: dev/pci/drm/drm_gem_dma_helper.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/drm_gem_dma_helper.c,v diff -u -p -r1.4 drm_gem_dma_helper.c --- dev/pci/drm/drm_gem_dma_helper.c 10 Nov 2024 06:51:59 -0000 1.4 +++ dev/pci/drm/drm_gem_dma_helper.c 9 Dec 2024 09:56:24 -0000 @@ -208,7 +208,7 @@ drm_gem_dma_fault(struct drm_gem_object paddr = bus_dmamem_mmap(obj->dmat, obj->dmasegs, 1, offset, access_type, BUS_DMA_NOCACHE); if (paddr == -1) { - retval = VM_PAGER_BAD; + retval = EACCES; break; } @@ -218,7 +218,7 @@ drm_gem_dma_fault(struct drm_gem_object uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj); uvm_wait("drm_gem_dma_fault"); - return VM_PAGER_REFAULT; + return ERESTART; } } Index: dev/pci/drm/amd/amdgpu/amdgpu_gem.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/amd/amdgpu/amdgpu_gem.c,v diff -u -p -r1.10 amdgpu_gem.c --- dev/pci/drm/amd/amdgpu/amdgpu_gem.c 16 Jan 2024 23:37:52 -0000 1.10 +++ dev/pci/drm/amd/amdgpu/amdgpu_gem.c 9 Dec 2024 09:59:49 -0000 @@ -99,19 +99,7 @@ amdgpu_gem_fault(struct uvm_faultinfo *u ret = ttm_bo_vm_reserve(bo); if (ret) { - switch (ret) { - case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; - break; - case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; - break; - default: - ret = VM_PAGER_BAD; - break; - } - uvmfault_unlockall(ufi, NULL, uobj); - return ret; + goto out; } if (drm_dev_enter(ddev, &idx)) { @@ -137,18 +125,19 @@ amdgpu_gem_fault(struct uvm_faultinfo *u #endif unlock: + dma_resv_unlock(bo->base.resv); +out: switch (ret) { case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; + ret = 0; break; case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; + ret = ERESTART; break; default: - ret = VM_PAGER_BAD; + ret = EACCES; break; } - dma_resv_unlock(bo->base.resv); uvmfault_unlockall(ufi, NULL, uobj); return ret; } Index: dev/pci/drm/i915/gem/i915_gem_mman.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/i915/gem/i915_gem_mman.c,v diff -u -p -r1.20 i915_gem_mman.c --- dev/pci/drm/i915/gem/i915_gem_mman.c 19 Aug 2024 11:18:29 -0000 1.20 +++ dev/pci/drm/i915/gem/i915_gem_mman.c 9 Dec 2024 09:56:24 -0000 @@ -591,10 +591,10 @@ static int i915_error_to_vmf_fault(int e case -EFAULT: /* purged object */ case -ENODEV: /* bad object, how did you get here! */ case -ENXIO: /* unable to access backing store (on device) */ - return VM_PAGER_ERROR; + return EACCES; case -ENOMEM: /* our allocation failure */ - return VM_PAGER_ERROR; + return EACCES; /* XXX */ case 0: case -EAGAIN: @@ -607,7 +607,7 @@ static int i915_error_to_vmf_fault(int e * EBUSY is ok: this just means that another thread * already did the job. */ - return VM_PAGER_OK; + return 0; } } @@ -629,11 +629,11 @@ vm_fault_cpu(struct i915_mmap_offset *mm /* Sanity check that we allow writing into this object */ if (unlikely(i915_gem_object_is_readonly(obj) && write)) { uvmfault_unlockall(ufi, NULL, &obj->base.uobj); - return VM_PAGER_BAD; + return EACCES; } if (i915_gem_object_lock_interruptible(obj, NULL)) - return VM_PAGER_ERROR; + return EACCES; err = i915_gem_object_pin_pages(obj); if (err) @@ -921,7 +921,7 @@ i915_gem_fault(struct drm_gem_object *ge drm_vma_offset_unlock_lookup(dev->vma_offset_manager); if (!mmo) { uvmfault_unlockall(ufi, NULL, &gem_obj->uobj); - return VM_PAGER_BAD; + return EACCES; } KASSERT(gem_obj == &mmo->obj->base); Index: dev/pci/drm/i915/gem/i915_gem_ttm.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/i915/gem/i915_gem_ttm.c,v diff -u -p -r1.8 i915_gem_ttm.c --- dev/pci/drm/i915/gem/i915_gem_ttm.c 11 Oct 2024 02:48:48 -0000 1.8 +++ dev/pci/drm/i915/gem/i915_gem_ttm.c 9 Dec 2024 09:56:24 -0000 @@ -1240,20 +1240,20 @@ vm_fault_ttm(struct uvm_faultinfo *ufi, /* Sanity check that we allow writing into this object */ if (unlikely(i915_gem_object_is_readonly(obj) && write)) { uvmfault_unlockall(ufi, NULL, &obj->base.uobj); - return VM_PAGER_BAD; + return EACCES; } ret = ttm_bo_vm_reserve(bo); if (ret) { switch (ret) { case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; + ret = 0; break; case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; + ret = ERESTART; break; default: - ret = VM_PAGER_BAD; + ret = EACCES; break; } uvmfault_unlockall(ufi, NULL, &obj->base.uobj); @@ -1263,7 +1263,7 @@ vm_fault_ttm(struct uvm_faultinfo *ufi, if (obj->mm.madv != I915_MADV_WILLNEED) { dma_resv_unlock(bo->base.resv); uvmfault_unlockall(ufi, NULL, &obj->base.uobj); - return VM_PAGER_BAD; + return EACCES; } /* @@ -1285,7 +1285,7 @@ vm_fault_ttm(struct uvm_faultinfo *ufi, if (err) { dma_resv_unlock(bo->base.resv); uvmfault_unlockall(ufi, NULL, &obj->base.uobj); - return VM_PAGER_BAD; + return EACCES; } } else if (!i915_ttm_resource_mappable(bo->resource)) { int err = -ENODEV; @@ -1359,13 +1359,13 @@ vm_fault_ttm(struct uvm_faultinfo *ufi, out_rpm: switch (ret) { case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; + ret = 0; break; case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; + ret = ERESTART; break; default: - ret = VM_PAGER_BAD; + ret = EACCES; break; } Index: dev/pci/drm/radeon/radeon_gem.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/radeon/radeon_gem.c,v diff -u -p -r1.19 radeon_gem.c --- dev/pci/drm/radeon/radeon_gem.c 26 Jul 2024 03:42:02 -0000 1.19 +++ dev/pci/drm/radeon/radeon_gem.c 9 Dec 2024 09:56:24 -0000 @@ -115,13 +115,13 @@ unlock_resv: unlock_mclk: switch (ret) { case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; + ret = 0; break; case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; + ret = ERESTART; break; default: - ret = VM_PAGER_BAD; + ret = EACCES; break; } up_read(&rdev->pm.mclk_lock); Index: dev/pci/drm/ttm/ttm_bo_vm.c =================================================================== RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_vm.c,v diff -u -p -r1.32 ttm_bo_vm.c --- dev/pci/drm/ttm/ttm_bo_vm.c 16 Jan 2024 23:38:14 -0000 1.32 +++ dev/pci/drm/ttm/ttm_bo_vm.c 9 Dec 2024 09:56:24 -0000 @@ -598,37 +598,23 @@ ttm_bo_vm_fault(struct uvm_faultinfo *uf ret = ttm_bo_vm_reserve(bo); if (ret) { - switch (ret) { - case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; - break; - case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; - break; - default: - ret = VM_PAGER_BAD; - break; - } - - uvmfault_unlockall(ufi, NULL, uobj); - return ret; + goto out; } ret = ttm_bo_vm_fault_reserved(ufi, vaddr, TTM_BO_VM_NUM_PREFAULT, 1); + dma_resv_unlock(bo->base.resv); +out: switch (ret) { case VM_FAULT_NOPAGE: - ret = VM_PAGER_OK; + ret = 0; break; case VM_FAULT_RETRY: - ret = VM_PAGER_REFAULT; + ret = ERESTART; break; default: - ret = VM_PAGER_BAD; + ret = EACCES; break; } - - dma_resv_unlock(bo->base.resv); - uvmfault_unlockall(ufi, NULL, uobj); return ret; } Index: uvm/uvm_device.c =================================================================== RCS file: /cvs/src/sys/uvm/uvm_device.c,v diff -u -p -r1.67 uvm_device.c --- uvm/uvm_device.c 24 Jul 2024 12:15:55 -0000 1.67 +++ uvm/uvm_device.c 9 Dec 2024 09:57:12 -0000 @@ -331,7 +331,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad */ if (UVM_ET_ISCOPYONWRITE(entry)) { uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj); - return(VM_PAGER_ERROR); + return EACCES; } /* @@ -354,7 +354,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad /* * loop over the page range entering in as needed */ - retval = VM_PAGER_OK; + retval = 0; for (lcv = 0 ; lcv < npages ; lcv++, curr_offset += PAGE_SIZE, curr_va += PAGE_SIZE) { if ((flags & PGO_ALLPAGES) == 0 && lcv != centeridx) @@ -365,7 +365,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad paddr = (*mapfn)(device, curr_offset, access_type); if (paddr == -1) { - retval = VM_PAGER_ERROR; + retval = EACCES; /* XXX */ break; } mapprot = ufi->entry->protection; @@ -387,11 +387,11 @@ udv_fault(struct uvm_faultinfo *ufi, vad /* sync what we have so far */ pmap_update(ufi->orig_map->pmap); uvm_wait("udv_fault"); - return (VM_PAGER_REFAULT); + return ERESTART; } } uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj); pmap_update(ufi->orig_map->pmap); - return (retval); + return retval; } Index: uvm/uvm_fault.c =================================================================== RCS file: /cvs/src/sys/uvm/uvm_fault.c,v diff -u -p -r1.152 uvm_fault.c --- uvm/uvm_fault.c 4 Dec 2024 09:21:06 -0000 1.152 +++ uvm/uvm_fault.c 9 Dec 2024 09:56:24 -0000 @@ -621,20 +621,13 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad * providing a pgo_fault routine. */ if (uobj != NULL && uobj->pgops->pgo_fault != NULL) { - KERNEL_LOCK(); rw_enter(uobj->vmobjlock, RW_WRITE); + KERNEL_LOCK(); error = uobj->pgops->pgo_fault(&ufi, flt.startva, pages, flt.npages, flt.centeridx, fault_type, flt.access_type, PGO_LOCKED); KERNEL_UNLOCK(); - - if (error == VM_PAGER_OK) - error = 0; - else if (error == VM_PAGER_REFAULT) - error = ERESTART; - else - error = EACCES; } else { /* case 2: fault on backing obj or zero fill */ error = uvm_fault_lower(&ufi, &flt, pages);