| Message ID | 20211210214802.12620-1-rajneesh.bhardwaj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/amdgpu: Don't inherit GEM object VMAs in child process | expand |
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); > } >
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); >> }
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); }