On 2025-03-04 13:23, Chen, Xiaogang wrote:
>
>
> On 3/3/2025 11:21 PM, Felix Kuehling wrote:
>> On 2025-01-31 11:58, Xiaogang.Chen wrote:
>>> From: Xiaogang Chen <xiaogang.chen@xxxxxxx>
>>>
>>> When register a vm range at svm the added vm range may be split into multiple
>>> subranges and/or existing pranges got spitted. The new pranges need validated
>>> and mapped. This patch changes error handling for pranges that fail updating:
>> It may help if you clearly state the problem you're trying to solve to justify the changes in error handling. See more comments inline.
>>
> Current way is returning the last sub range error code if it got issue during migration, validation or map. If the last error is -EAGAIN, but there are other error codes at middle for other sub ranges we still return -EAGAIN. That causes same procedure repeated until the sub ranges that have other error code becomes the last one.
>
> I noticed it when looked at large range(more than 100GB) registration which split into multiple sub ranges. There were multiple unnecessary repeats until hit return code that is no -EAGAIN.
>
> As you said we may return immediately if hit no -EAGAIN, and hope app terminates. But if app does not terminate kfd drive will hold unused pranges until app stops.
>
>>> 1: free prange resources and remove it from svms if its updating fails as it
>>> will not be used.
>>> 2: return -EAGAIN when all pranges at update_list need redo valid/map,
>>> otherwise return no -EAGAIN error to user space to indicate failure. That
>>> removes unnecessary retries.
>>>
>>> Signed-off-by: Xiaogang Chen <xiaogang.chen@xxxxxxx>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 27 +++++++++++++++++++++++----
>>> 1 file changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index e32e19196f6b..455cb98bf16a 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -3716,8 +3716,19 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>>>
>>> out_unlock_range:
>>> mutex_unlock(&prange->migrate_mutex);
>>> - if (r)
>>> - ret = r;
>>> + /* this prange cannot be migraed, valid or map */
>>> + if (r) {
>>> + /* free this prange resources, remove it from svms */
>>> + svm_range_unlink(prange);
>>> + svm_range_remove_notifier(prange);
>>> + svm_range_free(prange, false);
>> Freeing the prange removes SVM mappings from the process. This will break the subsequent execution of the application. In case you were going to return -EAGAIN that's definitely wrong because the application would expect the SVM range to work after a successful retry.
> When return -EAGAIN app will do whole range registration again including rebuild sub ranges. And at this stage we do not know if subsequent sub ranges will be success or fail. So I release current sub range resource if it got error(including -EAGAIN). After processing all sub ranges if decide to have app do it again, the redo procedure will rebuild the released sub ranges.
>> I'm not sure the resource waste is a valid argument in case of a fatal error. I would expect the application to terminate anyways in this case, which would result in freeing the resources. Do you see a scenario where an application wants to continue running after this function returned a fatal error?
> I made a test app to check the behavior of registration of large range for debugging a real issue. I do not know if real app will continue to run when hit no -EAGAIN error code. The purpose here is making driver handle this case more general.
>>> +
>>> + /* ret got update when any r != -EAGAIN;
>>> + * return -EAGAIN when all pranges at update_list
>>> + * need redo valid/map */
>>> + if (r != -EAGAIN || !ret)
>>> + ret = r;
>> This is a good point. But the explanation is a bit misleading: "all pranges ... need redo" is not really true. There may also be ranges that were validated successfully. I think the point you're trying to make is this: Don't return -EAGAIN if there was any previous fatal error found.
> ok
>> I could potentially see a different optimization. If you encounter a fatal error, you can skip the rest of the ranges and return the error immediately.
>>
> As said above it is a another way to return immediately if hit no -EAGAIN. but should kfd driver release unused pragne resources any way?
>
No. Freeing the prange doesn't free any big resources, like VRAM. If VRAM is used by the range, the page mappings in the CPU virtual address space hold reference counts on the underlying BO. And that doesn't go away until the address range is munmapped. If anything, you may end up using more VRAM resources because the next time you validate, you may create a new VRAM BO, but the old one may not be released yet. You may also create problems for later callbacks (MMU notifiers and migrate-to-RAM) for those virtual addresses because you're destroying the SVM range structures that would be needed by those callbacks.
Regards,
Felix
> Regards
>
> Xiaogang
>
>>> + }
>>> }
>>>
>>> list_for_each_entry(prange, &remap_list, update_list) {
>>> @@ -3729,8 +3740,16 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>>> if (r)
>>> pr_debug("failed %d on remap svm range\n", r);
>>> mutex_unlock(&prange->migrate_mutex);
>>> - if (r)
>>> - ret = r;
>>> +
>>> + if (r) {
>>> + /* remove this prange */
>>> + svm_range_unlink(prange);
>>> + svm_range_remove_notifier(prange);
>>> + svm_range_free(prange, false);
>> Same as above.
>>
>> Regards,
>> Felix
>>
>>
>>> +
>>> + if (r != -EAGAIN || !ret)
>>> + ret = r;
>>> + }
>>> }
>>>
>>> dynamic_svm_range_dump(svms);
