Hi Chong, On 11/10/26 10:42, Li, Chong(Alan) wrote: > AMD General > > Hi, Christian. > > The lock of each fence indeed never changes. > Each PT update fence has its own lock. > > The issue is in the fence locking process. > > When a fence signals, dma_fence_signal() holds the fence's lock and > calls amdgpu_vm_tlb_seq_cb(), which increments vm->tlb_seq. > amdgpu_vm_tlb_seq() waits on the lock of vm->last_tlb_flush until > amdgpu_vm_tlb_seq_cb() completes. The lock of vm->last_tlb_flush > ensures that amdgpu_vm_tlb_seq() returns the incremented > vm->tlb_seq. This is the normal flow. > > But vm->last_tlb_flush may already point to a new fence: > amdgpu_vm_tlb_flush() replaces it as soon as a new PT update is > submitted, usually before the old fence signals. amdgpu_vm_tlb_seq() > then takes the new fence's lock, which is not held while the old > fence's callback runs, so it is not blocked and returns vm->tlb_seq > before it is incremented.
No you are mixing things up, vm->last_tlb_flush was something completely different. vm->last_seq_flush is the fence where we install the callback on and yes that is replaced with the next installed callback, but that doesn't matter the lock is always the same. vm->last_tlb_inv is the TLB invalidation fence which actively invalidates the TLB for a passid. That are two completely different things. Regards, Christian. > > Finally, updates = amdgpu_vm_tlb_seq() misses the increment, the > condition (*id)->flushed_updates < updates is not satisfied, and > needs_flush is false, so the job reuses the VMID without a VM flush. > > Please let me know if anything is unclear or if I'm missing something. > > > > Thanks, > Chong. > > > > > -----Original Message----- > From: Koenig, Christian <[email protected]> > Sent: Monday, November 9, 2026 10:08 PM > To: Li, Chong(Alan) <[email protected]>; [email protected] > Subject: Re: [PATCH] drm/amdgpu: fix page fault by not skipping VM flush > before tlb_seq update > > On 11/9/26 15:06, chong li wrote: >> amdgpu_vm_tlb_seq() waits for the tlb_seq callback by taking the lock >> of >> vm->last_tlb_flush. Newer page table updates replace >> vm->vm->last_tlb_flush >> immediately, without waiting for older callbacks. A job scheduled >> after an older fence signals but before its callback finishes takes >> the newer fence's lock, while the callback runs under the older fence's own >> lock. >> The job misses that tlb_seq increment and can reuse a VMID whose TLB >> was last flushed before the older page table update completed, without >> flushing the TLB again. > > That's not even remotely correct. > > All fences use the same lock, that's why this works correctly. > > Regards, > Christian. > >> >> how to fix: >> Count the flush-requiring page table updates whose tlb_seq callback >> has not run yet, and request a VM flush when a job reuses a VMID while >> this count is non-zero. >> >> Signed-off-by: chong li <[email protected]> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 10 ++++++++-- >> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 +++++++++++- >> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 10 ++++++++++ >> 3 files changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c >> index 9c879ac68352..b9e058da162b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c >> @@ -393,7 +393,8 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm >> *vm, >> unsigned vmhub = ring->vm_hub; >> uint64_t fence_context = adev->fence_context + ring->idx; >> bool needs_flush = vm->use_cpu_for_update; >> - uint64_t updates = amdgpu_vm_tlb_seq(vm); >> + bool tlb_pending; >> + uint64_t updates = amdgpu_vm_tlb_seq_pending(vm, &tlb_pending); >> int r; >> >> *id = vm->reserved_vmid[vmhub]; >> @@ -405,6 +406,8 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm >> *vm, >> !dma_fence_is_signaled((*id)->last_flush))) >> needs_flush = true; >> >> + needs_flush |= tlb_pending; >> + >> if ((*id)->owner != vm->immediate.fence_context || >> (!adev->vm_manager.concurrent_flush && needs_flush)) { >> struct dma_fence *tmp; >> @@ -456,7 +459,8 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm, >> unsigned vmhub = ring->vm_hub; >> struct amdgpu_kq_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub]; >> uint64_t fence_context = adev->fence_context + ring->idx; >> - uint64_t updates = amdgpu_vm_tlb_seq(vm); >> + bool tlb_pending; >> + uint64_t updates = amdgpu_vm_tlb_seq_pending(vm, &tlb_pending); >> int r; >> >> job->vm_needs_flush = vm->use_cpu_for_update; @@ -480,6 +484,8 @@ >> static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm, >> if ((*id)->flushed_updates < updates) >> needs_flush = true; >> >> + needs_flush |= tlb_pending; >> + >> if (needs_flush && !adev->vm_manager.concurrent_flush) >> continue; >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> index 2ba33212d507..6d15fd001a42 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> @@ -1095,6 +1095,9 @@ static void amdgpu_vm_tlb_seq_cb(struct >> dma_fence *fence, >> >> tlb_cb = container_of(cb, typeof(*tlb_cb), cb); >> atomic64_inc(&tlb_cb->vm->tlb_seq); >> + /* Pairs with atomic_read_acquire() in amdgpu_vm_tlb_seq_pending() */ >> + smp_mb__before_atomic(); >> + atomic_dec(&tlb_cb->vm->tlb_pending); >> kfree(tlb_cb); >> } >> >> @@ -1281,9 +1284,16 @@ int amdgpu_vm_update_range(struct amdgpu_device >> *adev, struct amdgpu_vm *vm, >> start = tmp; >> } >> >> + /* Must be visible before the update fence can signal */ >> + if (params.needs_flush) >> + atomic_inc(&vm->tlb_pending); >> + >> r = vm->update_funcs->commit(¶ms, fence); >> - if (r) >> + if (r) { >> + if (params.needs_flush) >> + atomic_dec(&vm->tlb_pending); >> goto error_free; >> + } >> >> if (params.needs_flush) { >> amdgpu_vm_tlb_flush(¶ms, fence, tlb_cb); diff --git >> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >> index d8e17f493f61..312fd0d268ce 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >> @@ -337,6 +337,8 @@ struct amdgpu_vm { >> >> /* Last finished delayed update */ >> atomic64_t tlb_seq; >> + /* Flush updates whose tlb_seq callback has not run yet */ >> + atomic_t tlb_pending; >> struct dma_fence *last_tlb_flush; >> atomic64_t kfd_last_flushed_seq; >> uint64_t tlb_fence_context; >> @@ -578,6 +580,14 @@ static inline uint64_t amdgpu_vm_tlb_seq(struct >> amdgpu_vm *vm) >> return atomic64_read(&vm->tlb_seq); >> } >> >> +/* Read tlb_pending first; pairs with the barrier in >> +amdgpu_vm_tlb_seq_cb() */ static inline uint64_t >> amdgpu_vm_tlb_seq_pending(struct amdgpu_vm *vm, >> + bool *pending) >> +{ >> + *pending = atomic_read_acquire(&vm->tlb_pending) != 0; >> + return amdgpu_vm_tlb_seq(vm); >> +} >> + >> void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev, >> unsigned int pasid, >> uint64_t addr, >
