Re: [PATCH v9 02/18] drm/amdgpu: add SVM core header and VM integration
Honglei Huang <[email protected]>
| Newsgroups | org.freedesktop.lists.amd-gfx,org.freedesktop.lists.dri-devel |
|---|---|
| Message-ID | <[email protected]> |
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 {
>>>>>>>
>>>>>>
>>>>
>>>
>