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