Re: [PATCH] drm/amdkfd: Change error handling at prange update in svm_range_set_attr

["Chen, Xiaogang" <xiaogang.chen@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 5008 bytes --]


On 3/3/2025 11:21 PM, Felix Kuehling wrote:
> On 2025-01-31 11:58, Xiaogang.Chen wrote:
>> From: Xiaogang Chen<xiaogang.chen@amd.com>
>>
>> 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@amd.com>
>> ---
>>   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?

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);

[-- Attachment #2: Type: text/html, Size: 7475 bytes --]