<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2021-11-17 6:18 p.m., Felix Kuehling wrote: </div> <blockquote type="cite" cite="mid:fb6b3750-56e4-8a79-c479-10c4cbbd3e4a@amd.com">On 2021-11-16 10:43 p.m., Philip Yang wrote: <blockquote type="cite">kfd process mmu release notifier callback drain retry fault to ensure no retry fault comes after removing kfd process from the hash table, otherwise svm page fault handler will fail to recover the fault and dump GPU vm fault log. Drain retry fault needs flush restore page fault work to wait for the last fault is handled because IH dispatch increase rptr first and then calls restore_pages, so restore pages may still handle the last fault but amdgpu_ih_has_checkpoint_processed return true. </blockquote> This fixes the problem, but it will result in waiting longer than necessary because the worker only finishes when the IH ring is empty. </blockquote> Working on new IH ring1 overflow patch to handle drain_retry_fault race, flush will not need here. <blockquote type="cite" cite="mid:fb6b3750-56e4-8a79-c479-10c4cbbd3e4a@amd.com"> <blockquote type="cite"> restore pages can not call mmget because mmput may call mmu notifier release to cause deadlock. </blockquote> See my comment inline. <blockquote type="cite"> Refactor deferred list work to call mmget and take mmap write lock to handle all ranges, to avoid mm is gone while inserting mmu notifier. Signed-off-by: Philip Yang <a class="moz-txt-link-rfc2396E" href="mailto:Philip.Yang@amd.com"><Philip.Yang@amd.com></a> --- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 6 +++ drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 69 ++++++++++++------------ drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 1 + 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index d4c8a6948a9f..8b4b045d5c92 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -1143,6 +1143,12 @@ static void kfd_process_notifier_release(struct mmu_notifier *mn, if (WARN_ON(p->mm != mm)) return; + /* + * Ensure no retry fault comes in afterwards, as page fault handler will + * not find kfd process and take mm lock to recover fault. + */ + svm_range_drain_retry_fault(&p->svms); + mutex_lock(&kfd_processes_mutex); hash_del_rcu(&p->kfd_processes); mutex_unlock(&kfd_processes_mutex); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index 88360f23eb61..c1f367934428 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1953,9 +1953,10 @@ svm_range_handle_list_op(struct svm_range_list *svms, struct svm_range *prange) } } -static void svm_range_drain_retry_fault(struct svm_range_list *svms) +void svm_range_drain_retry_fault(struct svm_range_list *svms) { struct kfd_process_device *pdd; + struct amdgpu_device *adev; struct kfd_process *p; uint32_t i; @@ -1967,9 +1968,11 @@ static void svm_range_drain_retry_fault(struct svm_range_list *svms) continue; pr_debug("drain retry fault gpu %d svms %p\n", i, svms); + adev = pdd->dev->adev; + amdgpu_ih_wait_on_checkpoint_process(adev, &adev->irq.ih1); - amdgpu_ih_wait_on_checkpoint_process(pdd->dev->adev, - &pdd->dev->adev->irq.ih1); + /* Wait for the last page fault is handled */ + flush_work(&adev->irq.ih1_work); pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms); } } @@ -1979,43 +1982,43 @@ static void svm_range_deferred_list_work(struct work_struct *work) struct svm_range_list *svms; struct svm_range *prange; struct mm_struct *mm; + struct kfd_process *p; svms = container_of(work, struct svm_range_list, deferred_list_work); pr_debug("enter svms 0x%p\n", svms); + p = container_of(svms, struct kfd_process, svms); + mm = p->mm; + + /* Take mm->mm_users to avoid mm is gone when inserting mmu notifier */ + if (!mm || !mmget_not_zero(mm)) { </blockquote> get_task_mm would be safer than relying on p->mm. I regret ever adding that to the process structure. </blockquote> Will use get_task_mm(pdd->process->lead_thread), it is safer as we take task reference. <blockquote type="cite" cite="mid:fb6b3750-56e4-8a79-c479-10c4cbbd3e4a@amd.com"> <blockquote type="cite">+ pr_debug("svms 0x%p process mm gone\n", svms); + return; + } +retry: + mmap_write_lock(mm); + + /* Checking for the need to drain retry faults must be inside + * mmap write lock to serialize with munmap notifiers. + */ + if (unlikely(READ_ONCE(svms->drain_pagefaults))) { + WRITE_ONCE(svms->drain_pagefaults, false); + mmap_write_unlock(mm); + svm_range_drain_retry_fault(svms); + goto retry; + } + spin_lock(&svms->deferred_list_lock); while (!list_empty(&svms->deferred_range_list)) { prange = list_first_entry(&svms->deferred_range_list, struct svm_range, deferred_list); + list_del_init(&prange->deferred_list); spin_unlock(&svms->deferred_list_lock); + pr_debug("prange 0x%p [0x%lx 0x%lx] op %d\n", prange, prange->start, prange->last, prange->work_item.op); - mm = prange->work_item.mm; -retry: - mmap_write_lock(mm); mutex_lock(&svms->lock); - - /* Checking for the need to drain retry faults must be in - * mmap write lock to serialize with munmap notifiers. - * - * Remove from deferred_list must be inside mmap write lock, - * otherwise, svm_range_list_lock_and_flush_work may hold mmap - * write lock, and continue because deferred_list is empty, then - * deferred_list handle is blocked by mmap write lock. - */ - spin_lock(&svms->deferred_list_lock); - if (unlikely(svms->drain_pagefaults)) { - svms->drain_pagefaults = false; - spin_unlock(&svms->deferred_list_lock); - mutex_unlock(&svms->lock); - mmap_write_unlock(mm); - svm_range_drain_retry_fault(svms); - goto retry; - } - list_del_init(&prange->deferred_list); - spin_unlock(&svms->deferred_list_lock); - mutex_lock(&prange->migrate_mutex); while (!list_empty(&prange->child_list)) { struct svm_range *pchild; @@ -2031,12 +2034,13 @@ static void svm_range_deferred_list_work(struct work_struct *work) svm_range_handle_list_op(svms, prange); mutex_unlock(&svms->lock); - mmap_write_unlock(mm); spin_lock(&svms->deferred_list_lock); } spin_unlock(&svms->deferred_list_lock); + mmap_write_unlock(mm); + mmput(mm); pr_debug("exit svms 0x%p\n", svms); } @@ -2600,12 +2604,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid, pr_debug("restoring svms 0x%p fault address 0x%llx\n", svms, addr); - mm = get_task_mm(p->lead_thread); - if (!mm) { - pr_debug("svms 0x%p failed to get mm\n", svms); - r = -ESRCH; - goto out; - } + /* mm is available because kfd_process_notifier_release drain fault */ </blockquote> This is not a valid assumption because the mm_users count is 0 when the notifier_release runs. So you can't rely on the mm being usable here while you're draining faults in notifier_release. A better way to avoid the deadlock would be to drain faults not in notifier_release, but in kfd_process_wq_release. </blockquote> Good idea to drain faults in kfd_process_wq_release, then we can keep get_task_mm(pdd->process->lead_thread), if task mm is gone, it is safe to ignore the fault, return 0, not -ESRCH. Regards, Philip <blockquote type="cite" cite="mid:fb6b3750-56e4-8a79-c479-10c4cbbd3e4a@amd.com"> Regards, Felix <blockquote type="cite">+ mm = p->mm; mmap_read_lock(mm); retry_write_locked: @@ -2708,7 +2708,6 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid, svm_range_count_fault(adev, p, gpuidx); - mmput(mm); out: kfd_unref_process(p); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h index 6dc91c33e80f..0a8bcdb3dddf 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h @@ -189,6 +189,7 @@ void svm_range_prefault(struct svm_range *prange, struct mm_struct *mm, struct kfd_process_device * svm_range_get_pdd_by_adev(struct svm_range *prange, struct amdgpu_device *adev); void svm_range_list_lock_and_flush_work(struct svm_range_list *svms, struct mm_struct *mm); +void svm_range_drain_retry_fault(struct svm_range_list *svms); /* SVM API and HMM page migration work together, device memory type * is initialized to not 0 when page migration register device memory. </blockquote> </blockquote> </body> </html>