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

[Chen, Xiaogang]

On 1/10/2025 11:33 AM, Philip Yang wrote:
>
>
> On 2025-01-10 11:23, Chen, Xiaogang wrote:
>>
>> On 1/10/2025 8:37 AM, Philip Yang wrote:
>>>
>>>
>>> On 2025-01-10 02:49, 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].
>>>>
>>>> v2:
>>>> Add mpages to break the loop earlier.
>>>>
>>>> v3:
>>>> Uses MIGRATE_PFN_MIGRATE to identify whether page could be migrated.
>>>
>>> The error handling need below change, with that fixed, this patch is
>>>
>>> Reviewed-by: Philip Yang<Philip.Yang at amd.com>
>>>
>>>> Signed-off-by: Emily Deng<Emily.Deng at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 ++++++++++-------
>>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c 
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
>>>> index 4b275937d05e..bfaccabeb3a0 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
>>>> @@ -278,10 +278,11 @@ 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;
>>>>       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,16 @@ 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]);
>>>> -
>>>> +        if (migrate->src[i] & MIGRATE_PFN_MIGRATE) {
>>>> +            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]);
>>>> +            mpages++;
>>>> +        }
>>>>           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,
>>>  out_free_vram_pages:
>>>         if (r) {
>>>                 pr_debug("failed %d to copy memory to vram\n", r);
>>> -               while (i--) {
>>> +
>>> +               for (i = 0; i < npages && mpages; i++) {
>>> +                       if (!dst[i])
>>> +                               continue;
>>>                         svm_migrate_put_vram_page(adev, dst[i]);
>>>                         migrate->dst[i] = 0;
>>> +                       mpages--;
>>>                 }
>>>         }
>>
>> This error handing not need recover all vram pages as error happened 
>> at middle. Can use se do {....} while(i--);
> no, for example migrate npage=cpage=4, and outside for loop, 
> svm_migrate_copy_memory_gart failed, dst[4] is out of range access.

You are right, but it is awkward. Inside loop we update dst[i] before do 
sdam that does not include the dst[i] just updated.

Regard

Xiaogang

>>
>> Regards
>>
>> Xiaogang
>>