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

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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