On 6/15/2026 8:43 PM, Christian König wrote:
On 6/15/26 14:31, Huang, Honglei wrote:
On 6/15/2026 7:49 PM, Christian König wrote:
On 6/15/26 11:36, Huang, Honglei wrote:
On 6/15/2026 4:08 PM, Christian König wrote:
On 6/12/26 15:20, Huang, Honglei wrote:
On 6/12/2026 8:02 PM, Christian König wrote:
On 6/12/26 11:09, Huang Rui wrote:
From: Honglei Huang <[email protected]>
...
+static inline void amdgpu_svm_assert_locked(struct amdgpu_svm *svm)
+{
+ lockdep_assert_held_write(&svm->svm_lock);
+}
What exactly is that lock protecting?
this lock is the driver_svm_lock required by the drm_gpusvm framework
It is registered by drm_gpusvm_driver_set_lock(), and drm_gpusvm lockdep
asserts it on every structural entry point, e.g.
drm_gpusvm_range_find_or_insert() / drm_gpusvm_range_remove().
Per amdgpu_svm it serializes the write/commit side against the fault
handler: range/notifier tree insert+remove, attribute changes, and the
garbage collector.
This is the same thing xe does, in drivers/gpu/drm/xe/xe_svm.c:
drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
This is clearly incorrect in that case. Our equivalent in amdgpu is
vm->eviction_lock.
That was already completely incorrect in the old KFD implementation, please
don't use that one as blueprint.
I really want to use vm->eviction_lock as SVM primary lock,
but it seems like in Xe, Xe uses vm->lock as an outer VM lock by design rwsem
instead of mutex lock to broad VM ownership, not an eviction lock, they are
semantically different.
No, they are actually identical in the handling.
I tried to replace the svm lock with eviction lock locally, ABBA dead lock
encountered:
amdgpu_svm_handle_fault
amdgpu_svm_lock (A: eviction_lock)
That's nonsense. This lock can only be grabbed while updating the mapping range.
fault_map_range
amdgpu_svm_range_update_mapping
drm_gpusvm_notifier_lock (B)
drm_gpusvm_notifier_invalidate
down_write(B - notifier_lock)
Same here. You simply can't call the VM code with the lock held.
The VM code itself must take it as appropriate.
I think the core gap is what exactly does SVM lock (drm_gpusvm_driver_set_lock)
do in the framework
I checked xe svm, in xe_svm.c:1209:__xe_svm_handle_pagefault() the very first
thing is:
lockdep_assert_held_write(&vm->lock);
The lock is already held on entry and stays held across the entire fault
progress:
xe_svm_garbage_collector(vm)
xe_svm_range_find_or_insert(...)
xe_svm_alloc_vram(...) (possible migration)
xe_svm_range_get_pages(...)
xe_vm_range_rebind(...) (the actual bind)
they are not grabbed while updating the mapping range.
And the drm_gpusvm framework itself requires the driver lock to be held at the
structural entry points:
drm_gpusvm_range_find_or_insert()
...
drm_gpusvm_driver_lock_held(gpusvm);
...
drm_gpusvm_range_remove()
...
drm_gpusvm_driver_lock_held(gpusvm);
...
The garbage collector and unmap paths hold the same lock in write mode:
xe_svm_garbage_collector()
...
lockdep_assert_held_write(&vm->lock);
...
__xe_svm_garbage_collector
...
xe_svm_unmap_address_range
...
lockdep_assert_held_write
__xe_svm_garbage_collector
drm_gpusvm_range_put
...
These codes indicate that this lock needs to be held on the outer layer to
protect the data structure of SVM, rather than just during GPU mapping.
In that case that is a major bug in the drm_svm handling. The lock *must* be
held only during GPU mapping and all other data structures lifetime handled by
reference counting.
That is a core requirement of the SVM handling because you can't allocate much
memory in the MMU notifier and so you also can't allocate memory under that
lock when it is held in the MMU notifier.
Will discuss this issue with drm gpu svm maintainer.
But at least under current code structure of drmgpu svm, use
eviction_lock can not make SVM functional, there are many assert in
drmgpu_svm framwork APIs, block the calling sequence/lock order in
amdgpu SVM.
Will discuss about maintainer about how to handle this condition in
drmgpu svm framework.And keep current lock unchanged before we have a
solid design.
Regards,
Honglei
Regards,
Christian.
Making eviction lock the main lock of SVM will indeed block the functionality
of SVM and also violate the design philosophyof drmgpu svm
as far as I ca see, at least I can not make the SVM functional under this
conditions.
I am not a expert of drmgpu svm, how about ask maintainer of drmgpu svm
to ask how to use the drm_gpusvm_driver_set_lock, and can eviction lock be used
for drm_gpusvm_driver_set_lock.
Regards,
Honglei
Regards,
Christian.
amdgpu_svm_invalidate
amdgpu_svm_range_invalidate
amdgpu_svm_range_notifier_event_begin
amdgpu_svm_range_zap_ptes
amdgpu_vm_update_range
amdgpu_vm_eviction_lock (A: eviction_lock)
the current SVM side can switch its registered driver lock at any time easily
as you wish, but doing so safely is not just an amdgpu SVM local change.
The issue is that change lock / change lock registration is easy, while lock
semantics are not. SVM execution paths like fault handling, invalidation,
mapping updates, garbage collection, notifier callbacks... are coupled with VM
locking and update flows follow the xe svm style. If we switch SVM to a
different VM lock now, we can introduce lock recursion, ABBA lock...
SVM can technically switch lock registration now.
But make all stack works and pass all tests maybe needs amdgpu VM refactoring.
Regards,
Honglei
Regards,
Christian.
