* [Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process
@ 2021-12-10 21:48 Rajneesh Bhardwaj
2021-12-11 0:55 ` Felix Kuehling
0 siblings, 1 reply; 3+ messages in thread
From: Rajneesh Bhardwaj @ 2021-12-10 21:48 UTC (permalink / raw)
To: amd-gfx, dri-devel
Cc: daniel.vetter, Felix Kuehling, Rajneesh Bhardwaj, David Yat Sin,
alexander.deucher, airlied, christian.koenig
When an application having open file access to a node forks, its shared
mappings also get reflected in the address space of child process even
though it cannot access them with the object permissions applied. With the
existing permission checks on the gem objects, it might be reasonable to
also create the VMAs with VM_DONTCOPY flag so a user space application
doesn't need to explicitly call the madvise(addr, len, MADV_DONTFORK)
system call to prevent the pages in the mapped range to appear in the
address space of the child process. It also prevents the memory leaks
due to additional reference counts on the mapped BOs in the child
process that prevented freeing the memory in the parent for which we had
worked around earlier in the user space inside the thunk library.
Additionally, we faced this issue when using CRIU to checkpoint restore
an application that had such inherited mappings in the child which
confuse CRIU when it mmaps on restore. Having this flag set for the
render node VMAs helps. VMAs mapped via KFD already take care of this so
this is needed only for the render nodes.
To limit the impact of the change to user space consumers such as OpenGL
etc, limit it to KFD BOs only.
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: David Yat Sin <david.yatsin@amd.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
---
Changes in v2:
* Addressed Christian's concerns for user space impact
* Further reduced the scope to KFD BOs only
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a224b5295edd..64a7931eda8c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -263,6 +263,9 @@ static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_str
!(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
vma->vm_flags &= ~VM_MAYWRITE;
+ if (bo->kfd_bo)
+ vma->vm_flags |= VM_DONTCOPY;
+
return drm_gem_ttm_mmap(obj, vma);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process
2021-12-10 21:48 [Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process Rajneesh Bhardwaj
@ 2021-12-11 0:55 ` Felix Kuehling
2021-12-14 7:17 ` Christian König
0 siblings, 1 reply; 3+ messages in thread
From: Felix Kuehling @ 2021-12-11 0:55 UTC (permalink / raw)
To: Rajneesh Bhardwaj, amd-gfx, dri-devel
Cc: alexander.deucher, airlied, David Yat Sin, christian.koenig,
daniel.vetter
On 2021-12-10 4:48 p.m., Rajneesh Bhardwaj wrote:
> When an application having open file access to a node forks, its shared
> mappings also get reflected in the address space of child process even
> though it cannot access them with the object permissions applied. With the
> existing permission checks on the gem objects, it might be reasonable to
> also create the VMAs with VM_DONTCOPY flag so a user space application
> doesn't need to explicitly call the madvise(addr, len, MADV_DONTFORK)
> system call to prevent the pages in the mapped range to appear in the
> address space of the child process. It also prevents the memory leaks
> due to additional reference counts on the mapped BOs in the child
> process that prevented freeing the memory in the parent for which we had
> worked around earlier in the user space inside the thunk library.
>
> Additionally, we faced this issue when using CRIU to checkpoint restore
> an application that had such inherited mappings in the child which
> confuse CRIU when it mmaps on restore. Having this flag set for the
> render node VMAs helps. VMAs mapped via KFD already take care of this so
> this is needed only for the render nodes.
>
> To limit the impact of the change to user space consumers such as OpenGL
> etc, limit it to KFD BOs only.
>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>
> Signed-off-by: David Yat Sin <david.yatsin@amd.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>
> Changes in v2:
> * Addressed Christian's concerns for user space impact
> * Further reduced the scope to KFD BOs only
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index a224b5295edd..64a7931eda8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -263,6 +263,9 @@ static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_str
> !(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
> vma->vm_flags &= ~VM_MAYWRITE;
>
> + if (bo->kfd_bo)
> + vma->vm_flags |= VM_DONTCOPY;
> +
> return drm_gem_ttm_mmap(obj, vma);
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process
2021-12-11 0:55 ` Felix Kuehling
@ 2021-12-14 7:17 ` Christian König
0 siblings, 0 replies; 3+ messages in thread
From: Christian König @ 2021-12-14 7:17 UTC (permalink / raw)
To: Felix Kuehling, Rajneesh Bhardwaj, amd-gfx, dri-devel
Cc: alexander.deucher, airlied, David Yat Sin, christian.koenig,
daniel.vetter
Am 11.12.21 um 01:55 schrieb Felix Kuehling:
> On 2021-12-10 4:48 p.m., Rajneesh Bhardwaj wrote:
>> When an application having open file access to a node forks, its shared
>> mappings also get reflected in the address space of child process even
>> though it cannot access them with the object permissions applied.
>> With the
>> existing permission checks on the gem objects, it might be reasonable to
>> also create the VMAs with VM_DONTCOPY flag so a user space application
>> doesn't need to explicitly call the madvise(addr, len, MADV_DONTFORK)
>> system call to prevent the pages in the mapped range to appear in the
>> address space of the child process. It also prevents the memory leaks
>> due to additional reference counts on the mapped BOs in the child
>> process that prevented freeing the memory in the parent for which we had
>> worked around earlier in the user space inside the thunk library.
>>
>> Additionally, we faced this issue when using CRIU to checkpoint restore
>> an application that had such inherited mappings in the child which
>> confuse CRIU when it mmaps on restore. Having this flag set for the
>> render node VMAs helps. VMAs mapped via KFD already take care of this so
>> this is needed only for the render nodes.
>>
>> To limit the impact of the change to user space consumers such as OpenGL
>> etc, limit it to KFD BOs only.
>>
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>
>> Signed-off-by: David Yat Sin <david.yatsin@amd.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>
> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
At some point we probably want to extend that to all AMDGPU BOs, but for
now the patch is Reviewed-by: Christian König <christian.koenig@amd.com>
Thanks,
Christian.
>
>
>> ---
>>
>> Changes in v2:
>> * Addressed Christian's concerns for user space impact
>> * Further reduced the scope to KFD BOs only
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index a224b5295edd..64a7931eda8c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -263,6 +263,9 @@ static int amdgpu_gem_object_mmap(struct
>> drm_gem_object *obj, struct vm_area_str
>> !(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
>> vma->vm_flags &= ~VM_MAYWRITE;
>> + if (bo->kfd_bo)
>> + vma->vm_flags |= VM_DONTCOPY;
>> +
>> return drm_gem_ttm_mmap(obj, vma);
>> }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-14 7:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-10 21:48 [Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process Rajneesh Bhardwaj
2021-12-11 0:55 ` Felix Kuehling
2021-12-14 7:17 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.