RE: [PATCH] drm/amdkfd: Fix partial migrate issue

["Deng, Emily" <Emily.Deng@xxxxxxx>]

[AMD Official Use Only - AMD Internal Distribution Only]

 

From: Yang, Philip <Philip.Yang@xxxxxxx>
Sent: Tuesday, January 7, 2025 7:51 AM
To: Deng, Emily <Emily.Deng@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/amdkfd: Fix partial migrate issue

 

 

On 2025-01-02 19:06, Emily Deng wrote:

For partial migrate from ram to vram, the migrate->cpages is not

equal to migrate->npages, should use migrate->npages to check all needed

migrate pages which could be copied or not.

And only need to set those pages could be migrated to migrate->dst[i], or

the migrate_vma_pages will migrate the wrong pages based on the migrate->dst[i].

Signed-off-by: Emily Deng <Emily.Deng@xxxxxxx>

---

 drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 8 ++++----

 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c

index 4b275937d05e..5c96c2d425e3 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c

@@ -278,7 +278,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,

                         struct migrate_vma *migrate, struct dma_fence **mfence,

                         dma_addr_t *scratch, uint64_t ttm_res_offset)

 {

-       uint64_t npages = migrate->cpages;

+       uint64_t npages = migrate->npages;

As partial migration size is based on prange granularity, by default 2MB, maybe always migrate->cpages equal to migrate->npages, that's why we didn't trigger this bug. Wondering how do you catch this bug? This bug will leak svm_bo too, as svm_migrate_get_vram_page reference counter is incorrect.

        struct amdgpu_device *adev = node->adev;

        struct device *dev = adev->dev;

        struct amdgpu_res_cursor cursor;

@@ -299,9 +299,6 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,

                struct page *spage;

                dst[i] = cursor.start + (j << PAGE_SHIFT);

-               migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);

-        svm_migrate_get_vram_page(prange, migrate->dst[i]);

-               migrate->dst[i] = migrate_pfn(migrate->dst[i]);

                spage = migrate_pfn_to_page(migrate->src[i]);

                if (spage && !is_zone_device_page(spage)) {

@@ -345,6 +342,9 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,

                } else {

                        j++;

                }

+               migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);

+        svm_migrate_get_vram_page(prange, migrate->dst[i]);

+               migrate->dst[i] = migrate_pfn(migrate->dst[i]);

This should move forward, to handle the corner case to migrate 1 page to the last page of VRAM res cursor.

Sorry, don’t understand what is the corner case? And the place you put has not much different with mine, as it doesn’t have any jump or continue in the code between your

code place and mine. But it has “if (r) goto out_free_vram_pages;” to handle the error case. For error case, I think the migrate->dst[i] also need to be set to null.

Please check this change, to add mpages accounting to break the loop earlier.

Good suggestion, I will add mpages in change.

 

Emily Deng

Best Wishes

 

 

-       uint64_t npages = migrate->cpages;
+       uint64_t npages = migrate->npages;
        struct amdgpu_device *adev = node->adev;
        struct device *dev = adev->dev;
        struct amdgpu_res_cursor cursor;
+       uint64_t mpages = 0;
        dma_addr_t *src;
        uint64_t *dst;
        uint64_t i, j;
@@ -295,14 +296,9 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
 
        amdgpu_res_first(prange->ttm_res, ttm_res_offset,
                         npages << PAGE_SHIFT, &cursor);
-       for (i = j = 0; i < npages; i++) {
+       for (i = j = 0; i < npages && mpages < migrate->cpages; i++) {
                struct page *spage;
 
-               dst[i] = cursor.start + (j << PAGE_SHIFT);
-               migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);
-               svm_migrate_get_vram_page(prange, migrate->dst[i]);
-               migrate->dst[i] = migrate_pfn(migrate->dst[i]);
-
                spage = migrate_pfn_to_page(migrate->src[i]);
                if (spage && !is_zone_device_page(spage)) {
                        src[i] = dma_map_page(dev, spage, 0, PAGE_SIZE,
@@ -322,6 +318,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                                                mfence);
                                if (r)
                                        goto out_free_vram_pages;
+                               mpages += j;
                                amdgpu_res_next(&cursor, (j + 1) << PAGE_SHIFT);
                                j = 0;
                        } else {
@@ -333,6 +330,11 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                pr_debug_ratelimited("dma mapping src to 0x%llx, pfn 0x%lx\n",
                                     src[i] >> PAGE_SHIFT, page_to_pfn(spage));
 
+               dst[i] = cursor.start + (j << PAGE_SHIFT);
+               migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);
+               svm_migrate_get_vram_page(prange, migrate->dst[i]);
+               migrate->dst[i] = migrate_pfn(migrate->dst[i]);
+
                if (j >= (cursor.size >> PAGE_SHIFT) - 1 && i < npages - 1) {
                        r = svm_migrate_copy_memory_gart(adev, src + i - j,
                                                         dst + i - j, j + 1,
@@ -340,6 +342,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                                                         mfence);
                        if (r)
                                goto out_free_vram_pages;
+                       mpages += j + 1;
                        amdgpu_res_next(&cursor, (j + 1) * PAGE_SIZE);
                        j = 0;
                } else {
(END)
@@ -322,6 +318,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                                                mfence);
                                if (r)
                                        goto out_free_vram_pages;
+                               mpages += j;
                                amdgpu_res_next(&cursor, (j + 1) << PAGE_SHIFT);
                                j = 0;
                        } else {
@@ -333,6 +330,11 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                pr_debug_ratelimited("dma mapping src to 0x%llx, pfn 0x%lx\n",
                                     src[i] >> PAGE_SHIFT, page_to_pfn(spage));
 
+               dst[i] = cursor.start + (j << PAGE_SHIFT);
+               migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);
+               svm_migrate_get_vram_page(prange, migrate->dst[i]);
+               migrate->dst[i] = migrate_pfn(migrate->dst[i]);
+
                if (j >= (cursor.size >> PAGE_SHIFT) - 1 && i < npages - 1) {
                        r = svm_migrate_copy_memory_gart(adev, src + i - j,
                                                         dst + i - j, j + 1,
@@ -340,6 +342,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
                                                         mfence);
                        if (r)
                                goto out_free_vram_pages;
+                       mpages += j + 1;
                        amdgpu_res_next(&cursor, (j + 1) * PAGE_SIZE);
                        j = 0;
                } else {

        }

        r = svm_migrate_copy_memory_gart(adev, src + i - j, dst + i - j, j,