[PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[Philip Yang]

Replace prange->mapped_to_gpu with prange->bitmap_mapped[], which is
based on prange granularity, updated when map to GPUS or unmap from
GPUs, to optimize multiple GPU map, unmap and retry fault recover.

svm_range_is_mapped is false only if no parital range mapping on any
GPUs.

Split the bitmap_mapped when unmap from cpu to split the prange.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 218 ++++++++++++++++++++++-----
 drivers/gpu/drm/amd/amdkfd/kfd_svm.h |   4 +-
 2 files changed, 184 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 040dc32ad475..626e0dd4ec79 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -292,12 +292,12 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
 					KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0);
 	}
 
-	/* free dma_addr array for each gpu */
+	/* free dma_addr array, bitmap_mapped for each gpu */
 	for (gpuidx = 0; gpuidx < MAX_GPU_INSTANCE; gpuidx++) {
-		if (prange->dma_addr[gpuidx]) {
+		if (prange->dma_addr[gpuidx])
 			kvfree(prange->dma_addr[gpuidx]);
-				prange->dma_addr[gpuidx] = NULL;
-		}
+		if (prange->bitmap_mapped[gpuidx])
+			bitmap_free(prange->bitmap_mapped[gpuidx]);
 	}
 
 	mutex_destroy(&prange->lock);
@@ -323,19 +323,38 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
 	uint64_t size = last - start + 1;
 	struct svm_range *prange;
 	struct kfd_process *p;
-
-	prange = kzalloc(sizeof(*prange), GFP_KERNEL);
-	if (!prange)
-		return NULL;
+	unsigned int nbits;
+	uint32_t gpuidx;
 
 	p = container_of(svms, struct kfd_process, svms);
 	if (!p->xnack_enabled && update_mem_usage &&
 	    amdgpu_amdkfd_reserve_mem_limit(NULL, size << PAGE_SHIFT,
 				    KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0)) {
 		pr_info("SVM mapping failed, exceeds resident system memory limit\n");
-		kfree(prange);
 		return NULL;
 	}
+
+	prange = kzalloc(sizeof(*prange), GFP_KERNEL);
+	if (!prange)
+		return NULL;
+
+	svm_range_set_default_attributes(&prange->preferred_loc,
+					 &prange->prefetch_loc,
+					 &prange->granularity, &prange->flags);
+
+	nbits = svm_range_mapped_nbits(size, prange->granularity);
+	pr_debug("prange 0x%p [0x%llx 0x%llx] bitmap_mapped nbits %d\n", prange,
+		 start, last, nbits);
+	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
+		prange->bitmap_mapped[gpuidx] = bitmap_zalloc(nbits, GFP_KERNEL);
+		if (!prange->bitmap_mapped[gpuidx]) {
+			while (gpuidx--)
+				bitmap_free(prange->bitmap_mapped[gpuidx]);
+			kfree(prange);
+			return NULL;
+		}
+	}
+
 	prange->npages = size;
 	prange->svms = svms;
 	prange->start = start;
@@ -354,10 +373,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
 		bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
 			    MAX_GPU_INSTANCE);
 
-	svm_range_set_default_attributes(&prange->preferred_loc,
-					 &prange->prefetch_loc,
-					 &prange->granularity, &prange->flags);
-
 	pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
 
 	return prange;
@@ -972,6 +987,48 @@ svm_range_split_nodes(struct svm_range *new, struct svm_range *old,
 	return 0;
 }
 
+static int
+svm_range_split_bitmap_mapped(struct svm_range *new, struct svm_range *old,
+			      uint64_t start, uint64_t last)
+{
+	struct kfd_process *p = container_of(new->svms, struct kfd_process, svms);
+	unsigned int nbits, old_nbits, old_nbits2;
+	unsigned long *bits;
+	uint32_t gpuidx;
+
+	nbits = svm_range_mapped_nbits(new->npages, new->granularity);
+	old_nbits = svm_range_mapped_nbits(old->npages, old->granularity);
+	old_nbits2 = svm_range_mapped_nbits(last - start + 1, old->granularity);
+
+	pr_debug("old 0x%p [0x%lx 0x%lx] => [0x%llx 0x%llx] nbits %d => %d\n",
+		 old, old->start, old->last, start, last, old_nbits, old_nbits2);
+	pr_debug("new 0x%p [0x%lx 0x%lx] nbits %d\n", new, new->start, new->last,
+		 nbits);
+
+	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
+		bits = bitmap_alloc(old_nbits2, GFP_KERNEL);
+		if (!bits)
+			return -ENOMEM;
+
+		if (start == old->start) {
+			bitmap_shift_right(new->bitmap_mapped[gpuidx],
+					   old->bitmap_mapped[gpuidx],
+					   old_nbits2, old_nbits);
+			bitmap_shift_right(bits, old->bitmap_mapped[gpuidx], 0,
+					   old_nbits2);
+		} else {
+			bitmap_copy(new->bitmap_mapped[gpuidx],
+				    old->bitmap_mapped[gpuidx], nbits);
+			bitmap_shift_right(bits, old->bitmap_mapped[gpuidx],
+					   nbits, old_nbits);
+		}
+		bitmap_free(old->bitmap_mapped[gpuidx]);
+		old->bitmap_mapped[gpuidx] = bits;
+	}
+
+	return 0;
+}
+
 /**
  * svm_range_split_adjust - split range and adjust
  *
@@ -1012,6 +1069,10 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
 			return r;
 	}
 
+	r = svm_range_split_bitmap_mapped(new, old, start, last);
+	if (r)
+		return r;
+
 	old->npages = last - start + 1;
 	old->start = start;
 	old->last = last;
@@ -1020,7 +1081,6 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
 	new->prefetch_loc = old->prefetch_loc;
 	new->actual_loc = old->actual_loc;
 	new->granularity = old->granularity;
-	new->mapped_to_gpu = old->mapped_to_gpu;
 	bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
 	bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
 
@@ -1310,6 +1370,84 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 				      fence);
 }
 
+/**
+ * svm_range_partial_mapped_dev - check if prange mapped on the specific gpu
+ *
+ * @gpuidx: the gpu to check
+ * @prange: prange to check
+ * @start: the start address in pages
+ * @last: the last address in pages
+ *
+ * Return:
+ * true: if any partial range mapped on gpu based on granularity boundary
+ * false: if the entire range not mapped
+ */
+static bool
+svm_range_partial_mapped_dev(uint32_t gpuidx, struct svm_range *prange,
+			     unsigned long start, unsigned long last)
+{
+	unsigned long index, off;
+	bool mapped = false;
+
+	start = max(start, prange->start);
+	last = min(last, prange->last);
+	if (last < start)
+		goto out;
+
+	for (off = start; off <= last; off += (1UL << prange->granularity)) {
+		index = (off - prange->start) >> prange->granularity;
+		if (test_bit(index, prange->bitmap_mapped[gpuidx])) {
+			mapped = true;
+			break;
+		}
+	}
+out:
+	pr_debug("prange 0x%p [0x%lx 0x%lx] mapped %d on gpu %d\n", prange,
+		 start, last, mapped, gpuidx);
+	return mapped;
+}
+
+static bool
+svm_range_partial_mapped(struct svm_range *prange, unsigned long start,
+			 unsigned long last)
+{
+	struct kfd_process *p = container_of(prange->svms, struct kfd_process, svms);
+	struct svm_range *pchild;
+	uint32_t gpuidx;
+
+	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
+		list_for_each_entry(pchild, &prange->child_list, child_list) {
+			if (svm_range_partial_mapped_dev(gpuidx, pchild, start, last))
+				return true;
+		}
+
+		if (svm_range_partial_mapped_dev(gpuidx, prange, start, last))
+			return true;
+	}
+	return false;
+}
+
+static bool svm_range_is_mapped(struct svm_range *prange)
+{
+	return svm_range_partial_mapped(prange, prange->start, prange->last);
+}
+
+static void
+svm_range_update_mapped(uint32_t gpuidx, struct svm_range *prange,
+			unsigned long start, unsigned long last, bool mapped)
+{
+	unsigned long index, nbits;
+
+	index = (start - prange->start) >> prange->granularity;
+	nbits = svm_range_mapped_nbits(last - start + 1, prange->granularity);
+	if (mapped)
+		bitmap_set(prange->bitmap_mapped[gpuidx], index, nbits);
+	else
+		bitmap_clear(prange->bitmap_mapped[gpuidx], index, nbits);
+	pr_debug("prange 0x%p [0x%lx 0x%lx] update mapped %d nbits %ld gpu %d\n",
+		 prange, start, last, mapped, nbits, gpuidx);
+}
+
 static int
 svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
 			  unsigned long last, uint32_t trigger)
@@ -1321,29 +1459,28 @@ svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
 	uint32_t gpuidx;
 	int r = 0;
 
-	if (!prange->mapped_to_gpu) {
-		pr_debug("prange 0x%p [0x%lx 0x%lx] not mapped to GPU\n",
-			 prange, prange->start, prange->last);
-		return 0;
-	}
-
-	if (prange->start == start && prange->last == last) {
-		pr_debug("unmap svms 0x%p prange 0x%p\n", prange->svms, prange);
-		prange->mapped_to_gpu = false;
-	}
-
 	bitmap_or(bitmap, prange->bitmap_access, prange->bitmap_aip,
 		  MAX_GPU_INSTANCE);
 	p = container_of(prange->svms, struct kfd_process, svms);
 
 	for_each_set_bit(gpuidx, bitmap, MAX_GPU_INSTANCE) {
-		pr_debug("unmap from gpu idx 0x%x\n", gpuidx);
 		pdd = kfd_process_device_from_gpuidx(p, gpuidx);
 		if (!pdd) {
 			pr_debug("failed to find device idx %d\n", gpuidx);
-			return -EINVAL;
+			continue;
+		}
+
+		if (!svm_range_partial_mapped_dev(gpuidx, prange, start, last)) {
+			pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] not mapped on gpu %d\n",
+				 prange->svms, prange, start, last, gpuidx);
+			continue;
 		}
 
+		svm_range_update_mapped(gpuidx, prange, start, last, false);
+
+		pr_debug("unmap svms 0x%p prange 0x%p [0x%lx 0x%lx] from gpu %d\n",
+			 prange->svms, prange, start, last, gpuidx);
+
 		kfd_smi_event_unmap_from_gpu(pdd->dev, p->lead_thread->pid,
 					     start, last, trigger);
 
@@ -1483,6 +1620,10 @@ svm_range_map_to_gpus(struct svm_range *prange, unsigned long offset,
 		if (r)
 			break;
 
+		if (!r)
+			svm_range_update_mapped(gpuidx, prange, prange->start + offset,
+						prange->start + offset + npages - 1, true);
+
 		if (fence) {
 			r = dma_fence_wait(fence, false);
 			dma_fence_put(fence);
@@ -1648,7 +1789,7 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 
 	if (bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
 		bitmap_copy(ctx->bitmap, prange->bitmap_access, MAX_GPU_INSTANCE);
-		if (!prange->mapped_to_gpu ||
+		if (!svm_range_is_mapped(prange) ||
 		    bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
 			r = 0;
 			goto free_ctx;
@@ -1729,9 +1870,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 			r = svm_range_map_to_gpus(prange, offset, npages, readonly,
 						  ctx->bitmap, flush_tlb);
 
-		if (!r && next == end)
-			prange->mapped_to_gpu = true;
-
 		svm_range_unlock(prange);
 
 		addr = next;
@@ -1900,10 +2038,10 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
 	if (!p->xnack_enabled ||
 	    (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) {
 		int evicted_ranges;
-		bool mapped = prange->mapped_to_gpu;
+		bool mapped = svm_range_is_mapped(prange);
 
 		list_for_each_entry(pchild, &prange->child_list, child_list) {
-			if (!pchild->mapped_to_gpu)
+			if (!svm_range_is_mapped(pchild))
 				continue;
 			mapped = true;
 			mutex_lock_nested(&pchild->lock, 1);
@@ -1966,7 +2104,9 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
 
 static struct svm_range *svm_range_clone(struct svm_range *old)
 {
+	struct kfd_process *p = container_of(old->svms, struct kfd_process, svms);
 	struct svm_range *new;
+	uint32_t gpuidx;
 
 	new = svm_range_new(old->svms, old->start, old->last, false);
 	if (!new)
@@ -1988,7 +2128,11 @@ static struct svm_range *svm_range_clone(struct svm_range *old)
 	new->prefetch_loc = old->prefetch_loc;
 	new->actual_loc = old->actual_loc;
 	new->granularity = old->granularity;
-	new->mapped_to_gpu = old->mapped_to_gpu;
+	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
+		bitmap_copy(new->bitmap_mapped[gpuidx], old->bitmap_mapped[gpuidx],
+			    svm_range_mapped_nbits(new->last - new->start + 1,
+						   new->granularity));
+	};
 	bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
 	bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
 
@@ -2107,7 +2251,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
 		next_start = min(node->last, last) + 1;
 
 		if (svm_range_is_same_attrs(p, prange, nattr, attrs) &&
-		    prange->mapped_to_gpu) {
+		    svm_range_is_mapped(prange)) {
 			/* nothing to do */
 		} else if (node->start < start || node->last > last) {
 			/* node intersects the update range and its attributes
@@ -3587,7 +3731,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
 
 		if (migrated && (!p->xnack_enabled ||
 		    (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) &&
-		    prange->mapped_to_gpu) {
+		    svm_range_is_mapped(prange)) {
 			pr_debug("restore_work will update mappings of GPUs\n");
 			mutex_unlock(&prange->migrate_mutex);
 			continue;
@@ -3598,7 +3742,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
 			continue;
 		}
 
-		flush_tlb = !migrated && update_mapping && prange->mapped_to_gpu;
+		flush_tlb = !migrated && update_mapping && svm_range_is_mapped(prange);
 
 		r = svm_range_validate_and_map(mm, prange, MAX_GPU_INSTANCE,
 					       true, flush_tlb);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
index d2e94d8fb4be..10c92c5e23a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
@@ -132,7 +132,7 @@ struct svm_range {
 	struct list_head		child_list;
 	DECLARE_BITMAP(bitmap_access, MAX_GPU_INSTANCE);
 	DECLARE_BITMAP(bitmap_aip, MAX_GPU_INSTANCE);
-	bool				mapped_to_gpu;
+	unsigned long			*bitmap_mapped[MAX_GPU_INSTANCE];
 };
 
 static inline void svm_range_lock(struct svm_range *prange)
@@ -163,6 +163,8 @@ static inline struct svm_range_bo *svm_range_bo_ref(struct svm_range_bo *svm_bo)
 		return NULL;
 }
 
+#define svm_range_mapped_nbits(size, granularity) DIV_ROUND_UP((size), 1UL << (granularity))
+
 int svm_range_list_init(struct kfd_process *p);
 void svm_range_list_fini(struct kfd_process *p);
 int svm_ioctl(struct kfd_process *p, enum kfd_ioctl_svm_op op, uint64_t start,
-- 
2.35.1

[PATCH 2/3] amd/amdkfd: Unmap range from GPUs based on granularity

[Philip Yang]

Align unmap range start and last address to granularity boundary.
Skip unmap if range is already unmapped from GPUs.

This also solve the rocgdb CWSR migration related issue.

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

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 626e0dd4ec79..ac65bf25c685 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2004,6 +2004,26 @@ static void svm_range_restore_work(struct work_struct *work)
 	mmput(mm);
 }
 
+static unsigned long
+svm_range_align_start(struct svm_range *prange, unsigned long start)
+{
+	unsigned long start_align;
+
+	start_align = ALIGN_DOWN(start, 1UL << prange->granularity);
+	start_align = max_t(unsigned long, start_align, prange->start);
+	return start_align;
+}
+
+static unsigned long
+svm_range_align_last(struct svm_range *prange, unsigned long last)
+{
+	unsigned long last_align;
+
+	last_align = ALIGN(last, 1UL << prange->granularity) - 1;
+	last_align = min_t(unsigned long, last_align, prange->last);
+	return last_align;
+}
+
 /**
  * svm_range_evict - evict svm range
  * @prange: svm range structure
@@ -2078,6 +2098,12 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
 		unsigned long s, l;
 		uint32_t trigger;
 
+		if (!svm_range_partial_mapped(prange, start, last)) {
+			pr_debug("svms 0x%p [0x%lx 0x%lx] unmapped already\n",
+				prange->svms, start, last);
+			return 0;
+		}
+
 		if (event == MMU_NOTIFY_MIGRATE)
 			trigger = KFD_SVM_UNMAP_TRIGGER_MMU_NOTIFY_MIGRATE;
 		else
@@ -2085,16 +2111,17 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
 
 		pr_debug("invalidate unmap svms 0x%p [0x%lx 0x%lx] from GPUs\n",
 			 prange->svms, start, last);
+
 		list_for_each_entry(pchild, &prange->child_list, child_list) {
 			mutex_lock_nested(&pchild->lock, 1);
-			s = max(start, pchild->start);
-			l = min(last, pchild->last);
+			s = svm_range_align_start(pchild, start);
+			l = svm_range_align_last(pchild, last);
 			if (l >= s)
 				svm_range_unmap_from_gpus(pchild, s, l, trigger);
 			mutex_unlock(&pchild->lock);
 		}
-		s = max(start, prange->start);
-		l = min(last, prange->last);
+		s = svm_range_align_start(prange, start);
+		l = svm_range_align_last(prange, last);
 		if (l >= s)
 			svm_range_unmap_from_gpus(prange, s, l, trigger);
 	}
-- 
2.35.1

[PATCH 3/3] drm/amdkfd: Check bitmap_mapped flag to skip retry fault

[Philip Yang]

Use bitmap_mapped flag to check if range already mapped to the specific
GPU, to skip the retry fault from different page of the same range.

Remove prange validate_timestamp which is not accurate for multiple
GPUs.

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

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index ac65bf25c685..5e063d902a46 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -43,10 +43,6 @@
 
 #define AMDGPU_SVM_RANGE_RESTORE_DELAY_MS 1
 
-/* Long enough to ensure no retry fault comes after svm range is restored and
- * page table is updated.
- */
-#define AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING	(2UL * NSEC_PER_MSEC)
 #if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
 #define dynamic_svm_range_dump(svms) \
 	_dynamic_func_call_no_desc("svm_range_dump", svm_range_debug_dump, svms)
@@ -365,7 +361,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
 	INIT_LIST_HEAD(&prange->deferred_list);
 	INIT_LIST_HEAD(&prange->child_list);
 	atomic_set(&prange->invalid, 0);
-	prange->validate_timestamp = 0;
 	mutex_init(&prange->migrate_mutex);
 	mutex_init(&prange->lock);
 
@@ -1876,8 +1871,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
 	}
 
 	svm_range_unreserve_bos(ctx);
-	if (!r)
-		prange->validate_timestamp = ktime_get_boottime();
 
 free_ctx:
 	kfree(ctx);
@@ -3162,15 +3155,6 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 		goto out_unlock_range;
 	}
 
-	/* skip duplicate vm fault on different pages of same range */
-	if (ktime_before(timestamp, ktime_add_ns(prange->validate_timestamp,
-				AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING))) {
-		pr_debug("svms 0x%p [0x%lx %lx] already restored\n",
-			 svms, prange->start, prange->last);
-		r = 0;
-		goto out_unlock_range;
-	}
-
 	/* __do_munmap removed VMA, return success as we are handling stale
 	 * retry fault.
 	 */
@@ -3196,6 +3180,14 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
 		goto out_unlock_range;
 	}
 
+	/* skip duplicate vm fault on different pages of same range */
+	if (svm_range_partial_mapped_dev(gpuidx, prange, addr, addr)) {
+		pr_debug("svms 0x%p [0x%lx %lx] already restored on gpu %d\n",
+			 svms, prange->start, prange->last, gpuidx);
+		r = 0;
+		goto out_unlock_range;
+	}
+
 	pr_debug("svms %p [0x%lx 0x%lx] best restore 0x%x, actual loc 0x%x\n",
 		 svms, prange->start, prange->last, best_loc,
 		 prange->actual_loc);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
index 10c92c5e23a7..3afc33a3dd30 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
@@ -125,7 +125,6 @@ struct svm_range {
 	uint32_t			actual_loc;
 	uint8_t				granularity;
 	atomic_t			invalid;
-	ktime_t				validate_timestamp;
 	struct mmu_interval_notifier	notifier;
 	struct svm_work_list_item	work_item;
 	struct list_head		deferred_list;
-- 
2.35.1

Re: [PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[Chen, Xiaogang]

On 9/29/2023 9:11 AM, Philip Yang wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Replace prange->mapped_to_gpu with prange->bitmap_mapped[], which is
> based on prange granularity, updated when map to GPUS or unmap from
> GPUs, to optimize multiple GPU map, unmap and retry fault recover.
>
> svm_range_is_mapped is false only if no parital range mapping on any
> GPUs.
>
> Split the bitmap_mapped when unmap from cpu to split the prange.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 218 ++++++++++++++++++++++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h |   4 +-
>   2 files changed, 184 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 040dc32ad475..626e0dd4ec79 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -292,12 +292,12 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>                                          KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0);
>          }
>
> -       /* free dma_addr array for each gpu */
> +       /* free dma_addr array, bitmap_mapped for each gpu */
>          for (gpuidx = 0; gpuidx < MAX_GPU_INSTANCE; gpuidx++) {
> -               if (prange->dma_addr[gpuidx]) {
> +               if (prange->dma_addr[gpuidx])
>                          kvfree(prange->dma_addr[gpuidx]);
> -                               prange->dma_addr[gpuidx] = NULL;
> -               }
> +               if (prange->bitmap_mapped[gpuidx])
> +                       bitmap_free(prange->bitmap_mapped[gpuidx]);
>          }
>
>          mutex_destroy(&prange->lock);
> @@ -323,19 +323,38 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>          uint64_t size = last - start + 1;
>          struct svm_range *prange;
>          struct kfd_process *p;
> -
> -       prange = kzalloc(sizeof(*prange), GFP_KERNEL);
> -       if (!prange)
> -               return NULL;
> +       unsigned int nbits;
> +       uint32_t gpuidx;
>
>          p = container_of(svms, struct kfd_process, svms);
>          if (!p->xnack_enabled && update_mem_usage &&
>              amdgpu_amdkfd_reserve_mem_limit(NULL, size << PAGE_SHIFT,
>                                      KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0)) {
>                  pr_info("SVM mapping failed, exceeds resident system memory limit\n");
> -               kfree(prange);
>                  return NULL;
>          }
> +
> +       prange = kzalloc(sizeof(*prange), GFP_KERNEL);
> +       if (!prange)
> +               return NULL;
> +
> +       svm_range_set_default_attributes(&prange->preferred_loc,
> +                                        &prange->prefetch_loc,
> +                                        &prange->granularity, &prange->flags);
> +
> +       nbits = svm_range_mapped_nbits(size, prange->granularity);
> +       pr_debug("prange 0x%p [0x%llx 0x%llx] bitmap_mapped nbits %d\n", prange,
> +                start, last, nbits);
> +       for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +               prange->bitmap_mapped[gpuidx] = bitmap_zalloc(nbits, GFP_KERNEL);
> +               if (!prange->bitmap_mapped[gpuidx]) {
> +                       while (gpuidx--)
> +                               bitmap_free(prange->bitmap_mapped[gpuidx]);
> +                       kfree(prange);
> +                       return NULL;
> +               }
> +       }
> +
>          prange->npages = size;
>          prange->svms = svms;
>          prange->start = start;
> @@ -354,10 +373,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>                  bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>                              MAX_GPU_INSTANCE);
>
> -       svm_range_set_default_attributes(&prange->preferred_loc,
> -                                        &prange->prefetch_loc,
> -                                        &prange->granularity, &prange->flags);
> -
>          pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>
>          return prange;
> @@ -972,6 +987,48 @@ svm_range_split_nodes(struct svm_range *new, struct svm_range *old,
>          return 0;
>   }
>
> +static int
> +svm_range_split_bitmap_mapped(struct svm_range *new, struct svm_range *old,
> +                             uint64_t start, uint64_t last)
> +{
> +       struct kfd_process *p = container_of(new->svms, struct kfd_process, svms);
> +       unsigned int nbits, old_nbits, old_nbits2;
> +       unsigned long *bits;
> +       uint32_t gpuidx;
> +
> +       nbits = svm_range_mapped_nbits(new->npages, new->granularity);
> +       old_nbits = svm_range_mapped_nbits(old->npages, old->granularity);
> +       old_nbits2 = svm_range_mapped_nbits(last - start + 1, old->granularity);
> +
> +       pr_debug("old 0x%p [0x%lx 0x%lx] => [0x%llx 0x%llx] nbits %d => %d\n",
> +                old, old->start, old->last, start, last, old_nbits, old_nbits2);
> +       pr_debug("new 0x%p [0x%lx 0x%lx] nbits %d\n", new, new->start, new->last,
> +                nbits);
> +
> +       for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +               bits = bitmap_alloc(old_nbits2, GFP_KERNEL);
> +               if (!bits)
> +                       return -ENOMEM;
> +
> +               if (start == old->start) {
> +                       bitmap_shift_right(new->bitmap_mapped[gpuidx],
> +                                          old->bitmap_mapped[gpuidx],
> +                                          old_nbits2, old_nbits);
> +                       bitmap_shift_right(bits, old->bitmap_mapped[gpuidx], 0,
> +                                          old_nbits2);
> +               } else {
> +                       bitmap_copy(new->bitmap_mapped[gpuidx],
> +                                   old->bitmap_mapped[gpuidx], nbits);
> +                       bitmap_shift_right(bits, old->bitmap_mapped[gpuidx],
> +                                          nbits, old_nbits);
> +               }
> +               bitmap_free(old->bitmap_mapped[gpuidx]);
> +               old->bitmap_mapped[gpuidx] = bits;
> +       }
> +
> +       return 0;
> +}
> +
>   /**
>    * svm_range_split_adjust - split range and adjust
>    *
> @@ -1012,6 +1069,10 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
>                          return r;
>          }
>
> +       r = svm_range_split_bitmap_mapped(new, old, start, last);
> +       if (r)
> +               return r;
> +
>          old->npages = last - start + 1;
>          old->start = start;
>          old->last = last;
> @@ -1020,7 +1081,6 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
>          new->prefetch_loc = old->prefetch_loc;
>          new->actual_loc = old->actual_loc;
>          new->granularity = old->granularity;
> -       new->mapped_to_gpu = old->mapped_to_gpu;
>          bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
>          bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
>
> @@ -1310,6 +1370,84 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>                                        fence);
>   }
>
> +/**
> + * svm_range_partial_mapped_dev - check if prange mapped on the specific gpu
> + *
> + * @gpuidx: the gpu to check
> + * @prange: prange to check
> + * @start: the start address in pages
> + * @last: the last address in pages
> + *
> + * Return:
> + * true: if any partial range mapped on gpu based on granularity boundary
> + * false: if the entire range not mapped
> + */
> +static bool
> +svm_range_partial_mapped_dev(uint32_t gpuidx, struct svm_range *prange,
> +                            unsigned long start, unsigned long last)
> +{
> +       unsigned long index, off;
> +       bool mapped = false;
> +
> +       start = max(start, prange->start);
> +       last = min(last, prange->last);
> +       if (last < start)
> +               goto out;
> +
> +       for (off = start; off <= last; off += (1UL << prange->granularity)) {
> +               index = (off - prange->start) >> prange->granularity;
> +               if (test_bit(index, prange->bitmap_mapped[gpuidx])) {
> +                       mapped = true;
> +                       break;
> +               }
> +       }
> +out:
> +       pr_debug("prange 0x%p [0x%lx 0x%lx] mapped %d on gpu %d\n", prange,
> +                start, last, mapped, gpuidx);
> +       return mapped;
> +}
> +
> +static bool
> +svm_range_partial_mapped(struct svm_range *prange, unsigned long start,
> +                        unsigned long last)
> +{
> +       struct kfd_process *p = container_of(prange->svms, struct kfd_process, svms);
> +       struct svm_range *pchild;
> +       uint32_t gpuidx;
> +
> +       for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +               list_for_each_entry(pchild, &prange->child_list, child_list) {
> +                       if (svm_range_partial_mapped_dev(gpuidx, pchild, start, last))
> +                               return true;
> +               }
> +
> +               if (svm_range_partial_mapped_dev(gpuidx, prange, start, last))
> +                       return true;
> +       }
> +       return false;
> +}
> +
> +static bool svm_range_is_mapped(struct svm_range *prange)
> +{
> +       return svm_range_partial_mapped(prange, prange->start, prange->last);
> +}
> +
I think svm_range_is_mapped actually means if there is any 
prange->granularity range inside prange is mapped to any gpu, not prange 
got mapped. The name is confusion.
> +static void
> +svm_range_update_mapped(uint32_t gpuidx, struct svm_range *prange,
> +                       unsigned long start, unsigned long last, bool mapped)
> +{
> +       unsigned long index, nbits;
> +
> +       index = (start - prange->start) >> prange->granularity;
> +       nbits = svm_range_mapped_nbits(last - start + 1, prange->granularity);
> +       if (mapped)
> +               bitmap_set(prange->bitmap_mapped[gpuidx], index, nbits);
> +       else
> +               bitmap_clear(prange->bitmap_mapped[gpuidx], index, nbits);
> +       pr_debug("prange 0x%p [0x%lx 0x%lx] update mapped %d nbits %ld gpu %d\n",
> +                prange, start, last, mapped, nbits, gpuidx);
> +}
> +
>   static int
>   svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
>                            unsigned long last, uint32_t trigger)
> @@ -1321,29 +1459,28 @@ svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
>          uint32_t gpuidx;
>          int r = 0;
>
> -       if (!prange->mapped_to_gpu) {
> -               pr_debug("prange 0x%p [0x%lx 0x%lx] not mapped to GPU\n",
> -                        prange, prange->start, prange->last);
> -               return 0;
> -       }
> -
> -       if (prange->start == start && prange->last == last) {
> -               pr_debug("unmap svms 0x%p prange 0x%p\n", prange->svms, prange);
> -               prange->mapped_to_gpu = false;
> -       }
> -
>          bitmap_or(bitmap, prange->bitmap_access, prange->bitmap_aip,
>                    MAX_GPU_INSTANCE);
>          p = container_of(prange->svms, struct kfd_process, svms);
>
>          for_each_set_bit(gpuidx, bitmap, MAX_GPU_INSTANCE) {
> -               pr_debug("unmap from gpu idx 0x%x\n", gpuidx);
>                  pdd = kfd_process_device_from_gpuidx(p, gpuidx);
>                  if (!pdd) {
>                          pr_debug("failed to find device idx %d\n", gpuidx);
> -                       return -EINVAL;
> +                       continue;
> +               }
> +
> +               if (!svm_range_partial_mapped_dev(gpuidx, prange, start, last)) {
> +                       pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] not mapped on gpu %d\n",
> +                                prange->svms, prange, start, last, gpuidx);
> +                       continue;
>                  }
>
> +               svm_range_update_mapped(gpuidx, prange, start, last, false);
> +
> +               pr_debug("unmap svms 0x%p prange 0x%p [0x%lx 0x%lx] from gpu %d\n",
> +                        prange->svms, prange, start, last, gpuidx);
> +
>                  kfd_smi_event_unmap_from_gpu(pdd->dev, p->lead_thread->pid,
>                                               start, last, trigger);
>
> @@ -1483,6 +1620,10 @@ svm_range_map_to_gpus(struct svm_range *prange, unsigned long offset,
>                  if (r)
>                          break;
>
> +               if (!r)
> +                       svm_range_update_mapped(gpuidx, prange, prange->start + offset,
> +                                               prange->start + offset + npages - 1, true);

svm_range_update_mapped set the mapping bitmap with prange->granularity, 
but the gpu mapping is for [prange->start + offset,  prange->start + 
offset + npages - 1]. The mapping bitmap covered range maybe bigger then 
the range that got mapped.

In following gpu page fault handler you use 
svm_range_partial_mapped_dev(gpuidx, prange, addr, addr) to check if 
addr is mapped. Is there a room left that addr is not mapped, but the 
mapping bitmap covers it? That would cause the page fault at addr never 
got handled.

Regard

Xiaogang

> +
>                  if (fence) {
>                          r = dma_fence_wait(fence, false);
>                          dma_fence_put(fence);
> @@ -1648,7 +1789,7 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>
>          if (bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
>                  bitmap_copy(ctx->bitmap, prange->bitmap_access, MAX_GPU_INSTANCE);
> -               if (!prange->mapped_to_gpu ||
> +               if (!svm_range_is_mapped(prange) ||
>                      bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
>                          r = 0;
>                          goto free_ctx;
> @@ -1729,9 +1870,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>                          r = svm_range_map_to_gpus(prange, offset, npages, readonly,
>                                                    ctx->bitmap, flush_tlb);
>
> -               if (!r && next == end)
> -                       prange->mapped_to_gpu = true;
> -
>                  svm_range_unlock(prange);
>
>                  addr = next;
> @@ -1900,10 +2038,10 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>          if (!p->xnack_enabled ||
>              (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) {
>                  int evicted_ranges;
> -               bool mapped = prange->mapped_to_gpu;
> +               bool mapped = svm_range_is_mapped(prange);
>
>                  list_for_each_entry(pchild, &prange->child_list, child_list) {
> -                       if (!pchild->mapped_to_gpu)
> +                       if (!svm_range_is_mapped(pchild))
>                                  continue;
>                          mapped = true;
>                          mutex_lock_nested(&pchild->lock, 1);
> @@ -1966,7 +2104,9 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>
>   static struct svm_range *svm_range_clone(struct svm_range *old)
>   {
> +       struct kfd_process *p = container_of(old->svms, struct kfd_process, svms);
>          struct svm_range *new;
> +       uint32_t gpuidx;
>
>          new = svm_range_new(old->svms, old->start, old->last, false);
>          if (!new)
> @@ -1988,7 +2128,11 @@ static struct svm_range *svm_range_clone(struct svm_range *old)
>          new->prefetch_loc = old->prefetch_loc;
>          new->actual_loc = old->actual_loc;
>          new->granularity = old->granularity;
> -       new->mapped_to_gpu = old->mapped_to_gpu;
> +       for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +               bitmap_copy(new->bitmap_mapped[gpuidx], old->bitmap_mapped[gpuidx],
> +                           svm_range_mapped_nbits(new->last - new->start + 1,
> +                                                  new->granularity));
> +       };
>          bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
>          bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
>
> @@ -2107,7 +2251,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>                  next_start = min(node->last, last) + 1;
>
>                  if (svm_range_is_same_attrs(p, prange, nattr, attrs) &&
> -                   prange->mapped_to_gpu) {
> +                   svm_range_is_mapped(prange)) {
>                          /* nothing to do */
>                  } else if (node->start < start || node->last > last) {
>                          /* node intersects the update range and its attributes
> @@ -3587,7 +3731,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>
>                  if (migrated && (!p->xnack_enabled ||
>                      (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) &&
> -                   prange->mapped_to_gpu) {
> +                   svm_range_is_mapped(prange)) {
>                          pr_debug("restore_work will update mappings of GPUs\n");
>                          mutex_unlock(&prange->migrate_mutex);
>                          continue;
> @@ -3598,7 +3742,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>                          continue;
>                  }
>
> -               flush_tlb = !migrated && update_mapping && prange->mapped_to_gpu;
> +               flush_tlb = !migrated && update_mapping && svm_range_is_mapped(prange);
>
>                  r = svm_range_validate_and_map(mm, prange, MAX_GPU_INSTANCE,
>                                                 true, flush_tlb);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index d2e94d8fb4be..10c92c5e23a7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -132,7 +132,7 @@ struct svm_range {
>          struct list_head                child_list;
>          DECLARE_BITMAP(bitmap_access, MAX_GPU_INSTANCE);
>          DECLARE_BITMAP(bitmap_aip, MAX_GPU_INSTANCE);
> -       bool                            mapped_to_gpu;
> +       unsigned long                   *bitmap_mapped[MAX_GPU_INSTANCE];
>   };
>
>   static inline void svm_range_lock(struct svm_range *prange)
> @@ -163,6 +163,8 @@ static inline struct svm_range_bo *svm_range_bo_ref(struct svm_range_bo *svm_bo)
>                  return NULL;
>   }
>
> +#define svm_range_mapped_nbits(size, granularity) DIV_ROUND_UP((size), 1UL << (granularity))
> +
>   int svm_range_list_init(struct kfd_process *p);
>   void svm_range_list_fini(struct kfd_process *p);
>   int svm_ioctl(struct kfd_process *p, enum kfd_ioctl_svm_op op, uint64_t start,
> --
> 2.35.1
>

Re: [PATCH 2/3] amd/amdkfd: Unmap range from GPUs based on granularity

[Chen, Xiaogang]

On 9/29/2023 9:11 AM, Philip Yang wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Align unmap range start and last address to granularity boundary.
> Skip unmap if range is already unmapped from GPUs.
>
> This also solve the rocgdb CWSR migration related issue.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 35 ++++++++++++++++++++++++----
>   1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 626e0dd4ec79..ac65bf25c685 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2004,6 +2004,26 @@ static void svm_range_restore_work(struct work_struct *work)
>          mmput(mm);
>   }
>
> +static unsigned long
> +svm_range_align_start(struct svm_range *prange, unsigned long start)
> +{
> +       unsigned long start_align;
> +
> +       start_align = ALIGN_DOWN(start, 1UL << prange->granularity);
> +       start_align = max_t(unsigned long, start_align, prange->start);
> +       return start_align;
> +}
> +
> +static unsigned long
> +svm_range_align_last(struct svm_range *prange, unsigned long last)
> +{
> +       unsigned long last_align;
> +
> +       last_align = ALIGN(last, 1UL << prange->granularity) - 1;

should be ALIGN(last + 1, 1UL << prange->granularity) - 1;? Here last is 
included last page number.

Regards

Xiaogang

> +       last_align = min_t(unsigned long, last_align, prange->last);
> +       return last_align;
> +}
> +
>   /**
>    * svm_range_evict - evict svm range
>    * @prange: svm range structure
> @@ -2078,6 +2098,12 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>                  unsigned long s, l;
>                  uint32_t trigger;
>
> +               if (!svm_range_partial_mapped(prange, start, last)) {
> +                       pr_debug("svms 0x%p [0x%lx 0x%lx] unmapped already\n",
> +                               prange->svms, start, last);
> +                       return 0;
> +               }
> +
>                  if (event == MMU_NOTIFY_MIGRATE)
>                          trigger = KFD_SVM_UNMAP_TRIGGER_MMU_NOTIFY_MIGRATE;
>                  else
> @@ -2085,16 +2111,17 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>
>                  pr_debug("invalidate unmap svms 0x%p [0x%lx 0x%lx] from GPUs\n",
>                           prange->svms, start, last);
> +
>                  list_for_each_entry(pchild, &prange->child_list, child_list) {
>                          mutex_lock_nested(&pchild->lock, 1);
> -                       s = max(start, pchild->start);
> -                       l = min(last, pchild->last);
> +                       s = svm_range_align_start(pchild, start);
> +                       l = svm_range_align_last(pchild, last);
>                          if (l >= s)
>                                  svm_range_unmap_from_gpus(pchild, s, l, trigger);
>                          mutex_unlock(&pchild->lock);
>                  }
> -               s = max(start, prange->start);
> -               l = min(last, prange->last);
> +               s = svm_range_align_start(prange, start);
> +               l = svm_range_align_last(prange, last);
>                  if (l >= s)
>                          svm_range_unmap_from_gpus(prange, s, l, trigger);
>          }
> --
> 2.35.1
>

Re: [PATCH 3/3] drm/amdkfd: Check bitmap_mapped flag to skip retry fault

[Chen, Xiaogang]

On 9/29/2023 9:11 AM, Philip Yang wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Use bitmap_mapped flag to check if range already mapped to the specific
> GPU, to skip the retry fault from different page of the same range.
>
> Remove prange validate_timestamp which is not accurate for multiple
> GPUs.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 24 ++++++++----------------
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h |  1 -
>   2 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index ac65bf25c685..5e063d902a46 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -43,10 +43,6 @@
>
>   #define AMDGPU_SVM_RANGE_RESTORE_DELAY_MS 1
>
> -/* Long enough to ensure no retry fault comes after svm range is restored and
> - * page table is updated.
> - */
> -#define AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING   (2UL * NSEC_PER_MSEC)
>   #if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
>   #define dynamic_svm_range_dump(svms) \
>          _dynamic_func_call_no_desc("svm_range_dump", svm_range_debug_dump, svms)
> @@ -365,7 +361,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>          INIT_LIST_HEAD(&prange->deferred_list);
>          INIT_LIST_HEAD(&prange->child_list);
>          atomic_set(&prange->invalid, 0);
> -       prange->validate_timestamp = 0;
>          mutex_init(&prange->migrate_mutex);
>          mutex_init(&prange->lock);
>
> @@ -1876,8 +1871,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>          }
>
>          svm_range_unreserve_bos(ctx);
> -       if (!r)
> -               prange->validate_timestamp = ktime_get_boottime();
>
>   free_ctx:
>          kfree(ctx);
> @@ -3162,15 +3155,6 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>                  goto out_unlock_range;
>          }
>
> -       /* skip duplicate vm fault on different pages of same range */
> -       if (ktime_before(timestamp, ktime_add_ns(prange->validate_timestamp,
> -                               AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING))) {
> -               pr_debug("svms 0x%p [0x%lx %lx] already restored\n",
> -                        svms, prange->start, prange->last);
> -               r = 0;
> -               goto out_unlock_range;
> -       }
> -
>          /* __do_munmap removed VMA, return success as we are handling stale
>           * retry fault.
>           */
> @@ -3196,6 +3180,14 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>                  goto out_unlock_range;
>          }
>
> +       /* skip duplicate vm fault on different pages of same range */

I think the following call means if the prange->granularity range that 
the addr is in is mapped on gpuidex already, not different pages of same 
range.

Regards

Xiaogang

> +       if (svm_range_partial_mapped_dev(gpuidx, prange, addr, addr)) {
> +               pr_debug("svms 0x%p [0x%lx %lx] already restored on gpu %d\n",
> +                        svms, prange->start, prange->last, gpuidx);
> +               r = 0;
> +               goto out_unlock_range;
> +       }
> +
>          pr_debug("svms %p [0x%lx 0x%lx] best restore 0x%x, actual loc 0x%x\n",
>                   svms, prange->start, prange->last, best_loc,
>                   prange->actual_loc);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index 10c92c5e23a7..3afc33a3dd30 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -125,7 +125,6 @@ struct svm_range {
>          uint32_t                        actual_loc;
>          uint8_t                         granularity;
>          atomic_t                        invalid;
> -       ktime_t                         validate_timestamp;
>          struct mmu_interval_notifier    notifier;
>          struct svm_work_list_item       work_item;
>          struct list_head                deferred_list;
> --
> 2.35.1
>

Re: [PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[Felix Kuehling]

[-- Attachment #1: Type: text/plain, Size: 16520 bytes --]


On 2023-09-29 10:11, Philip Yang wrote:
> Replace prange->mapped_to_gpu with prange->bitmap_mapped[], which is
> based on prange granularity, updated when map to GPUS or unmap from
> GPUs, to optimize multiple GPU map, unmap and retry fault recover.
>
> svm_range_is_mapped is false only if no parital range mapping on any
> GPUs.
>
> Split the bitmap_mapped when unmap from cpu to split the prange.
>
> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 218 ++++++++++++++++++++++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h |   4 +-
>   2 files changed, 184 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 040dc32ad475..626e0dd4ec79 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -292,12 +292,12 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>   					KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0);
>   	}
>   
> -	/* free dma_addr array for each gpu */
> +	/* free dma_addr array, bitmap_mapped for each gpu */
>   	for (gpuidx = 0; gpuidx < MAX_GPU_INSTANCE; gpuidx++) {
> -		if (prange->dma_addr[gpuidx]) {
> +		if (prange->dma_addr[gpuidx])
>   			kvfree(prange->dma_addr[gpuidx]);
> -				prange->dma_addr[gpuidx] = NULL;
> -		}
> +		if (prange->bitmap_mapped[gpuidx])
> +			bitmap_free(prange->bitmap_mapped[gpuidx]);
>   	}
>   
>   	mutex_destroy(&prange->lock);
> @@ -323,19 +323,38 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>   	uint64_t size = last - start + 1;
>   	struct svm_range *prange;
>   	struct kfd_process *p;
> -
> -	prange = kzalloc(sizeof(*prange), GFP_KERNEL);
> -	if (!prange)
> -		return NULL;
> +	unsigned int nbits;
> +	uint32_t gpuidx;
>   
>   	p = container_of(svms, struct kfd_process, svms);
>   	if (!p->xnack_enabled && update_mem_usage &&
>   	    amdgpu_amdkfd_reserve_mem_limit(NULL, size << PAGE_SHIFT,
>   				    KFD_IOC_ALLOC_MEM_FLAGS_USERPTR, 0)) {
>   		pr_info("SVM mapping failed, exceeds resident system memory limit\n");
> -		kfree(prange);
>   		return NULL;
>   	}
> +
> +	prange = kzalloc(sizeof(*prange), GFP_KERNEL);
> +	if (!prange)
> +		return NULL;
> +
> +	svm_range_set_default_attributes(&prange->preferred_loc,
> +					 &prange->prefetch_loc,
> +					 &prange->granularity, &prange->flags);
> +
> +	nbits = svm_range_mapped_nbits(size, prange->granularity);
> +	pr_debug("prange 0x%p [0x%llx 0x%llx] bitmap_mapped nbits %d\n", prange,
> +		 start, last, nbits);
> +	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +		prange->bitmap_mapped[gpuidx] = bitmap_zalloc(nbits, GFP_KERNEL);
> +		if (!prange->bitmap_mapped[gpuidx]) {
> +			while (gpuidx--)
> +				bitmap_free(prange->bitmap_mapped[gpuidx]);
> +			kfree(prange);
> +			return NULL;
> +		}
> +	}
> +
>   	prange->npages = size;
>   	prange->svms = svms;
>   	prange->start = start;
> @@ -354,10 +373,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>   		bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>   			    MAX_GPU_INSTANCE);
>   
> -	svm_range_set_default_attributes(&prange->preferred_loc,
> -					 &prange->prefetch_loc,
> -					 &prange->granularity, &prange->flags);
> -
>   	pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>   
>   	return prange;
> @@ -972,6 +987,48 @@ svm_range_split_nodes(struct svm_range *new, struct svm_range *old,
>   	return 0;
>   }
>   
> +static int
> +svm_range_split_bitmap_mapped(struct svm_range *new, struct svm_range *old,
> +			      uint64_t start, uint64_t last)
> +{
> +	struct kfd_process *p = container_of(new->svms, struct kfd_process, svms);
> +	unsigned int nbits, old_nbits, old_nbits2;
> +	unsigned long *bits;
> +	uint32_t gpuidx;
> +
> +	nbits = svm_range_mapped_nbits(new->npages, new->granularity);
> +	old_nbits = svm_range_mapped_nbits(old->npages, old->granularity);
> +	old_nbits2 = svm_range_mapped_nbits(last - start + 1, old->granularity);

This may be off by one if start and last are not aligned on granularity 
boundaries. I think you need to calculate the index for each of start 
and last and subtract the indices. E.g. granularity = 9, start = 511, 
last = 512. last - start + 1 is 2 and the division tells you you need 
one bit. But this range touches two different granules, so you need two 
bits.


> +
> +	pr_debug("old 0x%p [0x%lx 0x%lx] => [0x%llx 0x%llx] nbits %d => %d\n",
> +		 old, old->start, old->last, start, last, old_nbits, old_nbits2);
> +	pr_debug("new 0x%p [0x%lx 0x%lx] nbits %d\n", new, new->start, new->last,
> +		 nbits);
> +
> +	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +		bits = bitmap_alloc(old_nbits2, GFP_KERNEL);
> +		if (!bits)
> +			return -ENOMEM;
> +
> +		if (start == old->start) {
> +			bitmap_shift_right(new->bitmap_mapped[gpuidx],
> +					   old->bitmap_mapped[gpuidx],
> +					   old_nbits2, old_nbits);
> +			bitmap_shift_right(bits, old->bitmap_mapped[gpuidx], 0,
> +					   old_nbits2);

Isn't this (shift = 0) the same as bitmap_copy?


> +		} else {
> +			bitmap_copy(new->bitmap_mapped[gpuidx],
> +				    old->bitmap_mapped[gpuidx], nbits);
> +			bitmap_shift_right(bits, old->bitmap_mapped[gpuidx],
> +					   nbits, old_nbits);
> +		}
> +		bitmap_free(old->bitmap_mapped[gpuidx]);
> +		old->bitmap_mapped[gpuidx] = bits;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * svm_range_split_adjust - split range and adjust
>    *
> @@ -1012,6 +1069,10 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
>   			return r;
>   	}
>   
> +	r = svm_range_split_bitmap_mapped(new, old, start, last);
> +	if (r)
> +		return r;
> +
>   	old->npages = last - start + 1;
>   	old->start = start;
>   	old->last = last;
> @@ -1020,7 +1081,6 @@ svm_range_split_adjust(struct svm_range *new, struct svm_range *old,
>   	new->prefetch_loc = old->prefetch_loc;
>   	new->actual_loc = old->actual_loc;
>   	new->granularity = old->granularity;
> -	new->mapped_to_gpu = old->mapped_to_gpu;
>   	bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
>   	bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
>   
> @@ -1310,6 +1370,84 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   				      fence);
>   }
>   
> +/**
> + * svm_range_partial_mapped_dev - check if prange mapped on the specific gpu
> + *
> + * @gpuidx: the gpu to check
> + * @prange: prange to check
> + * @start: the start address in pages
> + * @last: the last address in pages
> + *
> + * Return:
> + * true: if any partial range mapped on gpu based on granularity boundary
> + * false: if the entire range not mapped
> + */
> +static bool
> +svm_range_partial_mapped_dev(uint32_t gpuidx, struct svm_range *prange,
> +			     unsigned long start, unsigned long last)
> +{
> +	unsigned long index, off;
> +	bool mapped = false;
> +
> +	start = max(start, prange->start);
> +	last = min(last, prange->last);
> +	if (last < start)
> +		goto out;
> +
> +	for (off = start; off <= last; off += (1UL << prange->granularity)) {

It would be easier to just iterate over indexes instead of offsets. And 
even more efficient to avoid testing individual bits by using a higher 
level bitmap function, e.g. bitmap_find_next_bit E.g.

	unsigned long start_index, last_index;

	start = max(start, prange->start);
	last = min(last, prange->last);
	if (last < start)
		goto out;

	start_index = (start - prange->start) >> prange->granularity;
	last_index = (last - prange->start) >> prange->granularity;
	return find_next_bit(prange->bitmap_mapped[gpuidx], last_index + 1, start_index) <= last_index;
  

> +		index = (off - prange->start) >> prange->granularity;
> +		if (test_bit(index, prange->bitmap_mapped[gpuidx])) {
> +			mapped = true;
> +			break;
> +		}
> +	}
> +out:
> +	pr_debug("prange 0x%p [0x%lx 0x%lx] mapped %d on gpu %d\n", prange,
> +		 start, last, mapped, gpuidx);
> +	return mapped;
> +}
> +
> +static bool
> +svm_range_partial_mapped(struct svm_range *prange, unsigned long start,
> +			 unsigned long last)
> +{
> +	struct kfd_process *p = container_of(prange->svms, struct kfd_process, svms);
> +	struct svm_range *pchild;
> +	uint32_t gpuidx;
> +
> +	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +		list_for_each_entry(pchild, &prange->child_list, child_list) {
> +			if (svm_range_partial_mapped_dev(gpuidx, pchild, start, last))
> +				return true;
> +		}
> +
> +		if (svm_range_partial_mapped_dev(gpuidx, prange, start, last))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool svm_range_is_mapped(struct svm_range *prange)
> +{
> +	return svm_range_partial_mapped(prange, prange->start, prange->last);
> +}
> +
> +static void
> +svm_range_update_mapped(uint32_t gpuidx, struct svm_range *prange,
> +			unsigned long start, unsigned long last, bool mapped)
> +{
> +	unsigned long index, nbits;
> +
> +	index = (start - prange->start) >> prange->granularity;
> +	nbits = svm_range_mapped_nbits(last - start + 1, prange->granularity);

This may be off by one if start and last are not aligned on granularity 
boundaries. I think you need to calculate the index for each of start 
and last and subtract the indices.


> +	if (mapped)
> +		bitmap_set(prange->bitmap_mapped[gpuidx], index, nbits);
> +	else
> +		bitmap_clear(prange->bitmap_mapped[gpuidx], index, nbits);
> +	pr_debug("prange 0x%p [0x%lx 0x%lx] update mapped %d nbits %ld gpu %d\n",
> +		 prange, start, last, mapped, nbits, gpuidx);
> +}
> +
>   static int
>   svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
>   			  unsigned long last, uint32_t trigger)
> @@ -1321,29 +1459,28 @@ svm_range_unmap_from_gpus(struct svm_range *prange, unsigned long start,
>   	uint32_t gpuidx;
>   	int r = 0;
>   
> -	if (!prange->mapped_to_gpu) {
> -		pr_debug("prange 0x%p [0x%lx 0x%lx] not mapped to GPU\n",
> -			 prange, prange->start, prange->last);
> -		return 0;
> -	}
> -
> -	if (prange->start == start && prange->last == last) {
> -		pr_debug("unmap svms 0x%p prange 0x%p\n", prange->svms, prange);
> -		prange->mapped_to_gpu = false;
> -	}
> -
>   	bitmap_or(bitmap, prange->bitmap_access, prange->bitmap_aip,
>   		  MAX_GPU_INSTANCE);
>   	p = container_of(prange->svms, struct kfd_process, svms);
>   
>   	for_each_set_bit(gpuidx, bitmap, MAX_GPU_INSTANCE) {
> -		pr_debug("unmap from gpu idx 0x%x\n", gpuidx);
>   		pdd = kfd_process_device_from_gpuidx(p, gpuidx);
>   		if (!pdd) {
>   			pr_debug("failed to find device idx %d\n", gpuidx);
> -			return -EINVAL;
> +			continue;
> +		}
> +
> +		if (!svm_range_partial_mapped_dev(gpuidx, prange, start, last)) {
> +			pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] not mapped on gpu %d\n",
> +				 prange->svms, prange, start, last, gpuidx);
> +			continue;
>   		}
>   
> +		svm_range_update_mapped(gpuidx, prange, start, last, false);
> +
> +		pr_debug("unmap svms 0x%p prange 0x%p [0x%lx 0x%lx] from gpu %d\n",
> +			 prange->svms, prange, start, last, gpuidx);
> +
>   		kfd_smi_event_unmap_from_gpu(pdd->dev, p->lead_thread->pid,
>   					     start, last, trigger);
>   
> @@ -1483,6 +1620,10 @@ svm_range_map_to_gpus(struct svm_range *prange, unsigned long offset,
>   		if (r)
>   			break;
>   
> +		if (!r)
> +			svm_range_update_mapped(gpuidx, prange, prange->start + offset,
> +						prange->start + offset + npages - 1, true);
> +
>   		if (fence) {
>   			r = dma_fence_wait(fence, false);
>   			dma_fence_put(fence);
> @@ -1648,7 +1789,7 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>   
>   	if (bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
>   		bitmap_copy(ctx->bitmap, prange->bitmap_access, MAX_GPU_INSTANCE);
> -		if (!prange->mapped_to_gpu ||
> +		if (!svm_range_is_mapped(prange) ||

I think this gives you the wrong answer. As I understand it, 
svm_range_is_mapped returns true, if any part of the range is currently 
mapped on any GPU. But you'd only want to skip validate_and_map is all 
of the range is currently mapped on the subset of GPUs you're interested in.


>   		    bitmap_empty(ctx->bitmap, MAX_GPU_INSTANCE)) {
>   			r = 0;
>   			goto free_ctx;
> @@ -1729,9 +1870,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>   			r = svm_range_map_to_gpus(prange, offset, npages, readonly,
>   						  ctx->bitmap, flush_tlb);
>   
> -		if (!r && next == end)
> -			prange->mapped_to_gpu = true;
> -
>   		svm_range_unlock(prange);
>   
>   		addr = next;
> @@ -1900,10 +2038,10 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>   	if (!p->xnack_enabled ||
>   	    (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) {
>   		int evicted_ranges;
> -		bool mapped = prange->mapped_to_gpu;
> +		bool mapped = svm_range_is_mapped(prange);
>   
>   		list_for_each_entry(pchild, &prange->child_list, child_list) {
> -			if (!pchild->mapped_to_gpu)
> +			if (!svm_range_is_mapped(pchild))
>   				continue;
>   			mapped = true;
Doesn't svm_range_is_mapped already consider child ranges? So you don't 
need to set mapped here.


>   			mutex_lock_nested(&pchild->lock, 1);
> @@ -1966,7 +2104,9 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>   
>   static struct svm_range *svm_range_clone(struct svm_range *old)
>   {
> +	struct kfd_process *p = container_of(old->svms, struct kfd_process, svms);
>   	struct svm_range *new;
> +	uint32_t gpuidx;
>   
>   	new = svm_range_new(old->svms, old->start, old->last, false);
>   	if (!new)
> @@ -1988,7 +2128,11 @@ static struct svm_range *svm_range_clone(struct svm_range *old)
>   	new->prefetch_loc = old->prefetch_loc;
>   	new->actual_loc = old->actual_loc;
>   	new->granularity = old->granularity;
> -	new->mapped_to_gpu = old->mapped_to_gpu;
> +	for_each_set_bit(gpuidx, p->svms.bitmap_supported, p->n_pdds) {
> +		bitmap_copy(new->bitmap_mapped[gpuidx], old->bitmap_mapped[gpuidx],
> +			    svm_range_mapped_nbits(new->last - new->start + 1,
> +						   new->granularity));
> +	};
>   	bitmap_copy(new->bitmap_access, old->bitmap_access, MAX_GPU_INSTANCE);
>   	bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
>   
> @@ -2107,7 +2251,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>   		next_start = min(node->last, last) + 1;
>   
>   		if (svm_range_is_same_attrs(p, prange, nattr, attrs) &&
> -		    prange->mapped_to_gpu) {
> +		    svm_range_is_mapped(prange)) {

This is probably wrong too. I think you need a check, whether a specific 
range is completely mapped on all GPUs that need access.

Regards,
   Felix


>   			/* nothing to do */
>   		} else if (node->start < start || node->last > last) {
>   			/* node intersects the update range and its attributes
> @@ -3587,7 +3731,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>   
>   		if (migrated && (!p->xnack_enabled ||
>   		    (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)) &&
> -		    prange->mapped_to_gpu) {
> +		    svm_range_is_mapped(prange)) {
>   			pr_debug("restore_work will update mappings of GPUs\n");
>   			mutex_unlock(&prange->migrate_mutex);
>   			continue;
> @@ -3598,7 +3742,7 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>   			continue;
>   		}
>   
> -		flush_tlb = !migrated && update_mapping && prange->mapped_to_gpu;
> +		flush_tlb = !migrated && update_mapping && svm_range_is_mapped(prange);
>   
>   		r = svm_range_validate_and_map(mm, prange, MAX_GPU_INSTANCE,
>   					       true, flush_tlb);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index d2e94d8fb4be..10c92c5e23a7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -132,7 +132,7 @@ struct svm_range {
>   	struct list_head		child_list;
>   	DECLARE_BITMAP(bitmap_access, MAX_GPU_INSTANCE);
>   	DECLARE_BITMAP(bitmap_aip, MAX_GPU_INSTANCE);
> -	bool				mapped_to_gpu;
> +	unsigned long			*bitmap_mapped[MAX_GPU_INSTANCE];
>   };
>   
>   static inline void svm_range_lock(struct svm_range *prange)
> @@ -163,6 +163,8 @@ static inline struct svm_range_bo *svm_range_bo_ref(struct svm_range_bo *svm_bo)
>   		return NULL;
>   }
>   
> +#define svm_range_mapped_nbits(size, granularity) DIV_ROUND_UP((size), 1UL << (granularity))
> +
>   int svm_range_list_init(struct kfd_process *p);
>   void svm_range_list_fini(struct kfd_process *p);
>   int svm_ioctl(struct kfd_process *p, enum kfd_ioctl_svm_op op, uint64_t start,

[-- Attachment #2: Type: text/html, Size: 18197 bytes --]

Re: [PATCH 2/3] amd/amdkfd: Unmap range from GPUs based on granularity

[Felix Kuehling]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

On 2023-09-29 10:11, Philip Yang wrote:
> Align unmap range start and last address to granularity boundary.
> Skip unmap if range is already unmapped from GPUs.

This only handles unmap due to MMU notifiers with XNACK on. What about 
svm_range_unmap_from_cpu?

Regards,
   Felix


>
> This also solve the rocgdb CWSR migration related issue.
>
> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 35 ++++++++++++++++++++++++----
>   1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 626e0dd4ec79..ac65bf25c685 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2004,6 +2004,26 @@ static void svm_range_restore_work(struct work_struct *work)
>   	mmput(mm);
>   }
>   
> +static unsigned long
> +svm_range_align_start(struct svm_range *prange, unsigned long start)
> +{
> +	unsigned long start_align;
> +
> +	start_align = ALIGN_DOWN(start, 1UL << prange->granularity);
> +	start_align = max_t(unsigned long, start_align, prange->start);
> +	return start_align;
> +}
> +
> +static unsigned long
> +svm_range_align_last(struct svm_range *prange, unsigned long last)
> +{
> +	unsigned long last_align;
> +
> +	last_align = ALIGN(last, 1UL << prange->granularity) - 1;

I think this should be

	last_align = ALIGN(last + 1, 1UL << prange->granularity) - 1;

Otherwise you're off by one granule when (last & (1UL << 
prange->granularity)) == 0.


> +	last_align = min_t(unsigned long, last_align, prange->last);
> +	return last_align;
> +}
> +
>   /**
>    * svm_range_evict - evict svm range
>    * @prange: svm range structure
> @@ -2078,6 +2098,12 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>   		unsigned long s, l;
>   		uint32_t trigger;
>   
> +		if (!svm_range_partial_mapped(prange, start, last)) {
> +			pr_debug("svms 0x%p [0x%lx 0x%lx] unmapped already\n",
> +				prange->svms, start, last);
> +			return 0;
> +		}
> +
>   		if (event == MMU_NOTIFY_MIGRATE)
>   			trigger = KFD_SVM_UNMAP_TRIGGER_MMU_NOTIFY_MIGRATE;
>   		else
> @@ -2085,16 +2111,17 @@ svm_range_evict(struct svm_range *prange, struct mm_struct *mm,
>   
>   		pr_debug("invalidate unmap svms 0x%p [0x%lx 0x%lx] from GPUs\n",
>   			 prange->svms, start, last);
> +
>   		list_for_each_entry(pchild, &prange->child_list, child_list) {
>   			mutex_lock_nested(&pchild->lock, 1);
> -			s = max(start, pchild->start);
> -			l = min(last, pchild->last);
> +			s = svm_range_align_start(pchild, start);
> +			l = svm_range_align_last(pchild, last);
>   			if (l >= s)
>   				svm_range_unmap_from_gpus(pchild, s, l, trigger);
>   			mutex_unlock(&pchild->lock);
>   		}
> -		s = max(start, prange->start);
> -		l = min(last, prange->last);
> +		s = svm_range_align_start(prange, start);
> +		l = svm_range_align_last(prange, last);
>   		if (l >= s)
>   			svm_range_unmap_from_gpus(prange, s, l, trigger);
>   	}

[-- Attachment #2: Type: text/html, Size: 3784 bytes --]

Re: [PATCH 3/3] drm/amdkfd: Check bitmap_mapped flag to skip retry fault

[Felix Kuehling]

On 2023-09-29 10:11, Philip Yang wrote:
> Use bitmap_mapped flag to check if range already mapped to the specific
> GPU, to skip the retry fault from different page of the same range.
>
> Remove prange validate_timestamp which is not accurate for multiple
> GPUs.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 24 ++++++++----------------
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h |  1 -
>   2 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index ac65bf25c685..5e063d902a46 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -43,10 +43,6 @@
>   
>   #define AMDGPU_SVM_RANGE_RESTORE_DELAY_MS 1
>   
> -/* Long enough to ensure no retry fault comes after svm range is restored and
> - * page table is updated.
> - */
> -#define AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING	(2UL * NSEC_PER_MSEC)
>   #if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
>   #define dynamic_svm_range_dump(svms) \
>   	_dynamic_func_call_no_desc("svm_range_dump", svm_range_debug_dump, svms)
> @@ -365,7 +361,6 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>   	INIT_LIST_HEAD(&prange->deferred_list);
>   	INIT_LIST_HEAD(&prange->child_list);
>   	atomic_set(&prange->invalid, 0);
> -	prange->validate_timestamp = 0;
>   	mutex_init(&prange->migrate_mutex);
>   	mutex_init(&prange->lock);
>   
> @@ -1876,8 +1871,6 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
>   	}
>   
>   	svm_range_unreserve_bos(ctx);
> -	if (!r)
> -		prange->validate_timestamp = ktime_get_boottime();
>   
>   free_ctx:
>   	kfree(ctx);
> @@ -3162,15 +3155,6 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   		goto out_unlock_range;
>   	}
>   
> -	/* skip duplicate vm fault on different pages of same range */
> -	if (ktime_before(timestamp, ktime_add_ns(prange->validate_timestamp,
> -				AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING))) {
> -		pr_debug("svms 0x%p [0x%lx %lx] already restored\n",
> -			 svms, prange->start, prange->last);
> -		r = 0;
> -		goto out_unlock_range;
> -	}
> -
>   	/* __do_munmap removed VMA, return success as we are handling stale
>   	 * retry fault.
>   	 */
> @@ -3196,6 +3180,14 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   		goto out_unlock_range;
>   	}
>   
> +	/* skip duplicate vm fault on different pages of same range */
> +	if (svm_range_partial_mapped_dev(gpuidx, prange, addr, addr)) {
> +		pr_debug("svms 0x%p [0x%lx %lx] already restored on gpu %d\n",
> +			 svms, prange->start, prange->last, gpuidx);
> +		r = 0;
> +		goto out_unlock_range;
> +	}
> +
>   	pr_debug("svms %p [0x%lx 0x%lx] best restore 0x%x, actual loc 0x%x\n",
>   		 svms, prange->start, prange->last, best_loc,
>   		 prange->actual_loc);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index 10c92c5e23a7..3afc33a3dd30 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -125,7 +125,6 @@ struct svm_range {
>   	uint32_t			actual_loc;
>   	uint8_t				granularity;
>   	atomic_t			invalid;
> -	ktime_t				validate_timestamp;
>   	struct mmu_interval_notifier	notifier;
>   	struct svm_work_list_item	work_item;
>   	struct list_head		deferred_list;

Re: [PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[Philip Yang]

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

Re: [PATCH 1/3] amd/amdkfd: Add granularity bitmap mapped to gpu flag

[Philip Yang]

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

Re: [PATCH 2/3] amd/amdkfd: Unmap range from GPUs based on granularity

[Philip Yang]

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

Re: [PATCH 2/3] amd/amdkfd: Unmap range from GPUs based on granularity

[Philip Yang]

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

Re: [PATCH 3/3] drm/amdkfd: Check bitmap_mapped flag to skip retry fault

[Philip Yang]

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