* [PATCH v2 2/3] drm/amdkfd: handle VMA remove race
2021-11-19 22:29 [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race Philip Yang
@ 2021-11-19 22:29 ` Philip Yang
2021-11-19 22:29 ` [PATCH v2 3/3] drm/amdkfd: simplify drain retry fault Philip Yang
2021-11-23 2:29 ` [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race Felix Kuehling
2 siblings, 0 replies; 4+ messages in thread
From: Philip Yang @ 2021-11-19 22:29 UTC (permalink / raw)
To: amd-gfx; +Cc: Philip Yang, Felix.Kuehling
VMA may be removed before unmap notifier callback, and deferred list
work remove range, return success for this special case as we are
handling stale retry fault.
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 5fa540828ed0..65daae9e4042 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2548,20 +2548,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;
@@ -2579,6 +2572,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
int32_t best_loc;
int32_t gpuidx = MAX_GPU_INSTANCE;
bool write_locked = false;
+ struct vm_area_struct *vma;
int r = 0;
if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
@@ -2654,7 +2648,17 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
goto out_unlock_range;
}
- if (!svm_fault_allowed(mm, addr, write_fault)) {
+ /* __do_munmap removed VMA, return success as we are handling stale
+ * retry fault.
+ */
+ 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);
+ r = 0;
+ goto out_unlock_range;
+ }
+
+ if (!svm_fault_allowed(vma, write_fault)) {
pr_debug("fault addr 0x%llx no %s permission\n", addr,
write_fault ? "write" : "read");
r = -EPERM;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 3/3] drm/amdkfd: simplify drain retry fault
2021-11-19 22:29 [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race Philip Yang
2021-11-19 22:29 ` [PATCH v2 2/3] drm/amdkfd: handle VMA remove race Philip Yang
@ 2021-11-19 22:29 ` Philip Yang
2021-11-23 2:29 ` [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race Felix Kuehling
2 siblings, 0 replies; 4+ messages in thread
From: Philip Yang @ 2021-11-19 22:29 UTC (permalink / raw)
To: amd-gfx; +Cc: Philip Yang, Felix.Kuehling
unmap range always increase atomic svms->drain_pagefaults to simplify
both parent range and child range unmap, page fault handle ignores the
retry fault if svms->drain_pagefaults is set to speed up interrupt
handling. svm_range_drain_retry_fault restart draining if another
range unmap from cpu.
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 30 ++++++++++++++++++++-------
2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 836ec8860c1b..7ea528941951 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -767,7 +767,7 @@ struct svm_range_list {
struct list_head deferred_range_list;
spinlock_t deferred_list_lock;
atomic_t evicted_ranges;
- bool drain_pagefaults;
+ atomic_t drain_pagefaults;
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 65daae9e4042..99f38ffbf43b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1957,10 +1957,16 @@ static void svm_range_drain_retry_fault(struct svm_range_list *svms)
{
struct kfd_process_device *pdd;
struct kfd_process *p;
+ int drain;
uint32_t i;
p = container_of(svms, struct kfd_process, svms);
+restart:
+ drain = atomic_read(&svms->drain_pagefaults);
+ if (!drain)
+ return;
+
for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
pdd = p->pdds[i];
if (!pdd)
@@ -1972,6 +1978,8 @@ static void svm_range_drain_retry_fault(struct svm_range_list *svms)
&pdd->dev->adev->irq.ih1);
pr_debug("drain retry fault gpu %d svms 0x%p done\n", i, svms);
}
+ if (atomic_cmpxchg(&svms->drain_pagefaults, drain, 0) != drain)
+ goto restart;
}
static void svm_range_deferred_list_work(struct work_struct *work)
@@ -1997,8 +2005,7 @@ static void svm_range_deferred_list_work(struct work_struct *work)
/* 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);
+ if (unlikely(atomic_read(&svms->drain_pagefaults))) {
mmap_write_unlock(mm);
svm_range_drain_retry_fault(svms);
goto retry;
@@ -2045,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);
@@ -2129,6 +2130,12 @@ 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".
+ */
+ atomic_inc(&svms->drain_pagefaults);
+
unmap_parent = start <= prange->start && last >= prange->last;
list_for_each_entry(pchild, &prange->child_list, child_list) {
@@ -2594,6 +2601,11 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
pr_debug("restoring svms 0x%p fault address 0x%llx\n", svms, addr);
+ if (atomic_read(&svms->drain_pagefaults)) {
+ pr_debug("draining retry fault, drop fault 0x%llx\n", addr);
+ goto out;
+ }
+
/* p->lead_thread is available as kfd_process_wq_release flush the work
* before releasing task ref.
*/
@@ -2740,6 +2752,7 @@ void svm_range_list_fini(struct kfd_process *p)
* Ensure no retry fault comes in afterwards, as page fault handler will
* not find kfd process and take mm lock to recover fault.
*/
+ atomic_inc(&p->svms.drain_pagefaults);
svm_range_drain_retry_fault(&p->svms);
@@ -2763,6 +2776,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->drain_pagefaults, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race
2021-11-19 22:29 [PATCH v2 1/3] drm/amdkfd: process exit and retry fault race Philip Yang
2021-11-19 22:29 ` [PATCH v2 2/3] drm/amdkfd: handle VMA remove race Philip Yang
2021-11-19 22:29 ` [PATCH v2 3/3] drm/amdkfd: simplify drain retry fault Philip Yang
@ 2021-11-23 2:29 ` Felix Kuehling
2 siblings, 0 replies; 4+ messages in thread
From: Felix Kuehling @ 2021-11-23 2:29 UTC (permalink / raw)
To: Philip Yang, amd-gfx
Am 2021-11-19 um 5:29 p.m. schrieb Philip Yang:
> kfd_process_wq_release 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.
>
> Refactor deferred list work to get_task_mm and take mmap write lock
> to handle all ranges, and avoid mm is gone while inserting mmu notifier.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
The series is
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 61 ++++++++++++++++------------
> 1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 9e566ec54cf5..5fa540828ed0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1979,43 +1979,42 @@ 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);
> + /* Avoid mm is gone when inserting mmu notifier */
> + mm = get_task_mm(p->lead_thread);
> + if (!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 +2030,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,10 +2600,12 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>
> pr_debug("restoring svms 0x%p fault address 0x%llx\n", svms, addr);
>
> + /* p->lead_thread is available as kfd_process_wq_release flush the work
> + * before releasing task ref.
> + */
> mm = get_task_mm(p->lead_thread);
> if (!mm) {
> pr_debug("svms 0x%p failed to get mm\n", svms);
> - r = -ESRCH;
> goto out;
> }
>
> @@ -2730,6 +2732,13 @@ void svm_range_list_fini(struct kfd_process *p)
> /* Ensure list work is finished before process is destroyed */
> flush_work(&p->svms.deferred_list_work);
>
> + /*
> + * 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);
> +
> +
> list_for_each_entry_safe(prange, next, &p->svms.list, list) {
> svm_range_unlink(prange);
> svm_range_remove_notifier(prange);
^ permalink raw reply [flat|nested] 4+ messages in thread