[PATCH] drm/amdkfd: remap unaligned svm ranges that have split

[Felix Kuehling]

On 2023-10-19 16:53, Philip Yang wrote:
>
>
> On 2023-10-19 16:05, Felix Kuehling wrote:
>>
>> On 2023-10-18 18:26, Alex Sierra wrote:
>>> Split SVM ranges that have been mapped into 2MB page table entries,
>>> require to be remap in case the split has happened in a non-aligned
>>> VA.
>>> [WHY]:
>>> This condition causes the 2MB page table entries be split into 4KB
>>> PTEs.
>>>
>>> Signed-off-by: Alex Sierra <alex.sierra at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 45 
>>> +++++++++++++++++++++-------
>>>   1 file changed, 34 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 7b81233bc9ae..1dd9a1cf2358 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -1104,26 +1104,34 @@ svm_range_split(struct svm_range *prange, 
>>> uint64_t start, uint64_t last,
>>>   }
>>>     static int
>>> -svm_range_split_tail(struct svm_range *prange,
>>> -             uint64_t new_last, struct list_head *insert_list)
>>> +svm_range_split_tail(struct svm_range *prange, uint64_t new_last,
>>> +             struct list_head *insert_list, struct list_head 
>>> *remap_list)
>>>   {
>>>       struct svm_range *tail;
>>>       int r = svm_range_split(prange, prange->start, new_last, &tail);
>>>   -    if (!r)
>>> +    if (!r) {
>>>           list_add(&tail->list, insert_list);
>>> +        if (!IS_ALIGNED(tail->last + 1 - tail->start,
>>> +                1UL << tail->granularity))
>>
>> I'm not sure about this condition. I thought this condition should be 
>> about the point where the range is split, not the size of it. So my 
>> understanding is that this should be
>>
>>         if (!IS_ALIGNED(new_last+1, 1UL << prange->granularity))
>
> I think prange->granularity is not always 9, 512 pages, we should 
> check the original prange size is larger than 512.
>
>           if (!IS_ALIGNED(new_last + 1, 512) && tail->last - 
> prange->start >= 512)
>
That's if you only want to protect against splitting of 2MB pages. If 
you also want to protect against splitting of fragments (together with 
your WIP patch series for the mapping bitmap), then we should use 
granularity.

Regards,
   Felix


>>
>>
>>> + list_add(&tail->update_list, remap_list);
>>> +    }
>>>       return r;
>>>   }
>>>     static int
>>> -svm_range_split_head(struct svm_range *prange,
>>> -             uint64_t new_start, struct list_head *insert_list)
>>> +svm_range_split_head(struct svm_range *prange, uint64_t new_start,
>>> +             struct list_head *insert_list, struct list_head 
>>> *remap_list)
>>>   {
>>>       struct svm_range *head;
>>>       int r = svm_range_split(prange, new_start, prange->last, &head);
>>>   -    if (!r)
>>> +    if (!r) {
>>>           list_add(&head->list, insert_list);
>>> +        if (!IS_ALIGNED(head->last + 1 - head->start,
>>> +                1UL << head->granularity))
>>
>> Similar as above.
>>
>>         if (!IS_ALIGNED(new_start, 1UL << prange->granularity))
>
>           if (!IS_ALIGNED(new_start, 512) && prange->last - 
> head->start >= 512)
>
>>
>> Regards,
>>   Felix
>>
>>
>>> + list_add(&head->update_list, remap_list);
>>> +    }
>>>       return r;
>>>   }
>>>   @@ -2113,7 +2121,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 *remove_list, struct list_head *remap_list)
>>>   {
>>>       unsigned long last = start + size - 1UL;
>>>       struct svm_range_list *svms = &p->svms;
>>> @@ -2129,6 +2137,7 @@ svm_range_add(struct kfd_process *p, uint64_t 
>>> start, uint64_t size,
>>>       INIT_LIST_HEAD(insert_list);
>>>       INIT_LIST_HEAD(remove_list);
>>>       INIT_LIST_HEAD(&new_list);
>>> +    INIT_LIST_HEAD(remap_list);
>>>         node = interval_tree_iter_first(&svms->objects, start, last);
>>>       while (node) {
>>> @@ -2153,6 +2162,7 @@ svm_range_add(struct kfd_process *p, uint64_t 
>>> start, uint64_t size,
>>>               struct svm_range *old = prange;
>>>                 prange = svm_range_clone(old);
>>> +
>
> unnecessary change.
>
> Regards,
>
> Philip
>
>>>               if (!prange) {
>>>                   r = -ENOMEM;
>>>                   goto out;
>>> @@ -2161,18 +2171,17 @@ svm_range_add(struct kfd_process *p, 
>>> uint64_t start, uint64_t size,
>>>               list_add(&old->update_list, remove_list);
>>>               list_add(&prange->list, insert_list);
>>>               list_add(&prange->update_list, update_list);
>>> -
>>>               if (node->start < start) {
>>>                   pr_debug("change old range start\n");
>>>                   r = svm_range_split_head(prange, start,
>>> -                             insert_list);
>>> +                             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,
>>> -                             insert_list);
>>> +                             insert_list, remap_list);
>>>                   if (r)
>>>                       goto out;
>>>               }
>>> @@ -3565,6 +3574,7 @@ svm_range_set_attr(struct kfd_process *p, 
>>> struct mm_struct *mm,
>>>       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;
>>>       struct svm_range *next;
>>> @@ -3596,7 +3606,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);
>>> +              &insert_list, &remove_list, &remap_list);
>>>       if (r) {
>>>           mutex_unlock(&svms->lock);
>>>           mmap_write_unlock(mm);
>>> @@ -3661,6 +3671,19 @@ svm_range_set_attr(struct kfd_process *p, 
>>> struct mm_struct *mm,
>>>               ret = r;
>>>       }
>>>   +    list_for_each_entry(prange, &remap_list, update_list) {
>>> +        pr_debug("Remapping prange 0x%p [0x%lx 0x%lx]\n",
>>> +             prange, prange->start, prange->last);
>>> +        mutex_lock(&prange->migrate_mutex);
>>> +        r = svm_range_validate_and_map(mm, prange, MAX_GPU_INSTANCE,
>>> +                           true, true, prange->mapped_to_gpu);
>>> +        if (r)
>>> +            pr_debug("failed %d on remap svm range\n", r);
>>> +        mutex_unlock(&prange->migrate_mutex);
>>> +        if (r)
>>> +            ret = r;
>>> +    }
>>> +
>>>       dynamic_svm_range_dump(svms);
>>>         mutex_unlock(&svms->lock);