[PATCH] drm/amdgpu: fix huge page handling on Vega10

[PATCH] drm/amdgpu: fix huge page handling on Vega10

[Christian König]

We accidentially set the huge flag on the parent instead of the childs.
This caused some VM faults under memory pressure.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 352b30409060..dad0e2342df9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 			continue;
 		}
 
-		/* First check if the entry is already handled */
-		if (cursor.pfn < frag_start) {
-			cursor.entry->huge = true;
-			amdgpu_vm_pt_next(adev, &cursor);
-			continue;
-		}
-
 		/* If it isn't already handled it can't be a huge page */
 		if (cursor.entry->huge) {
 			/* Add the entry to the relocated list to update it. */
@@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 			}
 		} while (frag_start < entry_end);
 
-		if (frag >= shift)
+		if (amdgpu_vm_pt_descendant(adev, &cursor)) {
+			/* Mark all child entries as huge */
+			while (cursor.pfn < frag_start) {
+				cursor.entry->huge = true;
+				amdgpu_vm_pt_next(adev, &cursor);
+			}
+
+		} else if (frag >= shift) {
+			/* or just move on to the next on the same level. */
 			amdgpu_vm_pt_next(adev, &cursor);
+		}
 	}
 
 	return 0;
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Alex Deucher]

On Mon, Nov 12, 2018 at 12:09 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> We accidentially set the huge flag on the parent instead of the childs.
> This caused some VM faults under memory pressure.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 352b30409060..dad0e2342df9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>                         continue;
>                 }
>
> -               /* First check if the entry is already handled */
> -               if (cursor.pfn < frag_start) {
> -                       cursor.entry->huge = true;
> -                       amdgpu_vm_pt_next(adev, &cursor);
> -                       continue;
> -               }
> -
>                 /* If it isn't already handled it can't be a huge page */
>                 if (cursor.entry->huge) {
>                         /* Add the entry to the relocated list to update it. */
> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>                         }
>                 } while (frag_start < entry_end);
>
> -               if (frag >= shift)
> +               if (amdgpu_vm_pt_descendant(adev, &cursor)) {
> +                       /* Mark all child entries as huge */
> +                       while (cursor.pfn < frag_start) {
> +                               cursor.entry->huge = true;
> +                               amdgpu_vm_pt_next(adev, &cursor);
> +                       }
> +
> +               } else if (frag >= shift) {
> +                       /* or just move on to the next on the same level. */
>                         amdgpu_vm_pt_next(adev, &cursor);
> +               }
>         }
>
>         return 0;
> --
> 2.14.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Samuel Pitoiset]

On 11/12/18 6:16 PM, Alex Deucher wrote:
> On Mon, Nov 12, 2018 at 12:09 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>> We accidentially set the huge flag on the parent instead of the childs.
>> This caused some VM faults under memory pressure.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>

Yes, this fixes GPU hangs with F12017 and RoTR with RADV.

Thanks for the fix Christian!

> 
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 352b30409060..dad0e2342df9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>                          continue;
>>                  }
>>
>> -               /* First check if the entry is already handled */
>> -               if (cursor.pfn < frag_start) {
>> -                       cursor.entry->huge = true;
>> -                       amdgpu_vm_pt_next(adev, &cursor);
>> -                       continue;
>> -               }
>> -
>>                  /* If it isn't already handled it can't be a huge page */
>>                  if (cursor.entry->huge) {
>>                          /* Add the entry to the relocated list to update it. */
>> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>                          }
>>                  } while (frag_start < entry_end);
>>
>> -               if (frag >= shift)
>> +               if (amdgpu_vm_pt_descendant(adev, &cursor)) {
>> +                       /* Mark all child entries as huge */
>> +                       while (cursor.pfn < frag_start) {
>> +                               cursor.entry->huge = true;
>> +                               amdgpu_vm_pt_next(adev, &cursor);
>> +                       }
>> +
>> +               } else if (frag >= shift) {
>> +                       /* or just move on to the next on the same level. */
>>                          amdgpu_vm_pt_next(adev, &cursor);
>> +               }
>>          }
>>
>>          return 0;
>> --
>> 2.14.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Christian König]

Am 12.11.18 um 22:45 schrieb Samuel Pitoiset:
>
>
> On 11/12/18 6:16 PM, Alex Deucher wrote:
>> On Mon, Nov 12, 2018 at 12:09 PM Christian König
>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>
>>> We accidentially set the huge flag on the parent instead of the childs.
>>> This caused some VM faults under memory pressure.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>
> Yes, this fixes GPU hangs with F12017 and RoTR with RADV.
>
> Thanks for the fix Christian!

Just FYI I take sentences as this as a "Tested-by: Samuel Pitoiset 
<samuel.pitoiset@gmail.com>".

So if you don't want your mail address to show up in a kernel commit 
start better say so before Alex pushes it upstream :)

Christian.

>
>>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 352b30409060..dad0e2342df9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct 
>>> amdgpu_pte_update_params *params,
>>>                          continue;
>>>                  }
>>>
>>> -               /* First check if the entry is already handled */
>>> -               if (cursor.pfn < frag_start) {
>>> -                       cursor.entry->huge = true;
>>> -                       amdgpu_vm_pt_next(adev, &cursor);
>>> -                       continue;
>>> -               }
>>> -
>>>                  /* If it isn't already handled it can't be a huge 
>>> page */
>>>                  if (cursor.entry->huge) {
>>>                          /* Add the entry to the relocated list to 
>>> update it. */
>>> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct 
>>> amdgpu_pte_update_params *params,
>>>                          }
>>>                  } while (frag_start < entry_end);
>>>
>>> -               if (frag >= shift)
>>> +               if (amdgpu_vm_pt_descendant(adev, &cursor)) {
>>> +                       /* Mark all child entries as huge */
>>> +                       while (cursor.pfn < frag_start) {
>>> +                               cursor.entry->huge = true;
>>> +                               amdgpu_vm_pt_next(adev, &cursor);
>>> +                       }
>>> +
>>> +               } else if (frag >= shift) {
>>> +                       /* or just move on to the next on the same 
>>> level. */
>>>                          amdgpu_vm_pt_next(adev, &cursor);
>>> +               }
>>>          }
>>>
>>>          return 0;
>>> -- 
>>> 2.14.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Samuel Pitoiset]

On 11/13/18 9:14 AM, Christian König wrote:
> Am 12.11.18 um 22:45 schrieb Samuel Pitoiset:
>>
>>
>> On 11/12/18 6:16 PM, Alex Deucher wrote:
>>> On Mon, Nov 12, 2018 at 12:09 PM Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>
>>>> We accidentially set the huge flag on the parent instead of the childs.
>>>> This caused some VM faults under memory pressure.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>
>> Yes, this fixes GPU hangs with F12017 and RoTR with RADV.
>>
>> Thanks for the fix Christian!
> 
> Just FYI I take sentences as this as a "Tested-by: Samuel Pitoiset 
> <samuel.pitoiset@gmail.com>".
> 
> So if you don't want your mail address to show up in a kernel commit 
> start better say so before Alex pushes it upstream :)

Feel free to add that tag :)

> 
> Christian.
> 
>>
>>>
>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 352b30409060..dad0e2342df9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct 
>>>> amdgpu_pte_update_params *params,
>>>>                          continue;
>>>>                  }
>>>>
>>>> -               /* First check if the entry is already handled */
>>>> -               if (cursor.pfn < frag_start) {
>>>> -                       cursor.entry->huge = true;
>>>> -                       amdgpu_vm_pt_next(adev, &cursor);
>>>> -                       continue;
>>>> -               }
>>>> -
>>>>                  /* If it isn't already handled it can't be a huge 
>>>> page */
>>>>                  if (cursor.entry->huge) {
>>>>                          /* Add the entry to the relocated list to 
>>>> update it. */
>>>> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct 
>>>> amdgpu_pte_update_params *params,
>>>>                          }
>>>>                  } while (frag_start < entry_end);
>>>>
>>>> -               if (frag >= shift)
>>>> +               if (amdgpu_vm_pt_descendant(adev, &cursor)) {
>>>> +                       /* Mark all child entries as huge */
>>>> +                       while (cursor.pfn < frag_start) {
>>>> +                               cursor.entry->huge = true;
>>>> +                               amdgpu_vm_pt_next(adev, &cursor);
>>>> +                       }
>>>> +
>>>> +               } else if (frag >= shift) {
>>>> +                       /* or just move on to the next on the same 
>>>> level. */
>>>>                          amdgpu_vm_pt_next(adev, &cursor);
>>>> +               }
>>>>          }
>>>>
>>>>          return 0;
>>>> -- 
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Kuehling, Felix]

On 2018-11-12 12:09 p.m., Christian König wrote:
> We accidentially set the huge flag on the parent instead of the childs.
> This caused some VM faults under memory pressure.

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

I got a bit confused when re-reading this code. Maybe part of it is that
cursor.entry is not really an entry (as in PTE), but a page table.
cursor.pfn identifies the entry within the table. Maybe renaming
cursor.entry to something like cursor.table would make it slightly more
intuitive.

Regards,
  Felix


>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 352b30409060..dad0e2342df9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>  			continue;
>  		}
>  
> -		/* First check if the entry is already handled */
> -		if (cursor.pfn < frag_start) {
> -			cursor.entry->huge = true;
> -			amdgpu_vm_pt_next(adev, &cursor);
> -			continue;
> -		}
> -
>  		/* If it isn't already handled it can't be a huge page */
>  		if (cursor.entry->huge) {
>  			/* Add the entry to the relocated list to update it. */
> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>  			}
>  		} while (frag_start < entry_end);
>  
> -		if (frag >= shift)
> +		if (amdgpu_vm_pt_descendant(adev, &cursor)) {
> +			/* Mark all child entries as huge */
> +			while (cursor.pfn < frag_start) {
> +				cursor.entry->huge = true;
> +				amdgpu_vm_pt_next(adev, &cursor);
> +			}
> +
> +		} else if (frag >= shift) {
> +			/* or just move on to the next on the same level. */
>  			amdgpu_vm_pt_next(adev, &cursor);
> +		}
>  	}
>  
>  	return 0;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix huge page handling on Vega10

[Christian König]

Am 13.11.18 um 00:50 schrieb Kuehling, Felix:
> On 2018-11-12 12:09 p.m., Christian König wrote:
>> We accidentially set the huge flag on the parent instead of the childs.
>> This caused some VM faults under memory pressure.
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
> I got a bit confused when re-reading this code. Maybe part of it is that
> cursor.entry is not really an entry (as in PTE), but a page table.
> cursor.pfn identifies the entry within the table. Maybe renaming
> cursor.entry to something like cursor.table would make it slightly more
> intuitive.

Good point and suggestion how to improve the naming. Going to make that 
change when I improve things further towards recoverable page faults.

Christian.

>
> Regards,
>    Felix
>
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 352b30409060..dad0e2342df9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1632,13 +1632,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>   			continue;
>>   		}
>>   
>> -		/* First check if the entry is already handled */
>> -		if (cursor.pfn < frag_start) {
>> -			cursor.entry->huge = true;
>> -			amdgpu_vm_pt_next(adev, &cursor);
>> -			continue;
>> -		}
>> -
>>   		/* If it isn't already handled it can't be a huge page */
>>   		if (cursor.entry->huge) {
>>   			/* Add the entry to the relocated list to update it. */
>> @@ -1701,8 +1694,17 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>   			}
>>   		} while (frag_start < entry_end);
>>   
>> -		if (frag >= shift)
>> +		if (amdgpu_vm_pt_descendant(adev, &cursor)) {
>> +			/* Mark all child entries as huge */
>> +			while (cursor.pfn < frag_start) {
>> +				cursor.entry->huge = true;
>> +				amdgpu_vm_pt_next(adev, &cursor);
>> +			}
>> +
>> +		} else if (frag >= shift) {
>> +			/* or just move on to the next on the same level. */
>>   			amdgpu_vm_pt_next(adev, &cursor);
>> +		}
>>   	}
>>   
>>   	return 0;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx