Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 6/23/2021 2:50 PM, Christian König wrote:


Am 23.06.21 um 14:25 schrieb Nirmoy Das:
Replace idr with xarray as we actually need hash functionality.
Cleanup code related to vm pasid by adding helper function.

Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 +++++++++++--------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
  2 files changed, 60 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index be841d62a1d4..e047e56a4be2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
      struct dma_fence_cb cb;
  };
  +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
+                   struct amdgpu_vm *vm,
+                   unsigned long pasid)

Some kerneldoc please describing why we have that function.


Alright.



+{
+    unsigned long flags;
+    int r;
+
+    if (pasid) {

You should probably reorder the code so that the old pasid is first removed and then eventually the new one added.

+ xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
+                      GFP_ATOMIC));
+        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);

As far as I can see this can just use xa_store() which also drops the need for GFP_ATOMIC here.


Do we need to have this irqsave/restore to keep passids safe for amdgpu_vm_handle_fault() ?

xa_store() takes the spinlock without irq flags so I wanted to keep old behavior.



+        if (r < 0)
+            return r;
+    } else {
+        unsigned long index;
+        struct amdgpu_vm *res;
+
+        xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+        xa_for_each(&adev->vm_manager.pasids, index, res) {
+            if (res == vm) {
+                __xa_erase(&adev->vm_manager.pasids, index);
+                break;
+            }
+        }
+        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);

That is really ugly, why is that necessary?

Do you mean the lock around xa_for_each() ? I think I can just used lock around __xa_erase() or just xa_erase() if just simple spinlock without flags is enough.


Regards,

Nirmoy



Christian

+    }
+
+    vm->pasid = pasid;
+
+    return 0;
+}
+
  /*
   * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
   * happens while holding this lock anywhere to prevent deadlocks when
@@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
        amdgpu_bo_unreserve(vm->root.bo);
  -    if (pasid) {
-        unsigned long flags;
-
-        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-                  GFP_ATOMIC);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-        if (r < 0)
-            goto error_free_root;
-
-        vm->pasid = pasid;
-    }
+    r = amdgpu_vm_set_pasid(adev, vm, pasid);
+    if (r)
+        goto error_free_root;
        INIT_KFIFO(vm->faults);
  @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
      if (r)
          goto unreserve_bo;
  -    if (pasid) {
-        unsigned long flags;
-
-        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-                  GFP_ATOMIC);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-
-        if (r == -ENOSPC)
-            goto unreserve_bo;
-        r = 0;
-    }
+    /* remove previous {pasid:vm} entry first */
+    r = amdgpu_vm_set_pasid(adev, vm, 0);
+    r = amdgpu_vm_set_pasid(adev, vm, pasid);
+    if (r)
+        goto unreserve_bo;
        /* Check if PD needs to be reinitialized and do it before
       * changing any other state, in case it fails.
@@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
                         to_amdgpu_bo_vm(vm->root.bo),
                         false);
          if (r)
-            goto free_idr;
+            goto free_pasid_entry;
      }
        /* Update VM state */
@@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
          r = amdgpu_bo_sync_wait(vm->root.bo,
                      AMDGPU_FENCE_OWNER_UNDEFINED, true);
          if (r)
-            goto free_idr;
+            goto free_pasid_entry;
            vm->update_funcs = &amdgpu_vm_cpu_funcs;
      } else {
@@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
      vm->last_update = NULL;
      vm->is_compute_context = true;
  -    if (vm->pasid) {
-        unsigned long flags;
-
-        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-        vm->pasid = 0;
-    }
-
      /* Free the shadow bo for compute VM */
amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
  -    if (pasid)
-        vm->pasid = pasid;
-
      goto unreserve_bo;
  -free_idr:
-    if (pasid) {
-        unsigned long flags;
+free_pasid_entry:
+    amdgpu_vm_set_pasid(adev, vm, 0);
  - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        idr_remove(&adev->vm_manager.pasid_idr, pasid);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-    }
  unreserve_bo:
      amdgpu_bo_unreserve(vm->root.bo);
      return r;
@@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
   */
  void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
  {
-    if (vm->pasid) {
-        unsigned long flags;
-
-        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-    }
-    vm->pasid = 0;
+    amdgpu_vm_set_pasid(adev, vm, 0);
      vm->is_compute_context = false;
  }
  @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
        root = amdgpu_bo_ref(vm->root.bo);
      amdgpu_bo_reserve(root, true);
-    if (vm->pasid) {
-        unsigned long flags;
-
-        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-        vm->pasid = 0;
-    }
-
+    amdgpu_vm_set_pasid(adev, vm, 0);
      dma_fence_wait(vm->last_unlocked, false);
      dma_fence_put(vm->last_unlocked);
  @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
      adev->vm_manager.vm_update_mode = 0;
  #endif
  -    idr_init(&adev->vm_manager.pasid_idr);
-    spin_lock_init(&adev->vm_manager.pasid_lock);
+    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
  }
    /**
@@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
   */
  void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
  {
-    WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
-    idr_destroy(&adev->vm_manager.pasid_idr);
+    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
+    xa_destroy(&adev->vm_manager.pasids);
        amdgpu_vmid_mgr_fini(adev);
  }
@@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct amdgpu_device *adev, u32 pasid,
      struct amdgpu_vm *vm;
      unsigned long flags;
  -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
+    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
  -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+    vm = xa_load(&adev->vm_manager.pasids, pasid);
      if (vm)
          *task_info = vm->task_info;
  - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
+    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
  }
    /**
@@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
      struct amdgpu_vm *vm;
      int r;
  -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
-    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+    vm = xa_load(&adev->vm_manager.pasids, pasid);
      if (vm) {
          root = amdgpu_bo_ref(vm->root.bo);
          is_compute_context = vm->is_compute_context;
      } else {
          root = NULL;
      }
-    spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
        if (!root)
          return false;
@@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
          goto error_unref;
        /* Double check that the VM still exists */
-    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
-    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+    vm = xa_load(&adev->vm_manager.pasids, pasid);
      if (vm && vm->root.bo != root)
          vm = NULL;
-    spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
      if (!vm)
          goto error_unlock;
  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index ddb85a85cbba..31c467764162 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
      /* PASID to VM mapping, will be used in interrupt context to
       * look up VM of a page fault
       */
-    struct idr                pasid_idr;
-    spinlock_t                pasid_lock;
+    struct xarray                pasids;
  };
    struct amdgpu_bo_va_mapping;

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux