[PATCH 1/2] drm/amdgpu: increase hmm range get pages timeout

[PATCH 1/2] drm/amdgpu: increase hmm range get pages timeout

[James Zhu]

When application tries to allocate all system memory and cause memory
to swap out. Needs more time for hmm_range_fault to validate the
remaining page for allocation. To be safe, increase timeout value to
1 second for 64MB range.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
index 081267161d40..b24eb5821fd1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
@@ -190,8 +190,8 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
 		pr_debug("hmm range: start = 0x%lx, end = 0x%lx",
 			hmm_range->start, hmm_range->end);
 
-		/* Assuming 128MB takes maximum 1 second to fault page address */
-		timeout = max((hmm_range->end - hmm_range->start) >> 27, 1UL);
+		/* Assuming 64MB takes maximum 1 second to fault page address */
+		timeout = max((hmm_range->end - hmm_range->start) >> 26, 1UL);
 		timeout *= HMM_RANGE_DEFAULT_TIMEOUT;
 		timeout = jiffies + msecs_to_jiffies(timeout);
 
-- 
2.25.1

[PATCH 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[James Zhu]

Needn't do schedule for each hmm_range_fault, and use cond_resched
to replace schedule.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
index b24eb5821fd1..c77c4eceea46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
@@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
 		hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
 		r = hmm_range_fault(hmm_range);
 		if (unlikely(r)) {
+			cond_resched();
 			/*
 			 * FIXME: This timeout should encompass the retry from
 			 * mmu_interval_read_retry() as well.
@@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
 			break;
 		hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
 		hmm_range->start = hmm_range->end;
-		schedule();
 	} while (hmm_range->end < end);
 
 	hmm_range->start = start;
-- 
2.25.1

Re: [PATCH 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[Christian König]

Am 09.12.23 um 00:01 schrieb James Zhu:
> Needn't do schedule for each hmm_range_fault, and use cond_resched
> to replace schedule.

cond_resched() is usually NAKed upstream since it is a NO-OP in most 
situations.

IIRC there was even a patch set to completely remove it.

Christian.

>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> index b24eb5821fd1..c77c4eceea46 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> @@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
>   		hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
>   		r = hmm_range_fault(hmm_range);
>   		if (unlikely(r)) {
> +			cond_resched();
>   			/*
>   			 * FIXME: This timeout should encompass the retry from
>   			 * mmu_interval_read_retry() as well.
> @@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
>   			break;
>   		hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
>   		hmm_range->start = hmm_range->end;
> -		schedule();
>   	} while (hmm_range->end < end);
>   
>   	hmm_range->start = start;

Re: [PATCH 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[James Zhu]

On 2023-12-11 05:38, Christian König wrote:
> Am 09.12.23 um 00:01 schrieb James Zhu:
>> Needn't do schedule for each hmm_range_fault, and use cond_resched
>> to replace schedule.
>
> cond_resched() is usually NAKed upstream since it is a NO-OP in most 
> situations.
[JZ] then let me change back to schedule(); Thanks!
>
> IIRC there was even a patch set to completely remove it.
>
> Christian.
>
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> index b24eb5821fd1..c77c4eceea46 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> @@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct 
>> mmu_interval_notifier *notifier,
>>           hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
>>           r = hmm_range_fault(hmm_range);
>>           if (unlikely(r)) {
>> +            cond_resched();
>>               /*
>>                * FIXME: This timeout should encompass the retry from
>>                * mmu_interval_read_retry() as well.
>> @@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct 
>> mmu_interval_notifier *notifier,
>>               break;
>>           hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
>>           hmm_range->start = hmm_range->end;
>> -        schedule();
>>       } while (hmm_range->end < end);
>>         hmm_range->start = start;
>

[PATCH v2 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[James Zhu]

Only schedule when hmm_range_fault returns error.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
index b24eb5821fd1..55b65fc04b65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
@@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
 		hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
 		r = hmm_range_fault(hmm_range);
 		if (unlikely(r)) {
+			schedule();
 			/*
 			 * FIXME: This timeout should encompass the retry from
 			 * mmu_interval_read_retry() as well.
@@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
 			break;
 		hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
 		hmm_range->start = hmm_range->end;
-		schedule();
 	} while (hmm_range->end < end);
 
 	hmm_range->start = start;
-- 
2.25.1

Re: [PATCH 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[Felix Kuehling]

On 2023-12-11 05:38, Christian König wrote:
> Am 09.12.23 um 00:01 schrieb James Zhu:
>> Needn't do schedule for each hmm_range_fault, and use cond_resched
>> to replace schedule.
>
> cond_resched() is usually NAKed upstream since it is a NO-OP in most 
> situations.

That's weird, because https://docs.kernel.org/RCU/stallwarn.html 
specifically recommends it to resolve RCU stall warnings. I previously 
told James to use that instead of schedule().

Regards,
   Felix


>
> IIRC there was even a patch set to completely remove it.
>
> Christian.
>
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> index b24eb5821fd1..c77c4eceea46 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> @@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct 
>> mmu_interval_notifier *notifier,
>>           hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
>>           r = hmm_range_fault(hmm_range);
>>           if (unlikely(r)) {
>> +            cond_resched();
>>               /*
>>                * FIXME: This timeout should encompass the retry from
>>                * mmu_interval_read_retry() as well.
>> @@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct 
>> mmu_interval_notifier *notifier,
>>               break;
>>           hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
>>           hmm_range->start = hmm_range->end;
>> -        schedule();
>>       } while (hmm_range->end < end);
>>         hmm_range->start = start;
>

Re: [PATCH 2/2] drm/amdgpu: make an improvement on amdgpu_hmm_range_get_pages

[Christian König]

Am 12.12.23 um 00:43 schrieb Felix Kuehling:
>
> On 2023-12-11 05:38, Christian König wrote:
>> Am 09.12.23 um 00:01 schrieb James Zhu:
>>> Needn't do schedule for each hmm_range_fault, and use cond_resched
>>> to replace schedule.
>>
>> cond_resched() is usually NAKed upstream since it is a NO-OP in most 
>> situations.
>
> That's weird, because https://docs.kernel.org/RCU/stallwarn.html 
> specifically recommends it to resolve RCU stall warnings. I previously 
> told James to use that instead of schedule().

I haven't followed the full discussion and rational about that either, 
could be that the documentation is outdated.

The latest RFC to remove cond_resched() and explaining the background 
can be found here: https://lwn.net/Articles/950581/

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> IIRC there was even a patch set to completely remove it.
>>
>> Christian.
>>
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> index b24eb5821fd1..c77c4eceea46 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> @@ -199,6 +199,7 @@ int amdgpu_hmm_range_get_pages(struct 
>>> mmu_interval_notifier *notifier,
>>>           hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
>>>           r = hmm_range_fault(hmm_range);
>>>           if (unlikely(r)) {
>>> +            cond_resched();
>>>               /*
>>>                * FIXME: This timeout should encompass the retry from
>>>                * mmu_interval_read_retry() as well.
>>> @@ -212,7 +213,6 @@ int amdgpu_hmm_range_get_pages(struct 
>>> mmu_interval_notifier *notifier,
>>>               break;
>>>           hmm_range->hmm_pfns += MAX_WALK_BYTE >> PAGE_SHIFT;
>>>           hmm_range->start = hmm_range->end;
>>> -        schedule();
>>>       } while (hmm_range->end < end);
>>>         hmm_range->start = start;
>>

Re: [PATCH 1/2] drm/amdgpu: increase hmm range get pages timeout

[James Zhu]

Ping ...

On 2023-12-08 18:01, James Zhu wrote:
> When application tries to allocate all system memory and cause memory
> to swap out. Needs more time for hmm_range_fault to validate the
> remaining page for allocation. To be safe, increase timeout value to
> 1 second for 64MB range.
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> index 081267161d40..b24eb5821fd1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
> @@ -190,8 +190,8 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
>   		pr_debug("hmm range: start = 0x%lx, end = 0x%lx",
>   			hmm_range->start, hmm_range->end);
>   
> -		/* Assuming 128MB takes maximum 1 second to fault page address */
> -		timeout = max((hmm_range->end - hmm_range->start) >> 27, 1UL);
> +		/* Assuming 64MB takes maximum 1 second to fault page address */
> +		timeout = max((hmm_range->end - hmm_range->start) >> 26, 1UL);
>   		timeout *= HMM_RANGE_DEFAULT_TIMEOUT;
>   		timeout = jiffies + msecs_to_jiffies(timeout);
>   

Re: [PATCH 1/2] drm/amdgpu: increase hmm range get pages timeout

[Felix Kuehling]

On 2023-12-13 10:24, James Zhu wrote:
> Ping ...
>
> On 2023-12-08 18:01, James Zhu wrote:
>> When application tries to allocate all system memory and cause memory
>> to swap out. Needs more time for hmm_range_fault to validate the
>> remaining page for allocation. To be safe, increase timeout value to
>> 1 second for 64MB range.
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>

This is not the first time we're incrementing this timeout. Eventually 
we should get rid of that and find a way to make this work reliably 
without a timeout. There can always be situations where faults take 
longer, and we should not fail randomly in those cases.

There are also some FIXMEs in this code that should be addressed at the 
same time.

That said, as a short-term fix, this patch is

Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>


>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> index 081267161d40..b24eb5821fd1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>> @@ -190,8 +190,8 @@ int amdgpu_hmm_range_get_pages(struct 
>> mmu_interval_notifier *notifier,
>>           pr_debug("hmm range: start = 0x%lx, end = 0x%lx",
>>               hmm_range->start, hmm_range->end);
>>   -        /* Assuming 128MB takes maximum 1 second to fault page 
>> address */
>> -        timeout = max((hmm_range->end - hmm_range->start) >> 27, 1UL);
>> +        /* Assuming 64MB takes maximum 1 second to fault page 
>> address */
>> +        timeout = max((hmm_range->end - hmm_range->start) >> 26, 1UL);
>>           timeout *= HMM_RANGE_DEFAULT_TIMEOUT;
>>           timeout = jiffies + msecs_to_jiffies(timeout);

Re: [PATCH 1/2] drm/amdgpu: increase hmm range get pages timeout

[James Zhu]

On 2023-12-13 11:23, Felix Kuehling wrote:
>
> On 2023-12-13 10:24, James Zhu wrote:
>> Ping ...
>>
>> On 2023-12-08 18:01, James Zhu wrote:
>>> When application tries to allocate all system memory and cause memory
>>> to swap out. Needs more time for hmm_range_fault to validate the
>>> remaining page for allocation. To be safe, increase timeout value to
>>> 1 second for 64MB range.
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>
> This is not the first time we're incrementing this timeout. Eventually 
> we should get rid of that and find a way to make this work reliably 
> without a timeout. There can always be situations where faults take 
> longer, and we should not fail randomly in those cases.
>
> There are also some FIXMEs in this code that should be addressed at 
> the same time.
>
> That said, as a short-term fix, this patch is
[JZ] Yes, it is just a short-term fix. the root cause is still under study,
>
> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> index 081267161d40..b24eb5821fd1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>> @@ -190,8 +190,8 @@ int amdgpu_hmm_range_get_pages(struct 
>>> mmu_interval_notifier *notifier,
>>>           pr_debug("hmm range: start = 0x%lx, end = 0x%lx",
>>>               hmm_range->start, hmm_range->end);
>>>   -        /* Assuming 128MB takes maximum 1 second to fault page 
>>> address */
>>> -        timeout = max((hmm_range->end - hmm_range->start) >> 27, 1UL);
>>> +        /* Assuming 64MB takes maximum 1 second to fault page 
>>> address */
>>> +        timeout = max((hmm_range->end - hmm_range->start) >> 26, 1UL);
>>>           timeout *= HMM_RANGE_DEFAULT_TIMEOUT;
>>>           timeout = jiffies + msecs_to_jiffies(timeout);