[PATCH 1/3] drm/amdkfd: process exit and retry fault race

[PATCH 1/3] drm/amdkfd: process exit and retry fault race

[Philip Yang]

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.

restore pages can not call mmget because mmput may call mmu notifier
release to cause deadlock.

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 <Philip.Yang@amd.com>
---
 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)) {
+		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 */
+	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.
-- 
2.17.1

[PATCH 2/3] drm/amdkfd: handle VMA remove race

[Philip Yang]

VMA may be removed before unmap notifier callback, restore pages take
mmap write lock to lookup VMA to avoid race, and then create unregister
new range and check VMA access permission, then downgrade to take mmap
read lock to recover fault. Refactor code to avoid duplicate VMA lookup.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 65 ++++++++++------------------
 1 file changed, 24 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index c1f367934428..3eb0a9491755 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2329,20 +2329,13 @@ svm_range_best_restore_location(struct svm_range *prange,
 }
 
 static int
-svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
-			       unsigned long *start, unsigned long *last,
-			       bool *is_heap_stack)
+svm_range_get_range_boundaries(struct kfd_process *p, struct vm_area_struct *vma,
+			       int64_t addr, unsigned long *start,
+			       unsigned long *last, bool *is_heap_stack)
 {
-	struct vm_area_struct *vma;
 	struct interval_tree_node *node;
 	unsigned long start_limit, end_limit;
 
-	vma = find_vma(p->mm, addr << PAGE_SHIFT);
-	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-		pr_debug("VMA does not exist in address [0x%llx]\n", addr);
-		return -EFAULT;
-	}
-
 	*is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
 			  vma->vm_end >= vma->vm_mm->start_brk) ||
 			 (vma->vm_start <= vma->vm_mm->start_stack &&
@@ -2437,9 +2430,10 @@ svm_range_check_vm_userptr(struct kfd_process *p, uint64_t start, uint64_t last,
 
 static struct
 svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
-						struct kfd_process *p,
-						struct mm_struct *mm,
-						int64_t addr)
+					       struct kfd_process *p,
+					       struct mm_struct *mm,
+					       struct vm_area_struct *vma,
+					       int64_t addr)
 {
 	struct svm_range *prange = NULL;
 	unsigned long start, last;
@@ -2449,7 +2443,7 @@ svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
 	uint64_t bo_l = 0;
 	int r;
 
-	if (svm_range_get_range_boundaries(p, addr, &start, &last,
+	if (svm_range_get_range_boundaries(p, vma, addr, &start, &last,
 					   &is_heap_stack))
 		return NULL;
 
@@ -2552,20 +2546,13 @@ svm_range_count_fault(struct amdgpu_device *adev, struct kfd_process *p,
 }
 
 static bool
-svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool write_fault)
+svm_fault_allowed(struct vm_area_struct *vma, bool write_fault)
 {
 	unsigned long requested = VM_READ;
-	struct vm_area_struct *vma;
 
 	if (write_fault)
 		requested |= VM_WRITE;
 
-	vma = find_vma(mm, addr << PAGE_SHIFT);
-	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-		pr_debug("address 0x%llx VMA is removed\n", addr);
-		return true;
-	}
-
 	pr_debug("requested 0x%lx, vma permission flags 0x%lx\n", requested,
 		vma->vm_flags);
 	return (vma->vm_flags & requested) == requested;
@@ -2582,7 +2569,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 	uint64_t timestamp;
 	int32_t best_loc;
 	int32_t gpuidx = MAX_GPU_INSTANCE;
-	bool write_locked = false;
+	struct vm_area_struct *vma = NULL;
 	int r = 0;
 
 	if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
@@ -2606,26 +2593,22 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 
 	/* mm is available because kfd_process_notifier_release drain fault */
 	mm = p->mm;
+	mmap_write_lock(mm);
+
+	vma = find_vma(p->mm, addr << PAGE_SHIFT);
+	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+		pr_debug("VMA not found for address 0x%llx\n", addr);
+		mmap_write_downgrade(mm);
+		r = -EFAULT;
+		goto out_unlock_mm;
+	}
 
-	mmap_read_lock(mm);
-retry_write_locked:
 	mutex_lock(&svms->lock);
 	prange = svm_range_from_addr(svms, addr, NULL);
 	if (!prange) {
 		pr_debug("failed to find prange svms 0x%p address [0x%llx]\n",
 			 svms, addr);
-		if (!write_locked) {
-			/* Need the write lock to create new range with MMU notifier.
-			 * Also flush pending deferred work to make sure the interval
-			 * tree is up to date before we add a new range
-			 */
-			mutex_unlock(&svms->lock);
-			mmap_read_unlock(mm);
-			mmap_write_lock(mm);
-			write_locked = true;
-			goto retry_write_locked;
-		}
-		prange = svm_range_create_unregistered_range(adev, p, mm, addr);
+		prange = svm_range_create_unregistered_range(adev, p, mm, vma, addr);
 		if (!prange) {
 			pr_debug("failed to create unregistered range svms 0x%p address [0x%llx]\n",
 				 svms, addr);
@@ -2634,8 +2617,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 			goto out_unlock_svms;
 		}
 	}
-	if (write_locked)
-		mmap_write_downgrade(mm);
+
+	mmap_write_downgrade(mm);
 
 	mutex_lock(&prange->migrate_mutex);
 
@@ -2652,7 +2635,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 		goto out_unlock_range;
 	}
 
-	if (!svm_fault_allowed(mm, addr, write_fault)) {
+	if (!svm_fault_allowed(vma, write_fault)) {
 		pr_debug("fault addr 0x%llx no %s permission\n", addr,
 			write_fault ? "write" : "read");
 		r = -EPERM;
@@ -2704,10 +2687,10 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 	mutex_unlock(&prange->migrate_mutex);
 out_unlock_svms:
 	mutex_unlock(&svms->lock);
+out_unlock_mm:
 	mmap_read_unlock(mm);
 
 	svm_range_count_fault(adev, p, gpuidx);
-
 out:
 	kfd_unref_process(p);
 
-- 
2.17.1

Re: [PATCH 2/3] drm/amdkfd: handle VMA remove race

[Felix Kuehling]

On 2021-11-16 10:43 p.m., Philip Yang wrote:
> VMA may be removed before unmap notifier callback, restore pages take
> mmap write lock to lookup VMA to avoid race,

The old code looked up the VMA after taking the mmap lock (either read 
or write) and kept holding the lock afterwards. I think even with your 
new code it's possible that the VMA disappears before you take the lock 
the first time, so always taking the write lock only reduces the time 
window in which things can go wrong, but it doesn't remove the race.

I still struggle to understand the race you're trying to fix. The only 
time the svm_restore_pages can see that the VMA is gone AND the prange 
is gone is after the deferred worker has removed the prange. But the 
fault draining in the deferred worker should prevent us from ever seeing 
stale faults in that situation. That means, if no prange is found and no 
VMA is found, it's definitely an application bug.

The only possible race is in the case where the prange still exists but 
the VMA is gone (needed by svm_fault_allowed). We can treat that as a 
special case where we just return success because we know that we're 
handling a stale fault for a VMA that's in the middle of being unmapped. 
The fact that the prange still existed means that there once was a valid 
mapping at the address but the deferred worker just hasn't had a chance 
to clean it up yet.

One more comment inline.


>   and then create unregister
> new range and check VMA access permission, then downgrade to take mmap
> read lock to recover fault. Refactor code to avoid duplicate VMA lookup.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 65 ++++++++++------------------
>   1 file changed, 24 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index c1f367934428..3eb0a9491755 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2329,20 +2329,13 @@ svm_range_best_restore_location(struct svm_range *prange,
>   }
>   
>   static int
> -svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
> -			       unsigned long *start, unsigned long *last,
> -			       bool *is_heap_stack)
> +svm_range_get_range_boundaries(struct kfd_process *p, struct vm_area_struct *vma,
> +			       int64_t addr, unsigned long *start,
> +			       unsigned long *last, bool *is_heap_stack)
>   {
> -	struct vm_area_struct *vma;
>   	struct interval_tree_node *node;
>   	unsigned long start_limit, end_limit;
>   
> -	vma = find_vma(p->mm, addr << PAGE_SHIFT);
> -	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> -		pr_debug("VMA does not exist in address [0x%llx]\n", addr);
> -		return -EFAULT;
> -	}
> -
>   	*is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
>   			  vma->vm_end >= vma->vm_mm->start_brk) ||
>   			 (vma->vm_start <= vma->vm_mm->start_stack &&
> @@ -2437,9 +2430,10 @@ svm_range_check_vm_userptr(struct kfd_process *p, uint64_t start, uint64_t last,
>   
>   static struct
>   svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
> -						struct kfd_process *p,
> -						struct mm_struct *mm,
> -						int64_t addr)
> +					       struct kfd_process *p,
> +					       struct mm_struct *mm,
> +					       struct vm_area_struct *vma,
> +					       int64_t addr)
>   {
>   	struct svm_range *prange = NULL;
>   	unsigned long start, last;
> @@ -2449,7 +2443,7 @@ svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
>   	uint64_t bo_l = 0;
>   	int r;
>   
> -	if (svm_range_get_range_boundaries(p, addr, &start, &last,
> +	if (svm_range_get_range_boundaries(p, vma, addr, &start, &last,
>   					   &is_heap_stack))
>   		return NULL;
>   
> @@ -2552,20 +2546,13 @@ svm_range_count_fault(struct amdgpu_device *adev, struct kfd_process *p,
>   }
>   
>   static bool
> -svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool write_fault)
> +svm_fault_allowed(struct vm_area_struct *vma, bool write_fault)
>   {
>   	unsigned long requested = VM_READ;
> -	struct vm_area_struct *vma;
>   
>   	if (write_fault)
>   		requested |= VM_WRITE;
>   
> -	vma = find_vma(mm, addr << PAGE_SHIFT);
> -	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> -		pr_debug("address 0x%llx VMA is removed\n", addr);
> -		return true;
> -	}
> -
>   	pr_debug("requested 0x%lx, vma permission flags 0x%lx\n", requested,
>   		vma->vm_flags);
>   	return (vma->vm_flags & requested) == requested;
> @@ -2582,7 +2569,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   	uint64_t timestamp;
>   	int32_t best_loc;
>   	int32_t gpuidx = MAX_GPU_INSTANCE;
> -	bool write_locked = false;
> +	struct vm_area_struct *vma = NULL;
>   	int r = 0;
>   
>   	if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
> @@ -2606,26 +2593,22 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   
>   	/* mm is available because kfd_process_notifier_release drain fault */
>   	mm = p->mm;
> +	mmap_write_lock(mm);

Always taking the write lock is unnecessary. I think we can keep the old 
strategy of retrying with the write lock only when necessary. I think 
this should work correctly as long as you lookup the VMA every time 
after taking either the mmap read or write lock. The vma you looked up 
should be valid as long as you hold that lock.

As I pointed out above, if svm_range_from_addr finds a prange but the 
VMA is missing, we can treat that as a special case and return success 
(just draining a stale fault on a VMA that's being unmapped).

Regards,
   Felix


> +
> +	vma = find_vma(p->mm, addr << PAGE_SHIFT);
> +	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> +		pr_debug("VMA not found for address 0x%llx\n", addr);
> +		mmap_write_downgrade(mm);
> +		r = -EFAULT;
> +		goto out_unlock_mm;
> +	}
>   
> -	mmap_read_lock(mm);
> -retry_write_locked:
>   	mutex_lock(&svms->lock);
>   	prange = svm_range_from_addr(svms, addr, NULL);
>   	if (!prange) {
>   		pr_debug("failed to find prange svms 0x%p address [0x%llx]\n",
>   			 svms, addr);
> -		if (!write_locked) {
> -			/* Need the write lock to create new range with MMU notifier.
> -			 * Also flush pending deferred work to make sure the interval
> -			 * tree is up to date before we add a new range
> -			 */
> -			mutex_unlock(&svms->lock);
> -			mmap_read_unlock(mm);
> -			mmap_write_lock(mm);
> -			write_locked = true;
> -			goto retry_write_locked;
> -		}
> -		prange = svm_range_create_unregistered_range(adev, p, mm, addr);
> +		prange = svm_range_create_unregistered_range(adev, p, mm, vma, addr);
>   		if (!prange) {
>   			pr_debug("failed to create unregistered range svms 0x%p address [0x%llx]\n",
>   				 svms, addr);
> @@ -2634,8 +2617,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   			goto out_unlock_svms;
>   		}
>   	}
> -	if (write_locked)
> -		mmap_write_downgrade(mm);
> +
> +	mmap_write_downgrade(mm);
>   
>   	mutex_lock(&prange->migrate_mutex);
>   
> @@ -2652,7 +2635,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   		goto out_unlock_range;
>   	}
>   
> -	if (!svm_fault_allowed(mm, addr, write_fault)) {
> +	if (!svm_fault_allowed(vma, write_fault)) {
>   		pr_debug("fault addr 0x%llx no %s permission\n", addr,
>   			write_fault ? "write" : "read");
>   		r = -EPERM;
> @@ -2704,10 +2687,10 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   	mutex_unlock(&prange->migrate_mutex);
>   out_unlock_svms:
>   	mutex_unlock(&svms->lock);
> +out_unlock_mm:
>   	mmap_read_unlock(mm);
>   
>   	svm_range_count_fault(adev, p, gpuidx);
> -
>   out:
>   	kfd_unref_process(p);
>   

Re: [PATCH 2/3] drm/amdkfd: handle VMA remove race

[philip yang]

[-- Attachment #1: Type: text/html, Size: 17812 bytes --]

Re: [PATCH 2/3] drm/amdkfd: handle VMA remove race

[Felix Kuehling]

Am 2021-11-18 um 10:00 a.m. schrieb philip yang:
>
>
> On 2021-11-17 7:10 p.m., Felix Kuehling wrote:
>> On 2021-11-16 10:43 p.m., Philip Yang wrote:
>>> VMA may be removed before unmap notifier callback, restore pages take
>>> mmap write lock to lookup VMA to avoid race,
>>
>> The old code looked up the VMA after taking the mmap lock (either
>> read or write) and kept holding the lock afterwards. I think even
>> with your new code it's possible that the VMA disappears before you
>> take the lock the first time, so always taking the write lock only
>> reduces the time window in which things can go wrong, but it doesn't
>> remove the race.
> Take mmap write lock will serialize with __do_munmap,

__do_munmap runs with the mmap write lock. Taking the read lock should
be sufficient to serialize with it.

Regards,
  Felix


> to ensure vma remove and unmap callback are done, because unmap
> callback set drain_retryfaults flag, so we can safely drain the
> faults, and it is app bug if vma not found after taking mmap write lock.
>>
>> I still struggle to understand the race you're trying to fix. The
>> only time the svm_restore_pages can see that the VMA is gone AND the
>> prange is gone is after the deferred worker has removed the prange.
>> But the fault draining in the deferred worker should prevent us from
>> ever seeing stale faults in that situation. That means, if no prange
>> is found and no VMA is found, it's definitely an application bug.
>>
>> The only possible race is in the case where the prange still exists
>> but the VMA is gone (needed by svm_fault_allowed). We can treat that
>> as a special case where we just return success because we know that
>> we're handling a stale fault for a VMA that's in the middle of being
>> unmapped. The fact that the prange still existed means that there
>> once was a valid mapping at the address but the deferred worker just
>> hasn't had a chance to clean it up yet.
>>
> Yes, that is only possible race.
>> One more comment inline.
>>
>>
>>>   and then create unregister
>>> new range and check VMA access permission, then downgrade to take mmap
>>> read lock to recover fault. Refactor code to avoid duplicate VMA
>>> lookup.
>>>
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 65
>>> ++++++++++------------------
>>>   1 file changed, 24 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index c1f367934428..3eb0a9491755 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -2329,20 +2329,13 @@ svm_range_best_restore_location(struct
>>> svm_range *prange,
>>>   }
>>>     static int
>>> -svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
>>> -                   unsigned long *start, unsigned long *last,
>>> -                   bool *is_heap_stack)
>>> +svm_range_get_range_boundaries(struct kfd_process *p, struct
>>> vm_area_struct *vma,
>>> +                   int64_t addr, unsigned long *start,
>>> +                   unsigned long *last, bool *is_heap_stack)
>>>   {
>>> -    struct vm_area_struct *vma;
>>>       struct interval_tree_node *node;
>>>       unsigned long start_limit, end_limit;
>>>   -    vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>> -    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>> -        pr_debug("VMA does not exist in address [0x%llx]\n", addr);
>>> -        return -EFAULT;
>>> -    }
>>> -
>>>       *is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
>>>                 vma->vm_end >= vma->vm_mm->start_brk) ||
>>>                (vma->vm_start <= vma->vm_mm->start_stack &&
>>> @@ -2437,9 +2430,10 @@ svm_range_check_vm_userptr(struct kfd_process
>>> *p, uint64_t start, uint64_t last,
>>>     static struct
>>>   svm_range *svm_range_create_unregistered_range(struct
>>> amdgpu_device *adev,
>>> -                        struct kfd_process *p,
>>> -                        struct mm_struct *mm,
>>> -                        int64_t addr)
>>> +                           struct kfd_process *p,
>>> +                           struct mm_struct *mm,
>>> +                           struct vm_area_struct *vma,
>>> +                           int64_t addr)
>>>   {
>>>       struct svm_range *prange = NULL;
>>>       unsigned long start, last;
>>> @@ -2449,7 +2443,7 @@ svm_range
>>> *svm_range_create_unregistered_range(struct amdgpu_device *adev,
>>>       uint64_t bo_l = 0;
>>>       int r;
>>>   -    if (svm_range_get_range_boundaries(p, addr, &start, &last,
>>> +    if (svm_range_get_range_boundaries(p, vma, addr, &start, &last,
>>>                          &is_heap_stack))
>>>           return NULL;
>>>   @@ -2552,20 +2546,13 @@ svm_range_count_fault(struct amdgpu_device
>>> *adev, struct kfd_process *p,
>>>   }
>>>     static bool
>>> -svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool
>>> write_fault)
>>> +svm_fault_allowed(struct vm_area_struct *vma, bool write_fault)
>>>   {
>>>       unsigned long requested = VM_READ;
>>> -    struct vm_area_struct *vma;
>>>         if (write_fault)
>>>           requested |= VM_WRITE;
>>>   -    vma = find_vma(mm, addr << PAGE_SHIFT);
>>> -    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>> -        pr_debug("address 0x%llx VMA is removed\n", addr);
>>> -        return true;
>>> -    }
>>> -
>>>       pr_debug("requested 0x%lx, vma permission flags 0x%lx\n",
>>> requested,
>>>           vma->vm_flags);
>>>       return (vma->vm_flags & requested) == requested;
>>> @@ -2582,7 +2569,7 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>       uint64_t timestamp;
>>>       int32_t best_loc;
>>>       int32_t gpuidx = MAX_GPU_INSTANCE;
>>> -    bool write_locked = false;
>>> +    struct vm_area_struct *vma = NULL;
>>>       int r = 0;
>>>         if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
>>> @@ -2606,26 +2593,22 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>         /* mm is available because kfd_process_notifier_release
>>> drain fault */
>>>       mm = p->mm;
>>> +    mmap_write_lock(mm);
>>
>> Always taking the write lock is unnecessary. I think we can keep the
>> old strategy of retrying with the write lock only when necessary. I
>> think this should work correctly as long as you lookup the VMA every
>> time after taking either the mmap read or write lock. The vma you
>> looked up should be valid as long as you hold that lock.
>>
>> As I pointed out above, if svm_range_from_addr finds a prange but the
>> VMA is missing, we can treat that as a special case and return
>> success (just draining a stale fault on a VMA that's being unmapped).
>
> ok, I will change svm_fault_allowed to return success if VMA is
> missing, it is simpler to handle this special race case, without
> taking mmap write lock.
>
> Regards,
>
> Philip
>
>>
>> Regards,
>>   Felix
>>
>>
>>> +
>>> +    vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>> +    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>> +        pr_debug("VMA not found for address 0x%llx\n", addr);
>>> +        mmap_write_downgrade(mm);
>>> +        r = -EFAULT;
>>> +        goto out_unlock_mm;
>>> +    }
>>>   -    mmap_read_lock(mm);
>>> -retry_write_locked:
>>>       mutex_lock(&svms->lock);
>>>       prange = svm_range_from_addr(svms, addr, NULL);
>>>       if (!prange) {
>>>           pr_debug("failed to find prange svms 0x%p address
>>> [0x%llx]\n",
>>>                svms, addr);
>>> -        if (!write_locked) {
>>> -            /* Need the write lock to create new range with MMU
>>> notifier.
>>> -             * Also flush pending deferred work to make sure the
>>> interval
>>> -             * tree is up to date before we add a new range
>>> -             */
>>> -            mutex_unlock(&svms->lock);
>>> -            mmap_read_unlock(mm);
>>> -            mmap_write_lock(mm);
>>> -            write_locked = true;
>>> -            goto retry_write_locked;
>>> -        }
>>> -        prange = svm_range_create_unregistered_range(adev, p, mm,
>>> addr);
>>> +        prange = svm_range_create_unregistered_range(adev, p, mm,
>>> vma, addr);
>>>           if (!prange) {
>>>               pr_debug("failed to create unregistered range svms
>>> 0x%p address [0x%llx]\n",
>>>                    svms, addr);
>>> @@ -2634,8 +2617,8 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>               goto out_unlock_svms;
>>>           }
>>>       }
>>> -    if (write_locked)
>>> -        mmap_write_downgrade(mm);
>>> +
>>> +    mmap_write_downgrade(mm);
>>>         mutex_lock(&prange->migrate_mutex);
>>>   @@ -2652,7 +2635,7 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>           goto out_unlock_range;
>>>       }
>>>   -    if (!svm_fault_allowed(mm, addr, write_fault)) {
>>> +    if (!svm_fault_allowed(vma, write_fault)) {
>>>           pr_debug("fault addr 0x%llx no %s permission\n", addr,
>>>               write_fault ? "write" : "read");
>>>           r = -EPERM;
>>> @@ -2704,10 +2687,10 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>       mutex_unlock(&prange->migrate_mutex);
>>>   out_unlock_svms:
>>>       mutex_unlock(&svms->lock);
>>> +out_unlock_mm:
>>>       mmap_read_unlock(mm);
>>>         svm_range_count_fault(adev, p, gpuidx);
>>> -
>>>   out:
>>>       kfd_unref_process(p);
>>>   

Re: [PATCH 2/3] drm/amdkfd: handle VMA remove race

[philip yang]

[-- Attachment #1: Type: text/html, Size: 17088 bytes --]

Re: [PATCH 2/3] drm/amdkfd: handle VMA remove race

[Felix Kuehling]

Am 2021-11-18 um 10:55 a.m. schrieb philip yang:
>
>
> On 2021-11-18 10:07 a.m., Felix Kuehling wrote:
>> Am 2021-11-18 um 10:00 a.m. schrieb philip yang:
>>> On 2021-11-17 7:10 p.m., Felix Kuehling wrote:
>>>> On 2021-11-16 10:43 p.m., Philip Yang wrote:
>>>>> VMA may be removed before unmap notifier callback, restore pages take
>>>>> mmap write lock to lookup VMA to avoid race,
>>>> The old code looked up the VMA after taking the mmap lock (either
>>>> read or write) and kept holding the lock afterwards. I think even
>>>> with your new code it's possible that the VMA disappears before you
>>>> take the lock the first time, so always taking the write lock only
>>>> reduces the time window in which things can go wrong, but it doesn't
>>>> remove the race.
>>> Take mmap write lock will serialize with __do_munmap,
>> __do_munmap runs with the mmap write lock. Taking the read lock should
>> be sufficient to serialize with it.
>
> __do_munmap takes mmap write lock to remove vma, then downgrade to
> read lock to call unmap_region.
>
Yes. But it does that after detaching the VMA. So holding the read lock
is sufficient to ensure that a VMA you have looked up remains valid and
that __do_munmap will not remove it.

Regards,
  Felix


> static int __vm_munmap(unsigned long start, size_t len, bool downgrade)
> {
>     int ret;
>     struct mm_struct *mm = current->mm;
>     LIST_HEAD(uf);
>
>     if (mmap_write_lock_killable(mm))
>         return -EINTR;
>
>     ret = __do_munmap(mm, start, len, &uf, downgrade);
>     /*
>      * Returning 1 indicates mmap_lock is downgraded.
>      * But 1 is not legal return value of vm_munmap() and munmap(), reset
>      * it to 0 before return.
>      */
>     if (ret == 1) {
>         mmap_read_unlock(mm);
>         ret = 0;
>     } else
>         mmap_write_unlock(mm);
>
> }
>
> int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
> {
>
> ...
>
>     /* Detach vmas from rbtree */
>     if (!detach_vmas_to_be_unmapped(mm, vma, prev, end))
>         downgrade = false;
>
>     if (downgrade)
>         mmap_write_downgrade(mm);
>
>     unmap_region(mm, vma, prev, start, end);
>
> }
>
> Regards,
>
> Philip
>
>> Regards,
>>   Felix
>>
>>
>>> to ensure vma remove and unmap callback are done, because unmap
>>> callback set drain_retryfaults flag, so we can safely drain the
>>> faults, and it is app bug if vma not found after taking mmap write lock.
>>>> I still struggle to understand the race you're trying to fix. The
>>>> only time the svm_restore_pages can see that the VMA is gone AND the
>>>> prange is gone is after the deferred worker has removed the prange.
>>>> But the fault draining in the deferred worker should prevent us from
>>>> ever seeing stale faults in that situation. That means, if no prange
>>>> is found and no VMA is found, it's definitely an application bug.
>>>>
>>>> The only possible race is in the case where the prange still exists
>>>> but the VMA is gone (needed by svm_fault_allowed). We can treat that
>>>> as a special case where we just return success because we know that
>>>> we're handling a stale fault for a VMA that's in the middle of being
>>>> unmapped. The fact that the prange still existed means that there
>>>> once was a valid mapping at the address but the deferred worker just
>>>> hasn't had a chance to clean it up yet.
>>>>
>>> Yes, that is only possible race.
>>>> One more comment inline.
>>>>
>>>>
>>>>>   and then create unregister
>>>>> new range and check VMA access permission, then downgrade to take mmap
>>>>> read lock to recover fault. Refactor code to avoid duplicate VMA
>>>>> lookup.
>>>>>
>>>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 65
>>>>> ++++++++++------------------
>>>>>   1 file changed, 24 insertions(+), 41 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>> index c1f367934428..3eb0a9491755 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>> @@ -2329,20 +2329,13 @@ svm_range_best_restore_location(struct
>>>>> svm_range *prange,
>>>>>   }
>>>>>     static int
>>>>> -svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
>>>>> -                   unsigned long *start, unsigned long *last,
>>>>> -                   bool *is_heap_stack)
>>>>> +svm_range_get_range_boundaries(struct kfd_process *p, struct
>>>>> vm_area_struct *vma,
>>>>> +                   int64_t addr, unsigned long *start,
>>>>> +                   unsigned long *last, bool *is_heap_stack)
>>>>>   {
>>>>> -    struct vm_area_struct *vma;
>>>>>       struct interval_tree_node *node;
>>>>>       unsigned long start_limit, end_limit;
>>>>>   -    vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>>>> -    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>>>> -        pr_debug("VMA does not exist in address [0x%llx]\n", addr);
>>>>> -        return -EFAULT;
>>>>> -    }
>>>>> -
>>>>>       *is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
>>>>>                 vma->vm_end >= vma->vm_mm->start_brk) ||
>>>>>                (vma->vm_start <= vma->vm_mm->start_stack &&
>>>>> @@ -2437,9 +2430,10 @@ svm_range_check_vm_userptr(struct kfd_process
>>>>> *p, uint64_t start, uint64_t last,
>>>>>     static struct
>>>>>   svm_range *svm_range_create_unregistered_range(struct
>>>>> amdgpu_device *adev,
>>>>> -                        struct kfd_process *p,
>>>>> -                        struct mm_struct *mm,
>>>>> -                        int64_t addr)
>>>>> +                           struct kfd_process *p,
>>>>> +                           struct mm_struct *mm,
>>>>> +                           struct vm_area_struct *vma,
>>>>> +                           int64_t addr)
>>>>>   {
>>>>>       struct svm_range *prange = NULL;
>>>>>       unsigned long start, last;
>>>>> @@ -2449,7 +2443,7 @@ svm_range
>>>>> *svm_range_create_unregistered_range(struct amdgpu_device *adev,
>>>>>       uint64_t bo_l = 0;
>>>>>       int r;
>>>>>   -    if (svm_range_get_range_boundaries(p, addr, &start, &last,
>>>>> +    if (svm_range_get_range_boundaries(p, vma, addr, &start, &last,
>>>>>                          &is_heap_stack))
>>>>>           return NULL;
>>>>>   @@ -2552,20 +2546,13 @@ svm_range_count_fault(struct amdgpu_device
>>>>> *adev, struct kfd_process *p,
>>>>>   }
>>>>>     static bool
>>>>> -svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool
>>>>> write_fault)
>>>>> +svm_fault_allowed(struct vm_area_struct *vma, bool write_fault)
>>>>>   {
>>>>>       unsigned long requested = VM_READ;
>>>>> -    struct vm_area_struct *vma;
>>>>>         if (write_fault)
>>>>>           requested |= VM_WRITE;
>>>>>   -    vma = find_vma(mm, addr << PAGE_SHIFT);
>>>>> -    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>>>> -        pr_debug("address 0x%llx VMA is removed\n", addr);
>>>>> -        return true;
>>>>> -    }
>>>>> -
>>>>>       pr_debug("requested 0x%lx, vma permission flags 0x%lx\n",
>>>>> requested,
>>>>>           vma->vm_flags);
>>>>>       return (vma->vm_flags & requested) == requested;
>>>>> @@ -2582,7 +2569,7 @@ svm_range_restore_pages(struct amdgpu_device
>>>>> *adev, unsigned int pasid,
>>>>>       uint64_t timestamp;
>>>>>       int32_t best_loc;
>>>>>       int32_t gpuidx = MAX_GPU_INSTANCE;
>>>>> -    bool write_locked = false;
>>>>> +    struct vm_area_struct *vma = NULL;
>>>>>       int r = 0;
>>>>>         if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
>>>>> @@ -2606,26 +2593,22 @@ svm_range_restore_pages(struct amdgpu_device
>>>>> *adev, unsigned int pasid,
>>>>>         /* mm is available because kfd_process_notifier_release
>>>>> drain fault */
>>>>>       mm = p->mm;
>>>>> +    mmap_write_lock(mm);
>>>> Always taking the write lock is unnecessary. I think we can keep the
>>>> old strategy of retrying with the write lock only when necessary. I
>>>> think this should work correctly as long as you lookup the VMA every
>>>> time after taking either the mmap read or write lock. The vma you
>>>> looked up should be valid as long as you hold that lock.
>>>>
>>>> As I pointed out above, if svm_range_from_addr finds a prange but the
>>>> VMA is missing, we can treat that as a special case and return
>>>> success (just draining a stale fault on a VMA that's being unmapped).
>>> ok, I will change svm_fault_allowed to return success if VMA is
>>> missing, it is simpler to handle this special race case, without
>>> taking mmap write lock.
>>>
>>> Regards,
>>>
>>> Philip
>>>
>>>> Regards,
>>>>   Felix
>>>>
>>>>
>>>>> +
>>>>> +    vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>>>> +    if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>>>> +        pr_debug("VMA not found for address 0x%llx\n", addr);
>>>>> +        mmap_write_downgrade(mm);
>>>>> +        r = -EFAULT;
>>>>> +        goto out_unlock_mm;
>>>>> +    }
>>>>>   -    mmap_read_lock(mm);
>>>>> -retry_write_locked:
>>>>>       mutex_lock(&svms->lock);
>>>>>       prange = svm_range_from_addr(svms, addr, NULL);
>>>>>       if (!prange) {
>>>>>           pr_debug("failed to find prange svms 0x%p address
>>>>> [0x%llx]\n",
>>>>>                svms, addr);
>>>>> -        if (!write_locked) {
>>>>> -            /* Need the write lock to create new range with MMU
>>>>> notifier.
>>>>> -             * Also flush pending deferred work to make sure the
>>>>> interval
>>>>> -             * tree is up to date before we add a new range
>>>>> -             */
>>>>> -            mutex_unlock(&svms->lock);
>>>>> -            mmap_read_unlock(mm);
>>>>> -            mmap_write_lock(mm);
>>>>> -            write_locked = true;
>>>>> -            goto retry_write_locked;
>>>>> -        }
>>>>> -        prange = svm_range_create_unregistered_range(adev, p, mm,
>>>>> addr);
>>>>> +        prange = svm_range_create_unregistered_range(adev, p, mm,
>>>>> vma, addr);
>>>>>           if (!prange) {
>>>>>               pr_debug("failed to create unregistered range svms
>>>>> 0x%p address [0x%llx]\n",
>>>>>                    svms, addr);
>>>>> @@ -2634,8 +2617,8 @@ svm_range_restore_pages(struct amdgpu_device
>>>>> *adev, unsigned int pasid,
>>>>>               goto out_unlock_svms;
>>>>>           }
>>>>>       }
>>>>> -    if (write_locked)
>>>>> -        mmap_write_downgrade(mm);
>>>>> +
>>>>> +    mmap_write_downgrade(mm);
>>>>>         mutex_lock(&prange->migrate_mutex);
>>>>>   @@ -2652,7 +2635,7 @@ svm_range_restore_pages(struct amdgpu_device
>>>>> *adev, unsigned int pasid,
>>>>>           goto out_unlock_range;
>>>>>       }
>>>>>   -    if (!svm_fault_allowed(mm, addr, write_fault)) {
>>>>> +    if (!svm_fault_allowed(vma, write_fault)) {
>>>>>           pr_debug("fault addr 0x%llx no %s permission\n", addr,
>>>>>               write_fault ? "write" : "read");
>>>>>           r = -EPERM;
>>>>> @@ -2704,10 +2687,10 @@ svm_range_restore_pages(struct amdgpu_device
>>>>> *adev, unsigned int pasid,
>>>>>       mutex_unlock(&prange->migrate_mutex);
>>>>>   out_unlock_svms:
>>>>>       mutex_unlock(&svms->lock);
>>>>> +out_unlock_mm:
>>>>>       mmap_read_unlock(mm);
>>>>>         svm_range_count_fault(adev, p, gpuidx);
>>>>> -
>>>>>   out:
>>>>>       kfd_unref_process(p);
>>>>>   

[PATCH 3/3] drm/amdkfd: simplify drain retry fault

[Philip Yang]

unmap range always set svms->drain_pagefaults flag to simplify both
parent range and child range unmap. Deferred list work takes mmap write
lock to read and clear svms->drain_pagefaults, to serialize with unmap
callback.

Add atomic flag svms->draining_faults, if svms->draining_faults is set,
page fault handle ignore the retry fault, to speed up interrupt handling.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c  | 24 ++++++++++++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 1d3f012bcd2a..4e4640b731e1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -767,6 +767,7 @@ struct svm_range_list {
 	spinlock_t			deferred_list_lock;
 	atomic_t			evicted_ranges;
 	bool				drain_pagefaults;
+	atomic_t			draining_faults;
 	struct delayed_work		restore_work;
 	DECLARE_BITMAP(bitmap_supported, MAX_GPU_INSTANCE);
 	struct task_struct 		*faulting_task;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 3eb0a9491755..d332462bf9d3 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1962,6 +1962,7 @@ void svm_range_drain_retry_fault(struct svm_range_list *svms)
 
 	p = container_of(svms, struct kfd_process, svms);
 
+	atomic_set(&svms->draining_faults, 1);
 	for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
 		pdd = p->pdds[i];
 		if (!pdd)
@@ -1975,6 +1976,7 @@ void svm_range_drain_retry_fault(struct svm_range_list *svms)
 		flush_work(&adev->irq.ih1_work);
 		pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms);
 	}
+	atomic_set(&svms->draining_faults, 0);
 }
 
 static void svm_range_deferred_list_work(struct work_struct *work)
@@ -2002,6 +2004,7 @@ static void svm_range_deferred_list_work(struct work_struct *work)
 	 * mmap write lock to serialize with munmap notifiers.
 	 */
 	if (unlikely(READ_ONCE(svms->drain_pagefaults))) {
+		atomic_set(&svms->draining_faults, 1);
 		WRITE_ONCE(svms->drain_pagefaults, false);
 		mmap_write_unlock(mm);
 		svm_range_drain_retry_fault(svms);
@@ -2049,12 +2052,6 @@ svm_range_add_list_work(struct svm_range_list *svms, struct svm_range *prange,
 			struct mm_struct *mm, enum svm_work_list_ops op)
 {
 	spin_lock(&svms->deferred_list_lock);
-	/* Make sure pending page faults are drained in the deferred worker
-	 * before the range is freed to avoid straggler interrupts on
-	 * unmapped memory causing "phantom faults".
-	 */
-	if (op == SVM_OP_UNMAP_RANGE)
-		svms->drain_pagefaults = true;
 	/* if prange is on the deferred list */
 	if (!list_empty(&prange->deferred_list)) {
 		pr_debug("update exist prange 0x%p work op %d\n", prange, op);
@@ -2133,6 +2130,13 @@ svm_range_unmap_from_cpu(struct mm_struct *mm, struct svm_range *prange,
 	pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] [0x%lx 0x%lx]\n", svms,
 		 prange, prange->start, prange->last, start, last);
 
+	/* Make sure pending page faults are drained in the deferred worker
+	 * before the range is freed to avoid straggler interrupts on
+	 * unmapped memory causing "phantom faults".
+	 */
+	pr_debug("set range drain_pagefaults true\n");
+	WRITE_ONCE(svms->drain_pagefaults, true);
+
 	unmap_parent = start <= prange->start && last >= prange->last;
 
 	list_for_each_entry(pchild, &prange->child_list, child_list) {
@@ -2595,6 +2599,13 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 	mm = p->mm;
 	mmap_write_lock(mm);
 
+	if (!!atomic_read(&svms->draining_faults) ||
+	    READ_ONCE(svms->drain_pagefaults)) {
+		pr_debug("draining retry fault, drop fault 0x%llx\n", addr);
+		mmap_write_downgrade(mm);
+		goto out_unlock_mm;
+	}
+
 	vma = find_vma(p->mm, addr << PAGE_SHIFT);
 	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
 		pr_debug("VMA not found for address 0x%llx\n", addr);
@@ -2732,6 +2743,7 @@ int svm_range_list_init(struct kfd_process *p)
 	mutex_init(&svms->lock);
 	INIT_LIST_HEAD(&svms->list);
 	atomic_set(&svms->evicted_ranges, 0);
+	atomic_set(&svms->draining_faults, 0);
 	INIT_DELAYED_WORK(&svms->restore_work, svm_range_restore_work);
 	INIT_WORK(&svms->deferred_list_work, svm_range_deferred_list_work);
 	INIT_LIST_HEAD(&svms->deferred_range_list);
-- 
2.17.1

Re: [PATCH 3/3] drm/amdkfd: simplify drain retry fault

[Felix Kuehling]

On 2021-11-16 10:43 p.m., Philip Yang wrote:
> unmap range always set svms->drain_pagefaults flag to simplify both
> parent range and child range unmap. Deferred list work takes mmap write
> lock to read and clear svms->drain_pagefaults, to serialize with unmap
> callback.
>
> Add atomic flag svms->draining_faults, if svms->draining_faults is set,
> page fault handle ignore the retry fault, to speed up interrupt handling.
Having both svms->drain_pagefaults and svms->draining_faults is 
confusing. Do we really need both?

Regards,
   Felix

>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h |  1 +
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c  | 24 ++++++++++++++++++------
>   2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 1d3f012bcd2a..4e4640b731e1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -767,6 +767,7 @@ struct svm_range_list {
>   	spinlock_t			deferred_list_lock;
>   	atomic_t			evicted_ranges;
>   	bool				drain_pagefaults;
> +	atomic_t			draining_faults;
>   	struct delayed_work		restore_work;
>   	DECLARE_BITMAP(bitmap_supported, MAX_GPU_INSTANCE);
>   	struct task_struct 		*faulting_task;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 3eb0a9491755..d332462bf9d3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1962,6 +1962,7 @@ void svm_range_drain_retry_fault(struct svm_range_list *svms)
>   
>   	p = container_of(svms, struct kfd_process, svms);
>   
> +	atomic_set(&svms->draining_faults, 1);
>   	for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
>   		pdd = p->pdds[i];
>   		if (!pdd)
> @@ -1975,6 +1976,7 @@ void svm_range_drain_retry_fault(struct svm_range_list *svms)
>   		flush_work(&adev->irq.ih1_work);
>   		pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms);
>   	}
> +	atomic_set(&svms->draining_faults, 0);
>   }
>   
>   static void svm_range_deferred_list_work(struct work_struct *work)
> @@ -2002,6 +2004,7 @@ static void svm_range_deferred_list_work(struct work_struct *work)
>   	 * mmap write lock to serialize with munmap notifiers.
>   	 */
>   	if (unlikely(READ_ONCE(svms->drain_pagefaults))) {
> +		atomic_set(&svms->draining_faults, 1);
>   		WRITE_ONCE(svms->drain_pagefaults, false);
>   		mmap_write_unlock(mm);
>   		svm_range_drain_retry_fault(svms);
> @@ -2049,12 +2052,6 @@ svm_range_add_list_work(struct svm_range_list *svms, struct svm_range *prange,
>   			struct mm_struct *mm, enum svm_work_list_ops op)
>   {
>   	spin_lock(&svms->deferred_list_lock);
> -	/* Make sure pending page faults are drained in the deferred worker
> -	 * before the range is freed to avoid straggler interrupts on
> -	 * unmapped memory causing "phantom faults".
> -	 */
> -	if (op == SVM_OP_UNMAP_RANGE)
> -		svms->drain_pagefaults = true;
>   	/* if prange is on the deferred list */
>   	if (!list_empty(&prange->deferred_list)) {
>   		pr_debug("update exist prange 0x%p work op %d\n", prange, op);
> @@ -2133,6 +2130,13 @@ svm_range_unmap_from_cpu(struct mm_struct *mm, struct svm_range *prange,
>   	pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] [0x%lx 0x%lx]\n", svms,
>   		 prange, prange->start, prange->last, start, last);
>   
> +	/* Make sure pending page faults are drained in the deferred worker
> +	 * before the range is freed to avoid straggler interrupts on
> +	 * unmapped memory causing "phantom faults".
> +	 */
> +	pr_debug("set range drain_pagefaults true\n");
> +	WRITE_ONCE(svms->drain_pagefaults, true);
> +
>   	unmap_parent = start <= prange->start && last >= prange->last;
>   
>   	list_for_each_entry(pchild, &prange->child_list, child_list) {
> @@ -2595,6 +2599,13 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   	mm = p->mm;
>   	mmap_write_lock(mm);
>   
> +	if (!!atomic_read(&svms->draining_faults) ||
> +	    READ_ONCE(svms->drain_pagefaults)) {
> +		pr_debug("draining retry fault, drop fault 0x%llx\n", addr);
> +		mmap_write_downgrade(mm);
> +		goto out_unlock_mm;
> +	}
> +
>   	vma = find_vma(p->mm, addr << PAGE_SHIFT);
>   	if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>   		pr_debug("VMA not found for address 0x%llx\n", addr);
> @@ -2732,6 +2743,7 @@ int svm_range_list_init(struct kfd_process *p)
>   	mutex_init(&svms->lock);
>   	INIT_LIST_HEAD(&svms->list);
>   	atomic_set(&svms->evicted_ranges, 0);
> +	atomic_set(&svms->draining_faults, 0);
>   	INIT_DELAYED_WORK(&svms->restore_work, svm_range_restore_work);
>   	INIT_WORK(&svms->deferred_list_work, svm_range_deferred_list_work);
>   	INIT_LIST_HEAD(&svms->deferred_range_list);

Re: [PATCH 3/3] drm/amdkfd: simplify drain retry fault

[philip yang]

[-- Attachment #1: Type: text/html, Size: 10569 bytes --]

Re: [PATCH 3/3] drm/amdkfd: simplify drain retry fault

[Felix Kuehling]

Am 2021-11-18 um 11:19 a.m. schrieb philip yang:
>
>
> On 2021-11-17 7:14 p.m., Felix Kuehling wrote:
>>
>> On 2021-11-16 10:43 p.m., Philip Yang wrote:
>>> unmap range always set svms->drain_pagefaults flag to simplify both
>>> parent range and child range unmap. Deferred list work takes mmap write
>>> lock to read and clear svms->drain_pagefaults, to serialize with unmap
>>> callback.
>>>
>>> Add atomic flag svms->draining_faults, if svms->draining_faults is set,
>>> page fault handle ignore the retry fault, to speed up interrupt
>>> handling.
>> Having both svms->drain_pagefaults and svms->draining_faults is
>> confusing. Do we really need both? 
>
> Using one flag, I can not find a way to handle the case, unmap new
> range. if the flag is set, restore_pages uses the flag to drop fault,
> then drain_retry_fault reset the flag after draining is done, we will
> not able to drain retry fault for the new range.
>
I think the correct solution would be to use atomic_inc to set the flag
and atomic_cmp_xchg in svm_range_drain_retry_fault to clear it. If the
flag was changed while svm_range_drain_retry_fault executed, it means
another drain was requested by someone else and the flag should remain
set for another round of draining.

Regards,
  Felix


> Regards,
>
> Philip
>
>> Regards,
>>   Felix
>>
>>>
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h |  1 +
>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c  | 24 ++++++++++++++++++------
>>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index 1d3f012bcd2a..4e4640b731e1 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> @@ -767,6 +767,7 @@ struct svm_range_list {
>>>       spinlock_t            deferred_list_lock;
>>>       atomic_t            evicted_ranges;
>>>       bool                drain_pagefaults;
>>> +    atomic_t            draining_faults;
>>>       struct delayed_work        restore_work;
>>>       DECLARE_BITMAP(bitmap_supported, MAX_GPU_INSTANCE);
>>>       struct task_struct         *faulting_task;
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 3eb0a9491755..d332462bf9d3 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -1962,6 +1962,7 @@ void svm_range_drain_retry_fault(struct
>>> svm_range_list *svms)
>>>         p = container_of(svms, struct kfd_process, svms);
>>>   +    atomic_set(&svms->draining_faults, 1);
>>>       for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
>>>           pdd = p->pdds[i];
>>>           if (!pdd)
>>> @@ -1975,6 +1976,7 @@ void svm_range_drain_retry_fault(struct
>>> svm_range_list *svms)
>>>           flush_work(&adev->irq.ih1_work);
>>>           pr_debug("drain retry fault gpu %d svms 0x%p done\n", i,
>>> svms);
>>>       }
>>> +    atomic_set(&svms->draining_faults, 0);
>>>   }
>>>     static void svm_range_deferred_list_work(struct work_struct *work)
>>> @@ -2002,6 +2004,7 @@ static void
>>> svm_range_deferred_list_work(struct work_struct *work)
>>>        * mmap write lock to serialize with munmap notifiers.
>>>        */
>>>       if (unlikely(READ_ONCE(svms->drain_pagefaults))) {
>>> +        atomic_set(&svms->draining_faults, 1);
>>>           WRITE_ONCE(svms->drain_pagefaults, false);
>>>           mmap_write_unlock(mm);
>>>           svm_range_drain_retry_fault(svms);
>>> @@ -2049,12 +2052,6 @@ svm_range_add_list_work(struct svm_range_list
>>> *svms, struct svm_range *prange,
>>>               struct mm_struct *mm, enum svm_work_list_ops op)
>>>   {
>>>       spin_lock(&svms->deferred_list_lock);
>>> -    /* Make sure pending page faults are drained in the deferred
>>> worker
>>> -     * before the range is freed to avoid straggler interrupts on
>>> -     * unmapped memory causing "phantom faults".
>>> -     */
>>> -    if (op == SVM_OP_UNMAP_RANGE)
>>> -        svms->drain_pagefaults = true;
>>>       /* if prange is on the deferred list */
>>>       if (!list_empty(&prange->deferred_list)) {
>>>           pr_debug("update exist prange 0x%p work op %d\n", prange,
>>> op);
>>> @@ -2133,6 +2130,13 @@ svm_range_unmap_from_cpu(struct mm_struct
>>> *mm, struct svm_range *prange,
>>>       pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] [0x%lx
>>> 0x%lx]\n", svms,
>>>            prange, prange->start, prange->last, start, last);
>>>   +    /* Make sure pending page faults are drained in the deferred
>>> worker
>>> +     * before the range is freed to avoid straggler interrupts on
>>> +     * unmapped memory causing "phantom faults".
>>> +     */
>>> +    pr_debug("set range drain_pagefaults true\n");
>>> +    WRITE_ONCE(svms->drain_pagefaults, true);
>>> +
>>>       unmap_parent = start <= prange->start && last >= prange->last;
>>>         list_for_each_entry(pchild, &prange->child_list, child_list) {
>>> @@ -2595,6 +2599,13 @@ svm_range_restore_pages(struct amdgpu_device
>>> *adev, unsigned int pasid,
>>>       mm = p->mm;
>>>       mmap_write_lock(mm);
>>>   +    if (!!atomic_read(&svms->draining_faults) ||
>>> +        READ_ONCE(svms->drain_pagefaults)) {
>>> +        pr_debug("draining retry fault, drop fault 0x%llx\n", addr);
>>> +        mmap_write_downgrade(mm);
>>> +        goto out_unlock_mm;
>>> +    }
>>> +
>>>       vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>>       if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>>           pr_debug("VMA not found for address 0x%llx\n", addr);
>>> @@ -2732,6 +2743,7 @@ int svm_range_list_init(struct kfd_process *p)
>>>       mutex_init(&svms->lock);
>>>       INIT_LIST_HEAD(&svms->list);
>>>       atomic_set(&svms->evicted_ranges, 0);
>>> +    atomic_set(&svms->draining_faults, 0);
>>>       INIT_DELAYED_WORK(&svms->restore_work, svm_range_restore_work);
>>>       INIT_WORK(&svms->deferred_list_work,
>>> svm_range_deferred_list_work);
>>>       INIT_LIST_HEAD(&svms->deferred_range_list);

Re: [PATCH 3/3] drm/amdkfd: simplify drain retry fault

[philip yang]

[-- Attachment #1: Type: text/html, Size: 9521 bytes --]

Re: [PATCH 1/3] drm/amdkfd: process exit and retry fault race

[Felix Kuehling]

On 2021-11-16 10:43 p.m., Philip Yang wrote:
> 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.

This fixes the problem, but it will result in waiting longer than 
necessary because the worker only finishes when the IH ring is empty.


>
> restore pages can not call mmget because mmput may call mmu notifier
> release to cause deadlock.

See my comment inline.


>
> 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 <Philip.Yang@amd.com>
> ---
>   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)) {

get_task_mm would be safer than relying on p->mm. I regret ever adding 
that to the process structure.


> +		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 */
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.

Regards,
   Felix


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

Re: [PATCH 1/3] drm/amdkfd: process exit and retry fault race

[philip yang]

[-- Attachment #1: Type: text/html, Size: 17486 bytes --]