[AMD Official Use Only - AMD Internal Distribution Only]
ping
-----Original Message-----
From: Xiaogang.Chen <xiaogang.c...@amd.com>
Sent: Wednesday, June 26, 2024 10:07 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix <felix.kuehl...@amd.com>; Yang, Philip
<philip.y...@amd.com>; Chen, Xiaogang <xiaogang.c...@amd.com>; Chen, Xiaogang
<xiaogang.c...@amd.com>
Subject: [PATCH v2] drm/amdkfd: Correct svm prange overlapping handling at
svm_range_set_attr ioctl
From: Xiaogang Chen <xiaogang.c...@amd.com>
When user adds new vm range that has overlapping with existing svm pranges
current kfd creats a cloned pragne and split it, then replaces original prange
by it. That destroy original prange locks and the cloned prange locks do not
inherit original prange lock contexts. This may cause issue if code still need
use these locks. In general we should keep using original prange, update its
internal data that got changed during split, then free the cloned prange.
This patch change vm range overlaping handling that does not remove existing
pranges, instead updates it for split and keeps its locks alive.
Signed-off-by: Xiaogang Chen<xiaogang.c...@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 112 ++++++++++++++++++++-------
1 file changed, 82 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 407636a68814..a66b8c96ee14 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1967,7 +1967,8 @@ svm_range_evict(struct svm_range *prange, struct
mm_struct *mm,
return r;
}
-static struct svm_range *svm_range_clone(struct svm_range *old)
+/* create a prange that has same range/size/addr etc info as old */
+static struct svm_range *svm_range_duplicate(struct svm_range *old)
{
struct svm_range *new;
@@ -1999,6 +2000,25 @@ static struct svm_range *svm_range_clone(struct
svm_range *old)
return new;
}
+/* copy range/size/addr info from src to dst prange */ static void
+svm_range_copy(struct svm_range *dst, struct svm_range *src) {
+ dst->npages = src->npages;
+ dst->start = src->start;
+ dst->last = src->last;
+
+ dst->vram_pages = src->vram_pages;
+ dst->offset = src->offset;
+
+ for (int i = 0; i < MAX_GPU_INSTANCE; i++) {
+ if (!src->dma_addr[i])
+ continue;
+
+ memcpy(dst->dma_addr[i], src->dma_addr[i],
+ src->npages * sizeof(*src->dma_addr[i]));
+ }
+}
+
void svm_range_set_max_pages(struct amdgpu_device *adev) {
uint64_t max_pages;
@@ -2057,20 +2077,19 @@ svm_range_split_new(struct svm_range_list *svms,
uint64_t start, uint64_t last,
* @attrs: array of attributes
* @update_list: output, the ranges need validate and update GPU mapping
* @insert_list: output, the ranges need insert to svms
- * @remove_list: output, the ranges are replaced and need remove from svms
* @remap_list: output, remap unaligned svm ranges
*
* Check if the virtual address range has overlap with any existing ranges,
* split partly overlapping ranges and add new ranges in the gaps. All changes
* should be applied to the range_list and interval tree transactionally. If
* any range split or allocation fails, the entire update fails. Therefore any
- * existing overlapping svm_ranges are cloned and the original svm_ranges left
+ * existing overlapping svm_ranges are duplicated and the original
+ svm_ranges left
* unchanged.
*
- * If the transaction succeeds, the caller can update and insert clones and
- * new ranges, then free the originals.
+ * If the transaction succeeds, the caller can update and insert split
+ ranges and
+ * new ranges.
*
- * Otherwise the caller can free the clones and new ranges, while the old
+ * Otherwise the caller can free the duplicated and new ranges, while
+ the old
* svm_ranges remain unchanged.
*
* Context: Process context, caller must hold svms->lock @@ -2082,7 +2101,7 @@
static int svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs,
struct list_head *update_list, struct list_head *insert_list,
- struct list_head *remove_list, struct list_head *remap_list)
+ struct list_head *remap_list)
{
unsigned long last = start + size - 1UL;
struct svm_range_list *svms = &p->svms; @@ -2090,13 +2109,14 @@
svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
struct svm_range *prange;
struct svm_range *tmp;
struct list_head new_list;
+ struct list_head modify_list;
int r = 0;
pr_debug("svms 0x%p [0x%llx 0x%lx]\n", &p->svms, start, last);
INIT_LIST_HEAD(update_list);
INIT_LIST_HEAD(insert_list);
- INIT_LIST_HEAD(remove_list);
+ INIT_LIST_HEAD(&modify_list);
INIT_LIST_HEAD(&new_list);
INIT_LIST_HEAD(remap_list);
@@ -2117,35 +2137,41 @@ svm_range_add(struct kfd_process *p, uint64_t start,
uint64_t size,
/* nothing to do */
} else if (node->start < start || node->last > last) {
/* node intersects the update range and its attributes
- * will change. Clone and split it, apply updates only
+ * will change. duplicate and split it, apply updates
only
* to the overlapping part
*/
- struct svm_range *old = prange;
+ /* prange_dup is a temperal prange that holds size and
addr info
+ * updates of pragne after split
+ */
+ struct svm_range *prange_dup;
- prange = svm_range_clone(old);
- if (!prange) {
+ prange_dup = svm_range_duplicate(prange);
+ if (!prange_dup) {
r = -ENOMEM;
goto out;
}
- list_add(&old->update_list, remove_list);
- list_add(&prange->list, insert_list);
- list_add(&prange->update_list, update_list);
-
+ /* split prange_dup */
if (node->start < start) {
pr_debug("change old range start\n");
- r = svm_range_split_head(prange, start,
+ r = svm_range_split_head(prange_dup, start,
insert_list,
remap_list);
if (r)
goto out;
}
if (node->last > last) {
pr_debug("change old range last\n");
- r = svm_range_split_tail(prange, last,
+ r = svm_range_split_tail(prange_dup, last,
insert_list,
remap_list);
if (r)
goto out;
}
+
+ /* split success, insert_list has new head/tail pranges
*/
+ /* move prange from svm list to modify list */
+ list_move_tail(&prange->list, &modify_list);
+ /* put prange_dup at pragne->update_list */
+ list_add(&prange_dup->list, &prange->update_list);
} else {
/* The node is contained within start..last,
* just update it
@@ -2178,8 +2204,38 @@ svm_range_add(struct kfd_process *p, uint64_t start,
uint64_t size,
svm_range_free(prange, false);
list_for_each_entry_safe(prange, tmp, &new_list, list)
svm_range_free(prange, true);
+
+ list_for_each_entry_safe(prange, tmp, &modify_list, list) {
+ struct svm_range *prange_dup;
+
+ /* free pragne_dup that is associated with prange on
modify_list */
+ prange_dup = list_first_entry(&prange->update_list,
struct svm_range, list);
+ if (prange_dup)
+ svm_range_free(prange_dup, false);
+
+ INIT_LIST_HEAD(&prange->update_list);
+ /* put prange back to svm list */
+ list_move_tail(&prange->list, &svms->list);
+ }
} else {
list_splice(&new_list, insert_list);
+
+ list_for_each_entry_safe(prange, tmp, &modify_list, list) {
+ struct svm_range *prange_dup;
+
+ prange_dup = list_first_entry(&prange->update_list,
struct svm_range, list);
+ if (prange_dup) {
+ /* update prange from prange_dup */
+ svm_range_copy(prange, prange_dup);
+ /* release temporal pragne_dup */
+ svm_range_free(prange_dup, false);
+ }
+ INIT_LIST_HEAD(&prange->update_list);
+
+ /* move prange from modify_list to insert_list and
update_list*/
+ list_move_tail(&prange->list, insert_list);
+ list_add(&prange->update_list, update_list);
+ }
}
return r;
@@ -3533,7 +3589,6 @@ svm_range_set_attr(struct kfd_process *p, struct
mm_struct *mm,
struct amdkfd_process_info *process_info = p->kgd_process_info;
struct list_head update_list;
struct list_head insert_list;
- struct list_head remove_list;
struct list_head remap_list;
struct svm_range_list *svms;
struct svm_range *prange;
@@ -3566,7 +3621,7 @@ svm_range_set_attr(struct kfd_process *p, struct
mm_struct *mm,
/* Add new range and split existing ranges as needed */
r = svm_range_add(p, start, size, nattr, attrs, &update_list,
- &insert_list, &remove_list, &remap_list);
+ &insert_list, &remap_list);
if (r) {
mutex_unlock(&svms->lock);
mmap_write_unlock(mm);
@@ -3574,21 +3629,18 @@ svm_range_set_attr(struct kfd_process *p, struct
mm_struct *mm,
}
/* Apply changes as a transaction */
list_for_each_entry_safe(prange, next, &insert_list, list) {
- svm_range_add_to_svms(prange);
- svm_range_add_notifier_locked(mm, prange);
+ /* prange can be new or existing range, put it at svms->list */
+ list_move_tail(&prange->list, &prange->svms->list);
+ /* update prange at interval trees: svms->objects and
+ * mm interval notifier tree
+ */
+ svm_range_update_notifier_and_interval_tree(mm, prange);
+
}
list_for_each_entry(prange, &update_list, update_list) {
svm_range_apply_attrs(p, prange, nattr, attrs, &update_mapping);
/* TODO: unmap ranges from GPU that lost access */
}
- list_for_each_entry_safe(prange, next, &remove_list, update_list) {
- pr_debug("unlink old 0x%p prange 0x%p [0x%lx 0x%lx]\n",
- prange->svms, prange, prange->start,
- prange->last);
- svm_range_unlink(prange);
- svm_range_remove_notifier(prange);
- svm_range_free(prange, false);
- }
mmap_write_downgrade(mm);
/* Trigger migrations and revalidate and map to GPUs as needed. If
--
2.25.1
