<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word"> [AMD Official Use Only - AMD Internal Distribution Only] <div> <div class="WordSection1"> <o:p> </o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Yang, Philip <Philip.Yang@amd.com> Sent: Tuesday, January 7, 2025 10:04 PM To: Deng, Emily <Emily.Deng@amd.com>; Yang, Philip <Philip.Yang@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdkfd: Fix partial migrate issue<o:p></o:p> </div> </div> <o:p> </o:p> <o:p> </o:p> <div> On 2025-01-06 21:31, Deng, Emily wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> [AMD Official Use Only - AMD Internal Distribution Only]<o:p></o:p> <o:p> </o:p> <div> <o:p></o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Yang, Philip <a href="mailto:Philip.Yang@amd.com"><Philip.Yang@amd.com></a> Sent: Tuesday, January 7, 2025 7:51 AM To: Deng, Emily <a href="mailto:Emily.Deng@amd.com"><Emily.Deng@amd.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH] drm/amdkfd: Fix partial migrate issue<o:p></o:p> </div> </div> <o:p></o:p> <o:p></o:p> <div> On 2025-01-02 19:06, Emily Deng wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>For partial migrate from ram to vram, the migrate->cpages is not<o:p></o:p></pre> <pre>equal to migrate->npages, should use migrate->npages to check all needed<o:p></o:p></pre> <pre>migrate pages which could be copied or not.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>And only need to set those pages could be migrated to migrate->dst[i], or<o:p></o:p></pre> <pre>the migrate_vma_pages will migrate the wrong pages based on the migrate->dst[i].<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Signed-off-by: Emily Deng <a href="mailto:Emily.Deng@amd.com"><Emily.Deng@amd.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 8 ++++----<o:p></o:p></pre> <pre> 1 file changed, 4 insertions(+), 4 deletions(-)<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c<o:p></o:p></pre> <pre>index 4b275937d05e..5c96c2d425e3 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c<o:p></o:p></pre> <pre>@@ -278,7 +278,7 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,<o:p></o:p></pre> <pre> struct migrate_vma *migrate, struct dma_fence **mfence,<o:p></o:p></pre> <pre> dma_addr_t *scratch, uint64_t ttm_res_offset)<o:p></o:p></pre> <pre> {<o:p></o:p></pre> <pre>- uint64_t npages = migrate->cpages;<o:p></o:p></pre> <pre>+ uint64_t npages = migrate->npages;<o:p></o:p></pre> </blockquote> 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.<o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre> <o:p></o:p></pre> <pre> struct amdgpu_device *adev = node->adev;<o:p></o:p></pre> <pre> struct device *dev = adev->dev;<o:p></o:p></pre> <pre> struct amdgpu_res_cursor cursor;<o:p></o:p></pre> <pre>@@ -299,9 +299,6 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,<o:p></o:p></pre> <pre> struct page *spage;<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> dst[i] = cursor.start + (j << PAGE_SHIFT);<o:p></o:p></pre> <pre>- migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);<o:p></o:p></pre> <pre>- svm_migrate_get_vram_page(prange, migrate->dst[i]);<o:p></o:p></pre> <pre>- migrate->dst[i] = migrate_pfn(migrate->dst[i]);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> spage = migrate_pfn_to_page(migrate->src[i]);<o:p></o:p></pre> <pre> if (spage && !is_zone_device_page(spage)) {<o:p></o:p></pre> <pre>@@ -345,6 +342,9 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,<o:p></o:p></pre> <pre> } else {<o:p></o:p></pre> <pre> j++;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre>+ migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);<o:p></o:p></pre> <pre>+ svm_migrate_get_vram_page(prange, migrate->dst[i]);<o:p></o:p></pre> <pre>+ migrate->dst[i] = migrate_pfn(migrate->dst[i]);<o:p></o:p></pre> </blockquote> This should move forward, to handle the corner case to migrate 1 page to the last page of VRAM res cursor.<o:p></o:p> 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<o:p></o:p> </div> </div> </blockquote> migrate->dst[i] should set before this if condition (not after if) to setup sdma copy migrate->src to migrate->dst, if condition is true for the case migrating to the last page of VRAM res cusor, as the VRAM physical address is not contiguous.<o:p></o:p> if (j >= (cursor.size >> PAGE_SHIFT) - 1 && i < npages - 1) {<o:p></o:p> Regards,<o:p></o:p> Philip<o:p></o:p> Maybe I am wrong, but I couldn’t find anywhere in svm_migrate_copy_memory_gart will use migrate->dst[i], it uses the dst[i] directly. So, I think it will have no effect. The reason why I put it to the tail is because if have some exceptions, for example “if (r) goto out_free_vram_pages;”, for this case, I think we need to treat the copy as fail, and couldn’t migrate these pages, so also couldn’t set the migrate->dst[i].<o:p></o:p> <o:p> </o:p> Emily Deng<o:p></o:p> Best Wishes<o:p></o:p> </div> <o:p> </o:p> <o:p> </o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> 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.<o:p></o:p> Please check this change, to add mpages accounting to break the loop earlier. <o:p> </o:p> Good suggestion, I will add mpages in change.<o:p></o:p> <o:p></o:p> Emily Deng<o:p></o:p> Best Wishes<o:p></o:p> </div> <o:p></o:p> <o:p></o:p> - 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 {<o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre> <o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> r = svm_migrate_copy_memory_gart(adev, src + i - j, dst + i - j, j,<o:p></o:p></pre> </blockquote> </div> </blockquote> </div> </div> </body> </html>