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

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

[Xiaogang.Chen]

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:

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);
+
+			/* 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;
+		}
 	}
 
 	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);
+
+			if (r != -EAGAIN || !ret)
+				ret = r;
+		}
 	}
 
 	dynamic_svm_range_dump(svms);
-- 
2.25.1

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

[Chen, Xiaogang]

[AMD Official Use Only - AMD Internal Distribution Only]

Ping

-----Original Message-----
From: Xiaogang.Chen <xiaogang.chen@amd.com>
Sent: Friday, January 31, 2025 10:59 AM
To: amd-gfx@lists.freedesktop.org
Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>
Subject: [PATCH] drm/amdkfd: Change error handling at prange update in svm_range_set_attr

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:

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);
+
+                       /* 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;
+               }
        }

        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);
+
+                       if (r != -EAGAIN || !ret)
+                               ret = r;
+               }
        }

        dynamic_svm_range_dump(svms);
--
2.25.1

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

[Felix Kuehling]

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.


>
> 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.

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?


> +
> +			/* 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.

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.


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

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

[Felix Kuehling]

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.


>
> 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.

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?


> +
> +			/* 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.

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.


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

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

[Chen, Xiaogang]

[-- 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 --]

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

[Felix Kuehling]

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

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

[Chen, Xiaogang]

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


On 3/4/2025 12:32 PM, Felix Kuehling wrote:
> 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@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?
>>
> 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.

yes, there are still something left that need svm data structures to 
back up though sub range got valid/map fail. And vram bo may be created 
again at redo procedure if call svm_range_free. I think mmu notifier is 
not an issue since also call svm_range_remove_notifier(prange) that 
removes mmu callback for this range.

Seems we have to carry on these unused prange data structures until app 
release them.

Thanks

Xiaogang

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

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