On 8/13/26 17:16, Huang, Honglei wrote:
...
So, if I understand your point correctly, the best way to solve the
serialization issue between these two locks is to turn them into a single
lock. Given that a full re-design of amdgpu_vm could take a significant
amount of time, it seems that using drm_gpusvm's notifier_lock as a
replacement for eviction_lock in amdgpu_vm would be the more practical
short-term solution.
Please correct me if I've misunderstood your position.
Thanks,
Ray
Hi Christian,
I made some changes based on your modification to unify the locking,
only use the notifier lock when svm is enabled,
instead of using notifier lock and eviciton lock at the same time.
see diff below, not sure if it is correct, so confirming with you：
static inline int amdgpu_vm_begin_critical(struct
amdgpu_vm_update_params *p)
{
- mutex_lock(&p->vm->eviction_lock);
+ struct amdgpu_vm *vm = p->vm;
+
+ if (vm->svm)
+ down_read(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_lock(&vm->eviction_lock);
p->saved_flags = memalloc_noreclaim_save();
- if (p->vm->evicting)
+ if (vm->evicting)
return -EBUSY;
if (p->hmm_range && !amdgpu_hmm_range_valid(p->hmm_range))
return -EAGAIN;
@@ -174,8 +181,13 @@ static inline int amdgpu_vm_begin_critical(struct
amdgpu_vm_update_params *p)
*/
static inline void amdgpu_vm_end_critical(struct
amdgpu_vm_update_params *p)
{
+ struct amdgpu_vm *vm = p->vm;
+
memalloc_noreclaim_restore(p->saved_flags);
- mutex_unlock(&p->vm->eviction_lock);
+ if (vm->svm)
+ up_read(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_unlock(&vm->eviction_lock);
}
if above is valid, I have a question that
some places still not using begin/end critical, using vm->eviction_lock
directly, do thoes places need to be changed?
amdgpu_vm_evictable():
scoped_cond_guard(mutex_try, return false, &vm->eviction_lock)
amdgpu_vm_validate():
scoped_guard(mutex, &vm->eviction_lock)
amdgpu_vm_ready()
scoped_guard(mutex, &vm->eviction_lock)
Regards,
Honglei
Hi Christian,
Following up on my previous mail, I changed the change. The diff to the
existing VM code is below.
Does this look correct to you? for unify notifier lock and eviction lock.
diff:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
@@ -28,6 +28,7 @@
#include "amdgpu_hmm.h"
#include "amdgpu_vm.h"
+#include "amdgpu_svm.h"
@@ -66,6 +67,9 @@ struct amdgpu_vm_update_params {
bool unlocked;
+ /** @svm_locked: caller already holds the drm_gpusvm notifier_lock */
+ bool svm_locked;
+
/**
* @pages_addr:
@@ -143,6 +147,30 @@
+/* SVM VMs serialize eviction under the notifier_lock (write); others
use eviction_lock. */
+static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
+{
+ if (amdgpu_svm_is_enabled(vm))
+ down_write(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_lock(&vm->eviction_lock);
+}
+
+static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
+{
+ if (amdgpu_svm_is_enabled(vm))
+ up_write(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_unlock(&vm->eviction_lock);
+}
+
+static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
+{
+ if (amdgpu_svm_is_enabled(vm))
+ return down_write_trylock(&vm->svm->gpusvm.notifier_lock);
+ return mutex_trylock(&vm->eviction_lock);
+}
+
static inline int amdgpu_vm_begin_critical(struct
amdgpu_vm_update_params *p)
{
+ if (amdgpu_svm_is_enabled(p->vm)) {
+ if (p->svm_locked)
+ lockdep_assert_held(&p->vm->svm->gpusvm.notifier_lock);
+ else
+ down_read(&p->vm->svm->gpusvm.notifier_lock);
+ p->saved_flags = memalloc_noreclaim_save();
+ if (p->vm->evicting)
+ return -EBUSY;
+ return 0;
+ }
+
mutex_lock(&p->vm->eviction_lock);
p->saved_flags = memalloc_noreclaim_save();
if (p->vm->evicting)
@@ static inline void amdgpu_vm_end_critical(struct
amdgpu_vm_update_params *p)
memalloc_noreclaim_restore(p->saved_flags);
+ if (amdgpu_svm_is_enabled(p->vm)) {
+ if (!p->svm_locked)
+ up_read(&p->vm->svm->gpusvm.notifier_lock);
+ return;
+ }
mutex_unlock(&p->vm->eviction_lock);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ amdgpu_vm_validate():
- scoped_guard(mutex, &vm->eviction_lock)
- vm->evicting = false;
+ amdgpu_vm_eviction_lock(vm);
+ vm->evicting = false;
+ amdgpu_vm_eviction_unlock(vm);
@@ amdgpu_vm_ready():
- scoped_guard(mutex, &vm->eviction_lock)
- ret = !vm->evicting;
+ amdgpu_vm_eviction_lock(vm);
+ ret = !vm->evicting;
+ amdgpu_vm_eviction_unlock(vm);
@@ amdgpu_vm_evictable():
- scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
- if (!dma_fence_is_signaled(vm->last_unlocked))
- return false;
- vm->evicting = true;
- }
+ if (!amdgpu_vm_eviction_trylock(vm))
+ return false;
+ if (!dma_fence_is_signaled(vm->last_unlocked)) {
+ amdgpu_vm_eviction_unlock(vm);
+ return false;
+ }
+ vm->evicting = true;
+ amdgpu_vm_eviction_unlock(vm);
return true;
@@ amdgpu_vm_map_range() / amdgpu_vm_unmap_range(): /* +bool
svm_locked param, params.svm_locked = svm_locked; non-SVM callers pass
false */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ /* amdgpu_vm_map_range()/amdgpu_vm_unmap_range() declarations: +bool
svm_locked */
Regards,
Honglei
Regards,
Christian.
Regards,
Honglei
The background is that XE uses a different page table allocation
approach than amdgpu and we need to drop this lock in amdgpu to be
able to allocate page tables. See function amdgpu_vm_pt_alloc().
With that design here that currently doesn't work at all.
We have two options, either use the drm_gpusvm notifier_lock as
eviction_lock in amdgpu_vm.c or re-design amdgpu_vm.c to use the
same approach for allocating page tables as XE.
Some engineer from Valve is working on re-designing amdgpu_vm.c,
but that will potentially take month if not years.
So my take is that the new SVM code needs to modify amdgpu_vm.c so
that the drm_gpusvm notifier_lock is used as eviction lock by the
VM code.
Regards,
Christian.
- driver_svm_lock: In addition to the locking mentioned above,
the
driver should implement a lock to safeguard core GPU SVM
function
calls that modify state, such as
drm_gpusvm_range_find_or_insert and
drm_gpusvm_range_remove.
Two locks, two jobs.
2) The lock held in the MMU notifier is notifier_lock, never
driver_svm_lock
drm_gpusvm_notifier_invalidate():
down_write(&gpusvm->notifier_lock);
...
gpusvm->ops->invalidate(gpusvm, notifier, mmu_range);
The driver invalidate callback runs under notifier_lock only.
Per the
framework's own notifier example it just unmaps pages
and queues the range to the garbage collector no allocation,
and it
does not take driver_svm_lock:
drm_gpusvm_range_unmap_pages(...);
drm_gpusvm_range_set_unmapped(...);
driver_garbage_collector_add(...);
3) driver_svm_lock is by design an allocating, process context lock
drm_gpusvm_range_find_or_insert() asserts it and then
allocates under it:
drm_gpusvm_range_find_or_insert():
drm_gpusvm_driver_lock_held(gpusvm);
...
range = drm_gpusvm_range_alloc(...);
... mmu_interval_notifier_insert(), kzalloc
drm_gpusvm_range_remove() asserts it and frees. This is only safe
because driver_svm_lock is a sleepable, reclaim friendly lock
that is
never taken from the MMU notifier. Reference counting
handles range *lifetime*, but it does not
serialize tree insert/remove, which is exactly why the
framework still
asserts driver_svm_lock on those two entry points regardless
of refcount.
Now the three concrete points:
A) Why the primary driver_svm_lock is required
It is a framework requirement, not an amdgpu invention:
- DOC: Locking says the driver "should implement" it.
- drm_gpusvm lockdep-asserts it on every structural entry:
drm_gpusvm_range_find_or_insert() and
drm_gpusvm_range_remove() both
call drm_gpusvm_driver_lock_held().
- The reference fault handler holds it across the whole fault:
GC -> find_or_insert -> migrate -> get_pages -> bind.
Xe does exactly this:
- xe_svm.c: drm_gpusvm_driver_set_lock(&vm->svm.gpusvm,
&vm->lock);
- xe_pagefault.c: down_write(&vm->lock); before dispatching
the fault
- __xe_svm_handle_pagefault(): lockdep_assert_held_write(&vm-
>lock);
held across GC / find_or_insert / alloc_vram / get_pages /
rebind
- xe_svm_garbage_collector(): lockdep_assert_held_write(&vm-
>lock);
amdgpu's svm_lock is the same driver_svm_lock, used the same way.
B) Why eviction_lock cannot be that lock
This lock eviction_lock can only be grabbed while updating the
mapping range.
and that is precisely why it cannot be driver_svm_lock.
driver_svm_lock must wrap find_or_insert, migration, and
drm_gpusvm_range_get_pages
eviction_lock is the opposite by contract:
- It is taken with memalloc_noreclaim_save() in
amdgpu_vm_begin_critical(), specifically so no reclaim
happens while
held (to avoid the reclaim -> MMU-notifier deadlock).
Holding it
across get_pages/migration breaks that.
- TTM eviction try-locks it: amdgpu_vm_evictable() does
scoped_cond_guard(mutex_try, return false, &vm-
>eviction_lock) and
sets vm->evicting. Long holds starve eviction.
- It is a plain mutex that the SVM map path re-enters:
amdgpu_svm_range_update_mapping() -> amdgpu_vm_map_range() ->
amdgpu_vm_begin_critical() -> mutex_lock(&vm-
>eviction_lock). If
eviction_lock were also the outer SVM lock, this is a self-
deadlock.
In short, eviction_lock has the contract of notifier_lock ,
not of
driver_svm_lock. This is also why the current split is correct:
svm_lock (outer) != eviction_lock (inner). Your own rule -
"you can't
call the VM code with the lock held, the VM code must take it
itself" -
is satisfied today only because they are separate: svm_lock is
held
while calling amdgpu_vm_map_range(), and amdgpu_vm_map_range()
takes
eviction_lock itself. Merging them is what would violate that
rule.
No, they Xe vm->lock and eviction_lock are actually identical in
the handling.
They are not. Xe's vm->lock is a rw_semaphore, the "outer most
lock" of
the VM , held down_write across the whole fault. amdgpu's
eviction_lock is a mutex taken only inside
amdgpu_vm_begin_critical()
during a PT update, under memalloc_noreclaim. Xe's eviction/
reclaim
handling is separate from vm->lock. The amdgpu analogue of
Xe's vm->lock
is svm_lock, not eviction_lock.
C) Reusing an existing amdgpu_vm lock as the primary lock needs
refactor amdgpu VM
Xe can register vm->lock because Xe's VM was designed with an
outer
rw_semaphore held across faults. amdgpu_vm has no such lock: only
eviction_lock , the root PD dma_resv , and a few spinlocks.
So do it like Xe means introducing a dedicated, outer,
sleepable VM
lock held across the fault. That lock is exactly svm_lock.
Folding it
into struct amdgpu_vm as a general vm->lock is a core amdgpu
VM refactor.
Regards,
Honglei
Regards,
Christian.
+
+#if IS_ENABLED(CONFIG_DRM_AMDGPU_SVM)
+void amdgpu_svm_flush_tlb(struct amdgpu_svm *svm);
+
+int amdgpu_svm_init(struct amdgpu_device *adev, struct
amdgpu_vm *vm);
+void amdgpu_svm_close(struct amdgpu_vm *vm);
+void amdgpu_svm_fini(struct amdgpu_vm *vm);
+
+void amdgpu_svm_put(struct amdgpu_svm *svm);
+struct amdgpu_svm *amdgpu_svm_lookup_by_pasid(struct
amdgpu_device *adev,
+ uint32_t pasid);
+int amdgpu_svm_handle_fault(struct amdgpu_device *adev,
uint32_t pasid,
+ uint64_t fault_page, uint64_t ts,
+ bool write_fault);
+bool amdgpu_svm_is_enabled(struct amdgpu_vm *vm);
+
+int amdgpu_gem_svm_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+void amdgpu_svm_clean_queue(struct amdgpu_svm *svm,
+ struct list_head *work_list);
+void amdgpu_svm_sync_work(struct amdgpu_svm *svm);
+int amdgpu_svm_garbage_collector(struct amdgpu_svm *svm);
+int amdgpu_svm_apply_attr_change(struct amdgpu_svm *svm,
+ const struct amdgpu_svm_attrs *old_attrs,
+ const struct amdgpu_svm_attrs *new_attrs,
+ unsigned long start_page,
+ unsigned long last_page);
+bool amdgpu_svm_devmem_possible(struct amdgpu_svm *svm);
+#else
+static inline int amdgpu_svm_init(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm)
+{
+ return 0;
+}
+
+static inline void amdgpu_svm_close(struct amdgpu_vm *vm)
+{
+}
+
+static inline void amdgpu_svm_fini(struct amdgpu_vm *vm)
+{
+}
+
+static inline int amdgpu_svm_handle_fault(struct amdgpu_device
*adev,
+ uint32_t pasid,
+ uint64_t fault_page,
+ uint64_t ts,
+ bool write_fault)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline bool amdgpu_svm_is_enabled(struct amdgpu_vm *vm)
+{
+ return false;
+}
+
+static inline int amdgpu_gem_svm_ioctl(struct drm_device *dev,
void *data,
+ struct drm_file *filp)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_DRM_AMDGPU_SVM */
+
+#endif /* __AMDGPU_SVM_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/
gpu/drm/amd/amdgpu/amdgpu_vm.h
index ec1196d390bb7..30463a83e2e60 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -43,6 +43,7 @@ struct amdgpu_bo_va;
struct amdgpu_job;
struct amdgpu_bo_list_entry;
struct amdgpu_bo_vm;
+struct amdgpu_svm;
/*
* GPUVM handling
@@ -373,6 +374,9 @@ struct amdgpu_vm {
/* cached fault info */
struct amdgpu_vm_fault_info fault_info;
+
+ /* SVM experimental implementation */
+ struct amdgpu_svm *svm;
};
struct amdgpu_vm_manager {
