From: "Christian König" <christian.koenig@amd.com>
To: "Bhardwaj, Rajneesh" <rajneesh.bhardwaj@amd.com>,
"Christian König" <ckoenig.leichtzumerken@gmail.com>,
amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: alexander.deucher@amd.com, daniel.vetter@ffwll.ch,
Felix Kuehling <Felix.Kuehling@amd.com>,
David Yat Sin <david.yatsin@amd.com>,
airlied@redhat.com
Subject: Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process
Date: Thu, 9 Dec 2021 16:27:18 +0100 [thread overview]
Message-ID: <de272de9-3f4a-db40-699a-41394cb699dc@amd.com> (raw)
In-Reply-To: <58d61e47-3796-3147-db6c-ea7912d16902@amd.com>
Hi Rajneesh,
yes, separating this from the drm_gem_mmap_obj() change is certainly a
good idea.
> The child cannot access the BOs mapped by the parent anyway with
> access restrictions applied
exactly that is not correct. That behavior is actively used by some
userspace stacks as far as I know.
Regards,
Christian.
Am 09.12.21 um 16:23 schrieb Bhardwaj, Rajneesh:
> Thanks Christian. Would it make it less intrusive if I just use the
> flag for ttm bo mmap and remove the drm_gem_mmap_obj change from this
> patch? For our use case, just the ttm_bo_mmap_obj change should
> suffice and we don't want to put any more work arounds in the user
> space (thunk, in our case).
>
> The child cannot access the BOs mapped by the parent anyway with
> access restrictions applied so I wonder why even inherit the vma?
>
> On 12/9/2021 2:54 AM, Christian König wrote:
>> Am 08.12.21 um 21:53 schrieb Rajneesh Bhardwaj:
>>> 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.
>>
>> Unfortunately that is most likely a NAK. We already tried something
>> similar.
>>
>> While it is illegal by the OpenGL specification and doesn't work for
>> most userspace stacks, we do have some implementations which call
>> fork() with a GL context open and expect it to work.
>>
>> Regards,
>> Christian.
>>
>>>
>>> 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>
>>> ---
>>> drivers/gpu/drm/drm_gem.c | 3 ++-
>>> drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +-
>>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 09c820045859..d9c4149f36dd 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -1058,7 +1058,8 @@ int drm_gem_mmap_obj(struct drm_gem_object
>>> *obj, unsigned long obj_size,
>>> goto err_drm_gem_object_put;
>>> }
>>> - vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
>>> VM_DONTDUMP;
>>> + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND
>>> + | VM_DONTDUMP | VM_DONTCOPY;
>>> vma->vm_page_prot =
>>> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>>> }
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>> index 33680c94127c..420a4898fdd2 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>> @@ -566,7 +566,7 @@ int ttm_bo_mmap_obj(struct vm_area_struct *vma,
>>> struct ttm_buffer_object *bo)
>>> vma->vm_private_data = bo;
>>> - vma->vm_flags |= VM_PFNMAP;
>>> + vma->vm_flags |= VM_PFNMAP | VM_DONTCOPY;
>>> vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>> return 0;
>>> }
>>
next prev parent reply other threads:[~2021-12-09 16:58 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-08 20:53 [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process Rajneesh Bhardwaj
2021-12-09 7:54 ` Christian König
2021-12-09 15:23 ` Bhardwaj, Rajneesh
2021-12-09 15:27 ` Christian König [this message]
2021-12-09 15:29 ` Bhardwaj, Rajneesh
2021-12-09 15:30 ` Christian König
2021-12-09 18:28 ` Felix Kuehling
2021-12-10 6:58 ` Christian König
2021-12-20 9:29 ` Daniel Vetter
2021-12-20 18:12 ` Bhardwaj, Rajneesh
2021-12-22 20:53 ` Daniel Vetter
2021-12-23 1:49 ` Bhardwaj, Rajneesh
2021-12-23 1:51 ` Bhardwaj, Rajneesh
2021-12-23 7:05 ` Christian König
2022-01-04 18:08 ` Felix Kuehling
2022-01-05 8:08 ` Christian König
2022-01-05 16:16 ` Felix Kuehling
2022-01-05 16:27 ` Felix Kuehling
2022-01-06 9:05 ` Christian König
2022-01-06 16:45 ` Felix Kuehling
2022-01-06 16:48 ` Christian König
2022-01-06 16:51 ` Felix Kuehling
2022-01-07 8:56 ` Christian König
2022-01-07 17:47 ` Felix Kuehling
2022-01-14 16:44 ` Daniel Vetter
2022-01-14 17:26 ` Christian König
2022-01-14 17:40 ` Felix Kuehling
2022-01-17 11:44 ` Christian König
2022-01-17 14:17 ` Felix Kuehling
2022-01-17 14:21 ` Christian König
2022-01-17 14:34 ` Felix Kuehling
2022-01-17 14:50 ` Marek Olšák
2022-01-17 16:23 ` Christian König
2022-01-10 17:30 ` Bhardwaj, Rajneesh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=de272de9-3f4a-db40-699a-41394cb699dc@amd.com \
--to=christian.koenig@amd.com \
--cc=Felix.Kuehling@amd.com \
--cc=airlied@redhat.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=daniel.vetter@ffwll.ch \
--cc=david.yatsin@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=rajneesh.bhardwaj@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).