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