[PATCH 1/4] drm/amdkfd: ratelimited svm debug messages

[PATCH 1/4] drm/amdkfd: ratelimited svm debug messages

[Philip Yang]

No function change, use pr_debug_ratelimited to avoid per page debug
message overflowing dmesg buf and console log.

use dev_err to show error message from unexpected situation, to provide
clue to help debug without enabling dynamic debug log.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 34 ++++++++++++------------
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 12 ++++-----
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index f53e17a94ad8..069422337cf7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -151,14 +151,14 @@ svm_migrate_copy_memory_gart(struct amdgpu_device *adev, dma_addr_t *sys,
 			gart_d = svm_migrate_direct_mapping_addr(adev, *vram);
 		}
 		if (r) {
-			pr_debug("failed %d to create gart mapping\n", r);
+			dev_err(adev->dev, "fail %d create gart mapping\n", r);
 			goto out_unlock;
 		}
 
 		r = amdgpu_copy_buffer(ring, gart_s, gart_d, size * PAGE_SIZE,
 				       NULL, &next, false, true, false);
 		if (r) {
-			pr_debug("failed %d to copy memory\n", r);
+			dev_err(adev->dev, "fail %d to copy memory\n", r);
 			goto out_unlock;
 		}
 
@@ -285,7 +285,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 
 	r = svm_range_vram_node_new(adev, prange, true);
 	if (r) {
-		pr_debug("failed %d get 0x%llx pages from vram\n", r, npages);
+		dev_err(adev->dev, "fail %d get %lld vram pages\n", r, npages);
 		goto out;
 	}
 
@@ -305,7 +305,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 					      DMA_TO_DEVICE);
 			r = dma_mapping_error(dev, src[i]);
 			if (r) {
-				pr_debug("failed %d dma_map_page\n", r);
+				dev_err(adev->dev, "fail %d dma_map_page\n", r);
 				goto out_free_vram_pages;
 			}
 		} else {
@@ -325,8 +325,8 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 			continue;
 		}
 
-		pr_debug("dma mapping src to 0x%llx, page_to_pfn 0x%lx\n",
-			 src[i] >> PAGE_SHIFT, page_to_pfn(spage));
+		pr_debug_ratelimited("dma mapping src to 0x%llx, pfn 0x%lx\n",
+				     src[i] >> PAGE_SHIFT, page_to_pfn(spage));
 
 		if (j >= (cursor.size >> PAGE_SHIFT) - 1 && i < npages - 1) {
 			r = svm_migrate_copy_memory_gart(adev, src + i - j,
@@ -347,7 +347,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 
 out_free_vram_pages:
 	if (r) {
-		pr_debug("failed %d to copy memory to vram\n", r);
+		dev_err(adev->dev, "fail %d to copy memory to vram\n", r);
 		while (i--) {
 			svm_migrate_put_vram_page(adev, dst[i]);
 			migrate->dst[i] = 0;
@@ -405,8 +405,8 @@ svm_migrate_vma_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 
 	r = migrate_vma_setup(&migrate);
 	if (r) {
-		pr_debug("failed %d prepare migrate svms 0x%p [0x%lx 0x%lx]\n",
-			 r, prange->svms, prange->start, prange->last);
+		dev_err(adev->dev, "fail %d vma setup 0x%p [0x%lx 0x%lx]\n",
+			r, prange->svms, prange->start, prange->last);
 		goto out_free;
 	}
 	if (migrate.cpages != npages) {
@@ -506,7 +506,7 @@ static void svm_migrate_page_free(struct page *page)
 	struct svm_range_bo *svm_bo = page->zone_device_data;
 
 	if (svm_bo) {
-		pr_debug("svm_bo ref left: %d\n", kref_read(&svm_bo->kref));
+		pr_debug_ratelimited("ref: %d\n", kref_read(&svm_bo->kref));
 		svm_range_bo_unref(svm_bo);
 	}
 }
@@ -563,8 +563,8 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
 
 		dpage = svm_migrate_get_sys_page(migrate->vma, addr);
 		if (!dpage) {
-			pr_debug("failed get page svms 0x%p [0x%lx 0x%lx]\n",
-				 prange->svms, prange->start, prange->last);
+			dev_err(adev->dev, "fail get page 0x%p [0x%lx 0x%lx]\n",
+				prange->svms, prange->start, prange->last);
 			r = -ENOMEM;
 			goto out_oom;
 		}
@@ -572,12 +572,12 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
 		dst[i] = dma_map_page(dev, dpage, 0, PAGE_SIZE, DMA_FROM_DEVICE);
 		r = dma_mapping_error(dev, dst[i]);
 		if (r) {
-			pr_debug("failed %d dma_map_page\n", r);
+			dev_err(adev->dev, "fail %d dma_map_page\n", r);
 			goto out_oom;
 		}
 
-		pr_debug("dma mapping dst to 0x%llx, page_to_pfn 0x%lx\n",
-			      dst[i] >> PAGE_SHIFT, page_to_pfn(dpage));
+		pr_debug_ratelimited("dma mapping dst to 0x%llx, pfn 0x%lx\n",
+				     dst[i] >> PAGE_SHIFT, page_to_pfn(dpage));
 
 		migrate->dst[i] = migrate_pfn(page_to_pfn(dpage));
 		migrate->dst[i] |= MIGRATE_PFN_LOCKED;
@@ -631,8 +631,8 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
 
 	r = migrate_vma_setup(&migrate);
 	if (r) {
-		pr_debug("failed %d prepare migrate svms 0x%p [0x%lx 0x%lx]\n",
-			 r, prange->svms, prange->start, prange->last);
+		dev_err(adev->dev, "fail %d vma setup 0x%p [0x%lx 0x%lx]\n",
+			r, prange->svms, prange->start, prange->last);
 		goto out_free;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 7c42a0d4e0de..7f0743fcfcc3 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -158,17 +158,17 @@ svm_range_dma_map_dev(struct amdgpu_device *adev, struct svm_range *prange,
 				   bo_adev->vm_manager.vram_base_offset -
 				   bo_adev->kfd.dev->pgmap.range.start;
 			addr[i] |= SVM_RANGE_VRAM_DOMAIN;
-			pr_debug("vram address detected: 0x%llx\n", addr[i]);
+			pr_debug_ratelimited("vram address: 0x%llx\n", addr[i]);
 			continue;
 		}
 		addr[i] = dma_map_page(dev, page, 0, PAGE_SIZE, dir);
 		r = dma_mapping_error(dev, addr[i]);
 		if (r) {
-			pr_debug("failed %d dma_map_page\n", r);
+			pr_err("failed %d dma_map_page\n", r);
 			return r;
 		}
-		pr_debug("dma mapping 0x%llx for page addr 0x%lx\n",
-			 addr[i] >> PAGE_SHIFT, page_to_pfn(page));
+		pr_debug_ratelimited("dma mapping 0x%llx for page addr 0x%lx\n",
+				     addr[i] >> PAGE_SHIFT, page_to_pfn(page));
 	}
 	return 0;
 }
@@ -217,7 +217,7 @@ void svm_range_dma_unmap(struct device *dev, dma_addr_t *dma_addr,
 	for (i = offset; i < offset + npages; i++) {
 		if (!svm_is_valid_dma_mapping_addr(dev, dma_addr[i]))
 			continue;
-		pr_debug("dma unmapping 0x%llx\n", dma_addr[i] >> PAGE_SHIFT);
+		pr_debug_ratelimited("unmap 0x%llx\n", dma_addr[i] >> PAGE_SHIFT);
 		dma_unmap_page(dev, dma_addr[i], PAGE_SIZE, dir);
 		dma_addr[i] = 0;
 	}
@@ -1459,7 +1459,7 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 		/* This should never happen. actual_loc gets set by
 		 * svm_migrate_ram_to_vram after allocating a BO.
 		 */
-		WARN(1, "VRAM BO missing during validation\n");
+		WARN_ONCE(1, "VRAM BO missing during validation\n");
 		return -EINVAL;
 	}
 
-- 
2.17.1

[PATCH 2/4] drm/amdkfd: handle partial migration cpages 0

[Philip Yang]

migrate_vma_setup may return cpages 0, means 0 page can be migrated,
treat this as error case to skip the rest of migration steps, and don't
change prange actual loc, to avoid warning message "VRAM BO missing
during validation".

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 48 ++++++++++++++----------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index 069422337cf7..9b68e3e8f2a1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -409,20 +409,25 @@ svm_migrate_vma_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
 			r, prange->svms, prange->start, prange->last);
 		goto out_free;
 	}
-	if (migrate.cpages != npages) {
-		pr_debug("Partial migration. 0x%lx/0x%llx pages can be migrated\n",
-			 migrate.cpages,
-			 npages);
-	}
 
-	if (migrate.cpages) {
-		r = svm_migrate_copy_to_vram(adev, prange, &migrate, &mfence,
-					     scratch);
-		migrate_vma_pages(&migrate);
-		svm_migrate_copy_done(adev, mfence);
-		migrate_vma_finalize(&migrate);
+	if (migrate.cpages != npages)
+		pr_debug("partial migration, 0x%lx/0x%llx pages migrated\n",
+			 migrate.cpages, npages);
+	else
+		pr_debug("0x%lx pages migrated\n", migrate.cpages);
+
+	if (!migrate.cpages) {
+		pr_debug("failed collect migrate sys pages [0x%lx 0x%lx]\n",
+			 prange->start, prange->last);
+		r = -ENOMEM;
+		goto out_free;
 	}
 
+	r = svm_migrate_copy_to_vram(adev, prange, &migrate, &mfence, scratch);
+	migrate_vma_pages(&migrate);
+	svm_migrate_copy_done(adev, mfence);
+	migrate_vma_finalize(&migrate);
+
 	svm_range_dma_unmap(adev->dev, scratch, 0, npages);
 	svm_range_free_dma_mappings(prange);
 
@@ -636,19 +641,24 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
 		goto out_free;
 	}
 
-	pr_debug("cpages %ld\n", migrate.cpages);
+	if (migrate.cpages != npages)
+		pr_debug("partial migration, 0x%lx/0x%llx pages migrated\n",
+			 migrate.cpages, npages);
+	else
+		pr_debug("0x%lx pages migrated\n", migrate.cpages);
 
-	if (migrate.cpages) {
-		r = svm_migrate_copy_to_ram(adev, prange, &migrate, &mfence,
-					    scratch, npages);
-		migrate_vma_pages(&migrate);
-		svm_migrate_copy_done(adev, mfence);
-		migrate_vma_finalize(&migrate);
-	} else {
+	if (!migrate.cpages) {
 		pr_debug("failed collect migrate device pages [0x%lx 0x%lx]\n",
 			 prange->start, prange->last);
+		r = -ENOMEM;
+		goto out_free;
 	}
 
+	r = svm_migrate_copy_to_ram(adev, prange, &migrate, &mfence,
+				    scratch, npages);
+	migrate_vma_pages(&migrate);
+	svm_migrate_copy_done(adev, mfence);
+	migrate_vma_finalize(&migrate);
 	svm_range_dma_unmap(adev->dev, scratch, 0, npages);
 
 out_free:
-- 
2.17.1

[PATCH 3/4] drm/amdkfd: avoid svm conflicting with userptr address

[Philip Yang]

For ioctl_alloc_memory_of_gpu to alloc userptr bo, check userptr address
does not exist in svm->objects.

For svm range allocation, look for address in the userptr range of
interval tree VA nodes.

Change helper svm_range_check_vm to return amdgpu_bo, which will be used
to avoid creating new svm range overlap with bo later.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++++---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 55 +++++++++++++++++++-----
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index f1e7edeb4e6b..34dfa6a938bf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1255,6 +1255,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
 	long err;
 	uint64_t offset = args->mmap_offset;
 	uint32_t flags = args->flags;
+	unsigned long start, last;
 
 	if (args->size == 0)
 		return -EINVAL;
@@ -1266,11 +1267,17 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
 	svm_range_list_lock_and_flush_work(&p->svms, current->mm);
 	mutex_lock(&p->svms.lock);
 	mmap_write_unlock(current->mm);
-	if (interval_tree_iter_first(&p->svms.objects,
-				     args->va_addr >> PAGE_SHIFT,
-				     (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
-		pr_err("Address: 0x%llx already allocated by SVM\n",
-			args->va_addr);
+
+	if (flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) {
+		start = args->mmap_offset >> PAGE_SHIFT;
+		last = (args->mmap_offset + args->size - 1) >> PAGE_SHIFT;
+	} else {
+		start = args->va_addr >> PAGE_SHIFT;
+		last = (args->va_addr + args->size - 1) >> PAGE_SHIFT;
+	}
+
+	if (interval_tree_iter_first(&p->svms.objects, start, last)) {
+		pr_err("[0x%lx 0x%lx] already allocated by SVM\n", start, last);
 		mutex_unlock(&p->svms.lock);
 		return -EADDRINUSE;
 	}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 7f0743fcfcc3..d49c08618714 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2679,15 +2679,18 @@ int svm_range_list_init(struct kfd_process *p)
  *
  * Context: Process context
  *
- * Return 0 - OK, if the range is not mapped.
+ * Return NULL - if the range is not mapped.
+ * amdgpu_bo - if the range is mapped by old API
  * Otherwise error code:
- * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
  * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
  * a signal. Release all buffer reservations and return to user-space.
  */
-static int
+static struct amdgpu_bo *
 svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
 {
+	struct amdgpu_bo_va_mapping *mapping;
+	struct interval_tree_node *node;
+	struct amdgpu_bo *bo = NULL;
 	uint32_t i;
 	int r;
 
@@ -2700,16 +2703,42 @@ svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
 		vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
 		r = amdgpu_bo_reserve(vm->root.bo, false);
 		if (r)
-			return r;
-		if (interval_tree_iter_first(&vm->va, start, last)) {
-			pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
-			amdgpu_bo_unreserve(vm->root.bo);
-			return -EADDRINUSE;
+			return ERR_PTR(r);
+		node = interval_tree_iter_first(&vm->va, start, last);
+		if (node) {
+			pr_debug("range [0x%llx 0x%llx] already TTM mapped\n",
+				 start, last);
+			mapping = container_of((struct rb_node *)node,
+					       struct amdgpu_bo_va_mapping, rb);
+			bo = mapping->bo_va->base.bo;
+			goto out_unreserve;
+		}
+
+		/* Check userptr by searching entire vm->va interval tree */
+		node = interval_tree_iter_first(&vm->va, 0, ~0ULL);
+		while (node) {
+			mapping = container_of((struct rb_node *)node,
+					       struct amdgpu_bo_va_mapping, rb);
+			bo = mapping->bo_va->base.bo;
+
+			if (amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm,
+							 start << PAGE_SHIFT,
+							 last << PAGE_SHIFT)) {
+				pr_debug("[0x%llx 0x%llx] userptr mapped\n",
+					 start, last);
+				goto out_unreserve;
+			}
+			bo = NULL;
+			node = interval_tree_iter_next(node, 0, ~0ULL);
 		}
+
+out_unreserve:
 		amdgpu_bo_unreserve(vm->root.bo);
+		if (bo)
+			break;
 	}
 
-	return 0;
+	return bo;
 }
 
 /**
@@ -2732,6 +2761,7 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
 	struct vm_area_struct *vma;
 	unsigned long end;
 	unsigned long start_unchg = start;
+	struct amdgpu_bo *bo;
 
 	start <<= PAGE_SHIFT;
 	end = start + (size << PAGE_SHIFT);
@@ -2743,7 +2773,12 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
 		start = min(end, vma->vm_end);
 	} while (start < end);
 
-	return svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
+	bo = svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
+	if (IS_ERR(bo))
+		return PTR_ERR(bo);
+	if (bo)
+		return -EADDRINUSE;
+	return 0;
 }
 
 /**
-- 
2.17.1

[PATCH 4/4] drm/amdkfd: create svm unregister range not overlap with userptr

[Philip Yang]

When creating new svm range to recover retry fault, avoid svm range
to overlap with ranges or userptr ranges managed by TTM, otherwise
svm migration will trigger TTM or userptr eviction, to evict user queues
unexpectedly.

Add helper amdgpu_ttm_tt_get_userptr because amdgpu_ttm_tt structure is
not accessed from outside of amdgpu_ttm.c.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 28 ++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index e2896ac2c9ce..93952e1bce5b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1251,6 +1251,19 @@ bool amdgpu_ttm_tt_is_userptr(struct ttm_tt *ttm)
 	return true;
 }
 
+/*
+ * amdgpu_ttm_tt_get_userptr - get userptr of the address range
+ */
+uint64_t amdgpu_ttm_tt_get_userptr(struct ttm_tt *ttm)
+{
+	struct amdgpu_ttm_tt *gtt = (void *)ttm;
+
+	if (gtt == NULL)
+		return 0;
+	return  gtt->userptr;
+}
+
+
 /*
  * amdgpu_ttm_tt_is_readonly - Is the ttm_tt object read only?
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index e69f3e8e06e5..1dd1a882280d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -186,6 +186,7 @@ bool amdgpu_ttm_tt_affect_userptr(struct ttm_tt *ttm, unsigned long start,
 bool amdgpu_ttm_tt_userptr_invalidated(struct ttm_tt *ttm,
 				       int *last_invalidated);
 bool amdgpu_ttm_tt_is_userptr(struct ttm_tt *ttm);
+uint64_t amdgpu_ttm_tt_get_userptr(struct ttm_tt *ttm);
 bool amdgpu_ttm_tt_is_readonly(struct ttm_tt *ttm);
 uint64_t amdgpu_ttm_tt_pde_flags(struct ttm_tt *ttm, struct ttm_resource *mem);
 uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_tt *ttm,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index d49c08618714..a2eb21deb06f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -45,6 +45,8 @@ static bool
 svm_range_cpu_invalidate_pagetables(struct mmu_interval_notifier *mni,
 				    const struct mmu_notifier_range *range,
 				    unsigned long cur_seq);
+static struct amdgpu_bo *
+svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last);
 
 static const struct mmu_interval_notifier_ops svm_range_mn_ops = {
 	.invalidate = svm_range_cpu_invalidate_pagetables,
@@ -2308,6 +2310,7 @@ svm_range_best_restore_location(struct svm_range *prange,
 
 	return -1;
 }
+
 static int
 svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
 				unsigned long *start, unsigned long *last)
@@ -2355,8 +2358,8 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
 		  vma->vm_end >> PAGE_SHIFT, *last);
 
 	return 0;
-
 }
+
 static struct
 svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
 						struct kfd_process *p,
@@ -2366,10 +2369,33 @@ svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
 	struct svm_range *prange = NULL;
 	unsigned long start, last;
 	uint32_t gpuid, gpuidx;
+	struct amdgpu_bo *bo;
 
 	if (svm_range_get_range_boundaries(p, addr, &start, &last))
 		return NULL;
 
+	bo = svm_range_check_vm(p, start, last);
+	if (bo) {
+		struct ttm_tt *ttm = bo->tbo.ttm;
+		unsigned long bo_s, bo_l;
+
+		if (amdgpu_ttm_tt_is_userptr(ttm)) {
+			bo_s = amdgpu_ttm_tt_get_userptr(ttm) >> PAGE_SHIFT;
+			bo_l = bo_s + ttm->num_pages - 1;
+			pr_debug("overlap userptr [0x%lx 0x%lx]\n", bo_s, bo_l);
+		} else {
+			bo_s = bo->kfd_bo->va;
+			bo_l = bo_s + ttm->num_pages - 1;
+			pr_debug("overlap range [0x%lx 0x%lx]\n", bo_s, bo_l);
+		}
+		if (addr >= bo_s && addr <= bo_l)
+			return NULL;
+		if (addr < bo_s)
+			last = bo_s - 1;
+		if (addr > bo_l)
+			start = bo_l + 1;
+	}
+
 	prange = svm_range_new(&p->svms, start, last);
 	if (!prange) {
 		pr_debug("Failed to create prange in address [0x%llx]\n", addr);
-- 
2.17.1

Re: [PATCH 3/4] drm/amdkfd: avoid svm conflicting with userptr address

[Felix Kuehling]

Am 2021-10-06 um 10:32 a.m. schrieb Philip Yang:
> For ioctl_alloc_memory_of_gpu to alloc userptr bo, check userptr address
> does not exist in svm->objects.
>
> For svm range allocation, look for address in the userptr range of
> interval tree VA nodes.

Why? The purpose of the check is to prevent the same GPU virtual address
being managed by the two different memory managers. So checking
args->va_addr should be correct even for userptr BOs. The CPU virtual
address should be OK to be mapped with userptr and SVM at the same time
as long as the userptr uses a distinct GPU virtual address.

Regards,
  Felix


>
> Change helper svm_range_check_vm to return amdgpu_bo, which will be used
> to avoid creating new svm range overlap with bo later.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++++---
>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 55 +++++++++++++++++++-----
>  2 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index f1e7edeb4e6b..34dfa6a938bf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1255,6 +1255,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>  	long err;
>  	uint64_t offset = args->mmap_offset;
>  	uint32_t flags = args->flags;
> +	unsigned long start, last;
>  
>  	if (args->size == 0)
>  		return -EINVAL;
> @@ -1266,11 +1267,17 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>  	svm_range_list_lock_and_flush_work(&p->svms, current->mm);
>  	mutex_lock(&p->svms.lock);
>  	mmap_write_unlock(current->mm);
> -	if (interval_tree_iter_first(&p->svms.objects,
> -				     args->va_addr >> PAGE_SHIFT,
> -				     (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
> -		pr_err("Address: 0x%llx already allocated by SVM\n",
> -			args->va_addr);
> +
> +	if (flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) {
> +		start = args->mmap_offset >> PAGE_SHIFT;
> +		last = (args->mmap_offset + args->size - 1) >> PAGE_SHIFT;
> +	} else {
> +		start = args->va_addr >> PAGE_SHIFT;
> +		last = (args->va_addr + args->size - 1) >> PAGE_SHIFT;
> +	}
> +
> +	if (interval_tree_iter_first(&p->svms.objects, start, last)) {
> +		pr_err("[0x%lx 0x%lx] already allocated by SVM\n", start, last);
>  		mutex_unlock(&p->svms.lock);
>  		return -EADDRINUSE;
>  	}
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 7f0743fcfcc3..d49c08618714 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2679,15 +2679,18 @@ int svm_range_list_init(struct kfd_process *p)
>   *
>   * Context: Process context
>   *
> - * Return 0 - OK, if the range is not mapped.
> + * Return NULL - if the range is not mapped.
> + * amdgpu_bo - if the range is mapped by old API
>   * Otherwise error code:
> - * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
>   * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
>   * a signal. Release all buffer reservations and return to user-space.
>   */
> -static int
> +static struct amdgpu_bo *
>  svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
>  {
> +	struct amdgpu_bo_va_mapping *mapping;
> +	struct interval_tree_node *node;
> +	struct amdgpu_bo *bo = NULL;
>  	uint32_t i;
>  	int r;
>  
> @@ -2700,16 +2703,42 @@ svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
>  		vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
>  		r = amdgpu_bo_reserve(vm->root.bo, false);
>  		if (r)
> -			return r;
> -		if (interval_tree_iter_first(&vm->va, start, last)) {
> -			pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
> -			amdgpu_bo_unreserve(vm->root.bo);
> -			return -EADDRINUSE;
> +			return ERR_PTR(r);
> +		node = interval_tree_iter_first(&vm->va, start, last);
> +		if (node) {
> +			pr_debug("range [0x%llx 0x%llx] already TTM mapped\n",
> +				 start, last);
> +			mapping = container_of((struct rb_node *)node,
> +					       struct amdgpu_bo_va_mapping, rb);
> +			bo = mapping->bo_va->base.bo;
> +			goto out_unreserve;
> +		}
> +
> +		/* Check userptr by searching entire vm->va interval tree */
> +		node = interval_tree_iter_first(&vm->va, 0, ~0ULL);
> +		while (node) {
> +			mapping = container_of((struct rb_node *)node,
> +					       struct amdgpu_bo_va_mapping, rb);
> +			bo = mapping->bo_va->base.bo;
> +
> +			if (amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm,
> +							 start << PAGE_SHIFT,
> +							 last << PAGE_SHIFT)) {
> +				pr_debug("[0x%llx 0x%llx] userptr mapped\n",
> +					 start, last);
> +				goto out_unreserve;
> +			}
> +			bo = NULL;
> +			node = interval_tree_iter_next(node, 0, ~0ULL);
>  		}
> +
> +out_unreserve:
>  		amdgpu_bo_unreserve(vm->root.bo);
> +		if (bo)
> +			break;
>  	}
>  
> -	return 0;
> +	return bo;
>  }
>  
>  /**
> @@ -2732,6 +2761,7 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>  	struct vm_area_struct *vma;
>  	unsigned long end;
>  	unsigned long start_unchg = start;
> +	struct amdgpu_bo *bo;
>  
>  	start <<= PAGE_SHIFT;
>  	end = start + (size << PAGE_SHIFT);
> @@ -2743,7 +2773,12 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>  		start = min(end, vma->vm_end);
>  	} while (start < end);
>  
> -	return svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
> +	bo = svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
> +	if (IS_ERR(bo))
> +		return PTR_ERR(bo);
> +	if (bo)
> +		return -EADDRINUSE;
> +	return 0;
>  }
>  
>  /**

Re: [PATCH 3/4] drm/amdkfd: avoid svm conflicting with userptr address

[philip yang]

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

Re: [PATCH 3/4] drm/amdkfd: avoid svm conflicting with userptr address

[Felix Kuehling]

Am 2021-10-06 um 2:09 p.m. schrieb philip yang:
>
>
> On 2021-10-06 1:22 p.m., Felix Kuehling wrote:
>> Am 2021-10-06 um 10:32 a.m. schrieb Philip Yang:
>>> For ioctl_alloc_memory_of_gpu to alloc userptr bo, check userptr address
>>> does not exist in svm->objects.
>>>
>>> For svm range allocation, look for address in the userptr range of
>>> interval tree VA nodes.
>> Why? The purpose of the check is to prevent the same GPU virtual address
>> being managed by the two different memory managers. So checking
>> args->va_addr should be correct even for userptr BOs. The CPU virtual
>> address should be OK to be mapped with userptr and SVM at the same time
>> as long as the userptr uses a distinct GPU virtual address.
>
> userptr cpu virtual address is registered to MMU notifier, if svm
> range overlap with userptr, then migration svm range triggers mmu
> notifier to evict userptr and evict user queues, for xnack on, this is
> not correct. And restore userptr will fail if svm range is migrated to
> vram because hmm_range_fault failed to get system pages, app will soft
> hang as queues are not restored.
>
OK. Then we need to check both the CPU and GPU virtual address ranges.
Having userptr or SVM registrations fail like that is not ideal. It only
changes the failure mode, but doesn't really fix applications affected
by this.

A real solution probably requires, that we reimplement userptrs using
the SVM API in the Thunk, when SVM is available. That way you avoid this
conflict between the two memory managers.

Regards,
  Felix


> Regards,
>
> Philip
>
>> Regards,
>>   Felix
>>
>>
>>> Change helper svm_range_check_vm to return amdgpu_bo, which will be used
>>> to avoid creating new svm range overlap with bo later.
>>>
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++++---
>>>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 55 +++++++++++++++++++-----
>>>  2 files changed, 57 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index f1e7edeb4e6b..34dfa6a938bf 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -1255,6 +1255,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>>>  	long err;
>>>  	uint64_t offset = args->mmap_offset;
>>>  	uint32_t flags = args->flags;
>>> +	unsigned long start, last;
>>>  
>>>  	if (args->size == 0)
>>>  		return -EINVAL;
>>> @@ -1266,11 +1267,17 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>>>  	svm_range_list_lock_and_flush_work(&p->svms, current->mm);
>>>  	mutex_lock(&p->svms.lock);
>>>  	mmap_write_unlock(current->mm);
>>> -	if (interval_tree_iter_first(&p->svms.objects,
>>> -				     args->va_addr >> PAGE_SHIFT,
>>> -				     (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
>>> -		pr_err("Address: 0x%llx already allocated by SVM\n",
>>> -			args->va_addr);
>>> +
>>> +	if (flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) {
>>> +		start = args->mmap_offset >> PAGE_SHIFT;
>>> +		last = (args->mmap_offset + args->size - 1) >> PAGE_SHIFT;
>>> +	} else {
>>> +		start = args->va_addr >> PAGE_SHIFT;
>>> +		last = (args->va_addr + args->size - 1) >> PAGE_SHIFT;
>>> +	}
>>> +
>>> +	if (interval_tree_iter_first(&p->svms.objects, start, last)) {
>>> +		pr_err("[0x%lx 0x%lx] already allocated by SVM\n", start, last);
>>>  		mutex_unlock(&p->svms.lock);
>>>  		return -EADDRINUSE;
>>>  	}
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 7f0743fcfcc3..d49c08618714 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -2679,15 +2679,18 @@ int svm_range_list_init(struct kfd_process *p)
>>>   *
>>>   * Context: Process context
>>>   *
>>> - * Return 0 - OK, if the range is not mapped.
>>> + * Return NULL - if the range is not mapped.
>>> + * amdgpu_bo - if the range is mapped by old API
>>>   * Otherwise error code:
>>> - * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
>>>   * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
>>>   * a signal. Release all buffer reservations and return to user-space.
>>>   */
>>> -static int
>>> +static struct amdgpu_bo *
>>>  svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
>>>  {
>>> +	struct amdgpu_bo_va_mapping *mapping;
>>> +	struct interval_tree_node *node;
>>> +	struct amdgpu_bo *bo = NULL;
>>>  	uint32_t i;
>>>  	int r;
>>>  
>>> @@ -2700,16 +2703,42 @@ svm_range_check_vm(struct kfd_process *p, uint64_t start, uint64_t last)
>>>  		vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
>>>  		r = amdgpu_bo_reserve(vm->root.bo, false);
>>>  		if (r)
>>> -			return r;
>>> -		if (interval_tree_iter_first(&vm->va, start, last)) {
>>> -			pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
>>> -			amdgpu_bo_unreserve(vm->root.bo);
>>> -			return -EADDRINUSE;
>>> +			return ERR_PTR(r);
>>> +		node = interval_tree_iter_first(&vm->va, start, last);
>>> +		if (node) {
>>> +			pr_debug("range [0x%llx 0x%llx] already TTM mapped\n",
>>> +				 start, last);
>>> +			mapping = container_of((struct rb_node *)node,
>>> +					       struct amdgpu_bo_va_mapping, rb);
>>> +			bo = mapping->bo_va->base.bo;
>>> +			goto out_unreserve;
>>> +		}
>>> +
>>> +		/* Check userptr by searching entire vm->va interval tree */
>>> +		node = interval_tree_iter_first(&vm->va, 0, ~0ULL);
>>> +		while (node) {
>>> +			mapping = container_of((struct rb_node *)node,
>>> +					       struct amdgpu_bo_va_mapping, rb);
>>> +			bo = mapping->bo_va->base.bo;
>>> +
>>> +			if (amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm,
>>> +							 start << PAGE_SHIFT,
>>> +							 last << PAGE_SHIFT)) {
>>> +				pr_debug("[0x%llx 0x%llx] userptr mapped\n",
>>> +					 start, last);
>>> +				goto out_unreserve;
>>> +			}
>>> +			bo = NULL;
>>> +			node = interval_tree_iter_next(node, 0, ~0ULL);
>>>  		}
>>> +
>>> +out_unreserve:
>>>  		amdgpu_bo_unreserve(vm->root.bo);
>>> +		if (bo)
>>> +			break;
>>>  	}
>>>  
>>> -	return 0;
>>> +	return bo;
>>>  }
>>>  
>>>  /**
>>> @@ -2732,6 +2761,7 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>>>  	struct vm_area_struct *vma;
>>>  	unsigned long end;
>>>  	unsigned long start_unchg = start;
>>> +	struct amdgpu_bo *bo;
>>>  
>>>  	start <<= PAGE_SHIFT;
>>>  	end = start + (size << PAGE_SHIFT);
>>> @@ -2743,7 +2773,12 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>>>  		start = min(end, vma->vm_end);
>>>  	} while (start < end);
>>>  
>>> -	return svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
>>> +	bo = svm_range_check_vm(p, start_unchg, (end - 1) >> PAGE_SHIFT);
>>> +	if (IS_ERR(bo))
>>> +		return PTR_ERR(bo);
>>> +	if (bo)
>>> +		return -EADDRINUSE;
>>> +	return 0;
>>>  }
>>>  
>>>  /**

Re: [PATCH 3/4] drm/amdkfd: avoid svm conflicting with userptr address

[philip yang]

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

Re: [PATCH 1/4] drm/amdkfd: ratelimited svm debug messages

[Felix Kuehling]

Am 2021-10-06 um 10:32 a.m. schrieb Philip Yang:
> No function change, use pr_debug_ratelimited to avoid per page debug
> message overflowing dmesg buf and console log.
>
> use dev_err to show error message from unexpected situation, to provide
> clue to help debug without enabling dynamic debug log.

AFAIK, dev_err does not print function and line-number information. So
you probably need to provide a little more context in these messages. I
think this could be done with a

    #define pr_fmt(fmt) "kfd_migrate: " fmt

in kfd_migrate.c. I'll make a few more specific comments inline.


>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 34 ++++++++++++------------
>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 12 ++++-----
>  2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> index f53e17a94ad8..069422337cf7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> @@ -151,14 +151,14 @@ svm_migrate_copy_memory_gart(struct amdgpu_device *adev, dma_addr_t *sys,
>  			gart_d = svm_migrate_direct_mapping_addr(adev, *vram);
>  		}
>  		if (r) {
> -			pr_debug("failed %d to create gart mapping\n", r);
> +			dev_err(adev->dev, "fail %d create gart mapping\n", r);
>  			goto out_unlock;
>  		}
>  
>  		r = amdgpu_copy_buffer(ring, gart_s, gart_d, size * PAGE_SIZE,
>  				       NULL, &next, false, true, false);
>  		if (r) {
> -			pr_debug("failed %d to copy memory\n", r);
> +			dev_err(adev->dev, "fail %d to copy memory\n", r);
>  			goto out_unlock;
>  		}
>  
> @@ -285,7 +285,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  	r = svm_range_vram_node_new(adev, prange, true);
>  	if (r) {
> -		pr_debug("failed %d get 0x%llx pages from vram\n", r, npages);
> +		dev_err(adev->dev, "fail %d get %lld vram pages\n", r, npages);

This message is misleading. svm_range_vram_node_new doesn't care about
npages. It allocates memory for the whole range or reuses an existing
allocation. So I'd drop the npages from the message.


>  		goto out;
>  	}
>  
> @@ -305,7 +305,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  					      DMA_TO_DEVICE);
>  			r = dma_mapping_error(dev, src[i]);
>  			if (r) {
> -				pr_debug("failed %d dma_map_page\n", r);
> +				dev_err(adev->dev, "fail %d dma_map_page\n", r);
>  				goto out_free_vram_pages;
>  			}
>  		} else {
> @@ -325,8 +325,8 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  			continue;
>  		}
>  
> -		pr_debug("dma mapping src to 0x%llx, page_to_pfn 0x%lx\n",
> -			 src[i] >> PAGE_SHIFT, page_to_pfn(spage));
> +		pr_debug_ratelimited("dma mapping src to 0x%llx, pfn 0x%lx\n",
> +				     src[i] >> PAGE_SHIFT, page_to_pfn(spage));
>  
>  		if (j >= (cursor.size >> PAGE_SHIFT) - 1 && i < npages - 1) {
>  			r = svm_migrate_copy_memory_gart(adev, src + i - j,
> @@ -347,7 +347,7 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  out_free_vram_pages:
>  	if (r) {
> -		pr_debug("failed %d to copy memory to vram\n", r);
> +		dev_err(adev->dev, "fail %d to copy memory to vram\n", r);

I think you only get here if svm_migrate_copy_memory_gart failed. That
function already prints its own error messages, so this probably doesn't
need to be a dev_err.


>  		while (i--) {
>  			svm_migrate_put_vram_page(adev, dst[i]);
>  			migrate->dst[i] = 0;
> @@ -405,8 +405,8 @@ svm_migrate_vma_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  	r = migrate_vma_setup(&migrate);
>  	if (r) {
> -		pr_debug("failed %d prepare migrate svms 0x%p [0x%lx 0x%lx]\n",
> -			 r, prange->svms, prange->start, prange->last);
> +		dev_err(adev->dev, "fail %d vma setup 0x%p [0x%lx 0x%lx]\n",
> +			r, prange->svms, prange->start, prange->last);
>  		goto out_free;
>  	}
>  	if (migrate.cpages != npages) {
> @@ -506,7 +506,7 @@ static void svm_migrate_page_free(struct page *page)
>  	struct svm_range_bo *svm_bo = page->zone_device_data;
>  
>  	if (svm_bo) {
> -		pr_debug("svm_bo ref left: %d\n", kref_read(&svm_bo->kref));
> +		pr_debug_ratelimited("ref: %d\n", kref_read(&svm_bo->kref));
>  		svm_range_bo_unref(svm_bo);
>  	}
>  }
> @@ -563,8 +563,8 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  		dpage = svm_migrate_get_sys_page(migrate->vma, addr);
>  		if (!dpage) {
> -			pr_debug("failed get page svms 0x%p [0x%lx 0x%lx]\n",
> -				 prange->svms, prange->start, prange->last);
> +			dev_err(adev->dev, "fail get page 0x%p [0x%lx 0x%lx]\n",
> +				prange->svms, prange->start, prange->last);

The prange->svms pointer (or its hash) is pretty meaningless in an error
message. It's OK in a debug message to correlate with other messages.
But in an error message that's always enabled, I'd prefer a more
readable ID. I think it basically stands for the process because svms is
part of the kfd_process structure.

prange->start/end are also not really meaningful for this failure. The
page allocation failure doesn't depend on the prange start and end
addresses. It's basically just an OOM error.

I think Linux will be pretty noisy about OOM errors, so we probably
don't need to add more messages about that here.


>  			r = -ENOMEM;
>  			goto out_oom;
>  		}
> @@ -572,12 +572,12 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>  		dst[i] = dma_map_page(dev, dpage, 0, PAGE_SIZE, DMA_FROM_DEVICE);
>  		r = dma_mapping_error(dev, dst[i]);
>  		if (r) {
> -			pr_debug("failed %d dma_map_page\n", r);
> +			dev_err(adev->dev, "fail %d dma_map_page\n", r);
>  			goto out_oom;
>  		}
>  
> -		pr_debug("dma mapping dst to 0x%llx, page_to_pfn 0x%lx\n",
> -			      dst[i] >> PAGE_SHIFT, page_to_pfn(dpage));
> +		pr_debug_ratelimited("dma mapping dst to 0x%llx, pfn 0x%lx\n",
> +				     dst[i] >> PAGE_SHIFT, page_to_pfn(dpage));
>  
>  		migrate->dst[i] = migrate_pfn(page_to_pfn(dpage));
>  		migrate->dst[i] |= MIGRATE_PFN_LOCKED;
> @@ -631,8 +631,8 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  	r = migrate_vma_setup(&migrate);
>  	if (r) {
> -		pr_debug("failed %d prepare migrate svms 0x%p [0x%lx 0x%lx]\n",
> -			 r, prange->svms, prange->start, prange->last);
> +		dev_err(adev->dev, "fail %d vma setup 0x%p [0x%lx 0x%lx]\n",
> +			r, prange->svms, prange->start, prange->last);
>  		goto out_free;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 7c42a0d4e0de..7f0743fcfcc3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -158,17 +158,17 @@ svm_range_dma_map_dev(struct amdgpu_device *adev, struct svm_range *prange,
>  				   bo_adev->vm_manager.vram_base_offset -
>  				   bo_adev->kfd.dev->pgmap.range.start;
>  			addr[i] |= SVM_RANGE_VRAM_DOMAIN;
> -			pr_debug("vram address detected: 0x%llx\n", addr[i]);
> +			pr_debug_ratelimited("vram address: 0x%llx\n", addr[i]);
>  			continue;
>  		}
>  		addr[i] = dma_map_page(dev, page, 0, PAGE_SIZE, dir);
>  		r = dma_mapping_error(dev, addr[i]);
>  		if (r) {
> -			pr_debug("failed %d dma_map_page\n", r);
> +			pr_err("failed %d dma_map_page\n", r);

Why not dev_err?

Regards,
  Felix


>  			return r;
>  		}
> -		pr_debug("dma mapping 0x%llx for page addr 0x%lx\n",
> -			 addr[i] >> PAGE_SHIFT, page_to_pfn(page));
> +		pr_debug_ratelimited("dma mapping 0x%llx for page addr 0x%lx\n",
> +				     addr[i] >> PAGE_SHIFT, page_to_pfn(page));
>  	}
>  	return 0;
>  }
> @@ -217,7 +217,7 @@ void svm_range_dma_unmap(struct device *dev, dma_addr_t *dma_addr,
>  	for (i = offset; i < offset + npages; i++) {
>  		if (!svm_is_valid_dma_mapping_addr(dev, dma_addr[i]))
>  			continue;
> -		pr_debug("dma unmapping 0x%llx\n", dma_addr[i] >> PAGE_SHIFT);
> +		pr_debug_ratelimited("unmap 0x%llx\n", dma_addr[i] >> PAGE_SHIFT);
>  		dma_unmap_page(dev, dma_addr[i], PAGE_SIZE, dir);
>  		dma_addr[i] = 0;
>  	}
> @@ -1459,7 +1459,7 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>  		/* This should never happen. actual_loc gets set by
>  		 * svm_migrate_ram_to_vram after allocating a BO.
>  		 */
> -		WARN(1, "VRAM BO missing during validation\n");
> +		WARN_ONCE(1, "VRAM BO missing during validation\n");
>  		return -EINVAL;
>  	}
>  

Re: [PATCH 2/4] drm/amdkfd: handle partial migration cpages 0

[Felix Kuehling]

Am 2021-10-06 um 10:32 a.m. schrieb Philip Yang:
> migrate_vma_setup may return cpages 0, means 0 page can be migrated,
> treat this as error case to skip the rest of migration steps, and don't
> change prange actual loc, to avoid warning message "VRAM BO missing
> during validation".
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 48 ++++++++++++++----------
>  1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> index 069422337cf7..9b68e3e8f2a1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> @@ -409,20 +409,25 @@ svm_migrate_vma_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
>  			r, prange->svms, prange->start, prange->last);
>  		goto out_free;
>  	}
> -	if (migrate.cpages != npages) {
> -		pr_debug("Partial migration. 0x%lx/0x%llx pages can be migrated\n",
> -			 migrate.cpages,
> -			 npages);
> -	}
>  
> -	if (migrate.cpages) {
> -		r = svm_migrate_copy_to_vram(adev, prange, &migrate, &mfence,
> -					     scratch);
> -		migrate_vma_pages(&migrate);
> -		svm_migrate_copy_done(adev, mfence);
> -		migrate_vma_finalize(&migrate);
> +	if (migrate.cpages != npages)
> +		pr_debug("partial migration, 0x%lx/0x%llx pages migrated\n",
> +			 migrate.cpages, npages);
> +	else
> +		pr_debug("0x%lx pages migrated\n", migrate.cpages);
> +
> +	if (!migrate.cpages) {
> +		pr_debug("failed collect migrate sys pages [0x%lx 0x%lx]\n",
> +			 prange->start, prange->last);
> +		r = -ENOMEM;

I think just returning an error here is incorrect. This error gets
handled in svm_migrate_ram_to_vram and prevents the following VMAs from
migrating as well (if the range spans multiple VMAs).

Maybe return the number of pages migrated, if successful. Then the
caller can add up all the successful migrations and update
prange->actual_loc only if the total is > 0.

Regards,
  Felix


> +		goto out_free;
>  	}
>  
> +	r = svm_migrate_copy_to_vram(adev, prange, &migrate, &mfence, scratch);
> +	migrate_vma_pages(&migrate);
> +	svm_migrate_copy_done(adev, mfence);
> +	migrate_vma_finalize(&migrate);
> +
>  	svm_range_dma_unmap(adev->dev, scratch, 0, npages);
>  	svm_range_free_dma_mappings(prange);
>  
> @@ -636,19 +641,24 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>  		goto out_free;
>  	}
>  
> -	pr_debug("cpages %ld\n", migrate.cpages);
> +	if (migrate.cpages != npages)
> +		pr_debug("partial migration, 0x%lx/0x%llx pages migrated\n",
> +			 migrate.cpages, npages);
> +	else
> +		pr_debug("0x%lx pages migrated\n", migrate.cpages);
>  
> -	if (migrate.cpages) {
> -		r = svm_migrate_copy_to_ram(adev, prange, &migrate, &mfence,
> -					    scratch, npages);
> -		migrate_vma_pages(&migrate);
> -		svm_migrate_copy_done(adev, mfence);
> -		migrate_vma_finalize(&migrate);
> -	} else {
> +	if (!migrate.cpages) {
>  		pr_debug("failed collect migrate device pages [0x%lx 0x%lx]\n",
>  			 prange->start, prange->last);
> +		r = -ENOMEM;
> +		goto out_free;
>  	}
>  
> +	r = svm_migrate_copy_to_ram(adev, prange, &migrate, &mfence,
> +				    scratch, npages);
> +	migrate_vma_pages(&migrate);
> +	svm_migrate_copy_done(adev, mfence);
> +	migrate_vma_finalize(&migrate);
>  	svm_range_dma_unmap(adev->dev, scratch, 0, npages);
>  
>  out_free:

Re: [PATCH 1/4] drm/amdkfd: ratelimited svm debug messages

[philip yang]

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