[PATCH] drm/amdgpu: fix potential VM faults

[PATCH] drm/amdgpu: fix potential VM faults

[Christian König]

When we allocate new page tables under memory
pressure we should not evict old ones.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 70d45d48907a..8e44ecaada35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 		.interruptible = (bp->type != ttm_bo_type_kernel),
 		.no_wait_gpu = bp->no_wait_gpu,
 		.resv = bp->resv,
-		.flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
+		.flags = bp->type != ttm_bo_type_kernel ?
+			TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
 	};
 	struct amdgpu_bo *bo;
 	unsigned long page_align, size = bp->size;
-- 
2.17.1

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

Re: [PATCH] drm/amdgpu: fix potential VM faults

[Deucher, Alexander]

[-- Attachment #1.1: Type: text/plain, Size: 1733 bytes --]

Acked-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Christian König <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Sent: Thursday, September 19, 2019 4:41 AM
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: [PATCH] drm/amdgpu: fix potential VM faults

When we allocate new page tables under memory
pressure we should not evict old ones.

Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 70d45d48907a..8e44ecaada35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
                 .interruptible = (bp->type != ttm_bo_type_kernel),
                 .no_wait_gpu = bp->no_wait_gpu,
                 .resv = bp->resv,
-               .flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
+               .flags = bp->type != ttm_bo_type_kernel ?
+                       TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
         };
         struct amdgpu_bo *bo;
         unsigned long page_align, size = bp->size;
--
2.17.1

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

[-- Attachment #1.2: Type: text/html, Size: 3316 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [PATCH] drm/amdgpu: fix potential VM faults

[Kuehling, Felix]

I'm not disagreeing with the change. Just trying to understand how this 
could have caused a VM fault. If the page tables are reserved or fenced 
while you allocate a new one, they would not be evicted. If they are not 
reserved or fenced, there should be no expectation that they stay resident.

Is this related to recoverable page fault handling? Do we need some more 
generic way to handle eviction of page tables and update the parent page 
directory (invalidate the corresponding PDE)?

Regards,
   Felix

On 2019-09-19 4:41, Christian König wrote:
> When we allocate new page tables under memory
> pressure we should not evict old ones.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 70d45d48907a..8e44ecaada35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   		.interruptible = (bp->type != ttm_bo_type_kernel),
>   		.no_wait_gpu = bp->no_wait_gpu,
>   		.resv = bp->resv,
> -		.flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
> +		.flags = bp->type != ttm_bo_type_kernel ?
> +			TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
>   	};
>   	struct amdgpu_bo *bo;
>   	unsigned long page_align, size = bp->size;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix potential VM faults

[Christian König]

> If the page tables are reserved or fenced while you allocate a new one, they would not be evicted.
And exactly that's not correct. The TTM_OPT_FLAG_ALLOW_RES_EVICT flag 
allows evicting of reserved objects.

This is useful for allocating per VM BOs, but is of course completely 
fatal in all other cases.

Regards,
Christian.

Am 19.09.19 um 16:29 schrieb Kuehling, Felix:
> I'm not disagreeing with the change. Just trying to understand how this
> could have caused a VM fault. If the page tables are reserved or fenced
> while you allocate a new one, they would not be evicted. If they are not
> reserved or fenced, there should be no expectation that they stay resident.
>
> Is this related to recoverable page fault handling? Do we need some more
> generic way to handle eviction of page tables and update the parent page
> directory (invalidate the corresponding PDE)?
>
> Regards,
>     Felix
>
> On 2019-09-19 4:41, Christian König wrote:
>> When we allocate new page tables under memory
>> pressure we should not evict old ones.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 70d45d48907a..8e44ecaada35 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>>    		.interruptible = (bp->type != ttm_bo_type_kernel),
>>    		.no_wait_gpu = bp->no_wait_gpu,
>>    		.resv = bp->resv,
>> -		.flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
>> +		.flags = bp->type != ttm_bo_type_kernel ?
>> +			TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
>>    	};
>>    	struct amdgpu_bo *bo;
>>    	unsigned long page_align, size = bp->size;

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

RE: [PATCH] drm/amdgpu: fix potential VM faults

[Liu, Monk]

Hi Christian

Theoretically the vm pt/pd should be allowed to be evicted like other BOs ..

If you encountered page fault and could be avoided by this patch, that means there is bug in the VM/ttm system , and your patch simply

w/a the root cause.

_____________________________________
Monk Liu|GPU Virtualization Team |AMD


-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian K?nig
Sent: Thursday, September 19, 2019 4:42 PM
To: amd-gfx@lists.freedesktop.org
Subject: [PATCH] drm/amdgpu: fix potential VM faults

When we allocate new page tables under memory pressure we should not evict old ones.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 70d45d48907a..8e44ecaada35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 		.interruptible = (bp->type != ttm_bo_type_kernel),
 		.no_wait_gpu = bp->no_wait_gpu,
 		.resv = bp->resv,
-		.flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
+		.flags = bp->type != ttm_bo_type_kernel ?
+			TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
 	};
 	struct amdgpu_bo *bo;
 	unsigned long page_align, size = bp->size;
--
2.17.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 potential VM faults

[Christian König]

Hi Monk,

this patch doesn't prevents PD/PT eviction.

The intention of the code here was that per VM BOs can evict other per 
VM BOs during allocation.

The problem my patch fixes is that this unfortunately also meant that 
allocation PDs/PTs could evict other PDs/PTs from the same process.

Regards,
Christian.

Am 25.09.19 um 15:51 schrieb Liu, Monk:
> Hi Christian
>
> Theoretically the vm pt/pd should be allowed to be evicted like other BOs ..
>
> If you encountered page fault and could be avoided by this patch, that means there is bug in the VM/ttm system , and your patch simply
>
> w/a the root cause.
>
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian K?nig
> Sent: Thursday, September 19, 2019 4:42 PM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH] drm/amdgpu: fix potential VM faults
>
> When we allocate new page tables under memory pressure we should not evict old ones.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 70d45d48907a..8e44ecaada35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -514,7 +514,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   		.interruptible = (bp->type != ttm_bo_type_kernel),
>   		.no_wait_gpu = bp->no_wait_gpu,
>   		.resv = bp->resv,
> -		.flags = TTM_OPT_FLAG_ALLOW_RES_EVICT
> +		.flags = bp->type != ttm_bo_type_kernel ?
> +			TTM_OPT_FLAG_ALLOW_RES_EVICT : 0
>   	};
>   	struct amdgpu_bo *bo;
>   	unsigned long page_align, size = bp->size;
> --
> 2.17.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