<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>Sorry for the typo in my previous email. Please read Adrian
Reber*<br>
</p>
<div class="moz-cite-prefix">On 12/22/2021 8:49 PM, Bhardwaj,
Rajneesh wrote:<br>
</div>
<blockquote type="cite" cite="mid:ebcdbdad-1d34-41be-0423-e97b994e2bb2@amd.com">
<p>Adding Adrian Rebel who is the CRIU maintainer and CRIU list<br>
</p>
<div class="moz-cite-prefix">On 12/22/2021 3:53 PM, Daniel Vetter
wrote:<br>
</div>
<blockquote type="cite" cite="mid:YcOQN%2Fl7W66W%2FX0f@phenom.ffwll.local">
<pre class="moz-quote-pre" wrap="">On Mon, Dec 20, 2021 at 01:12:51PM -0500, Bhardwaj, Rajneesh wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On 12/20/2021 4:29 AM, Daniel Vetter wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Fri, Dec 10, 2021 at 07:58:50AM +0100, Christian König wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Am 09.12.21 um 19:28 schrieb Felix Kuehling:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Am 2021-12-09 um 10:30 a.m. schrieb Christian König:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">That still won't work.
But I think we could do this change for the amdgpu mmap callback only.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">If graphics user mode has problems with it, we could even make this
specific to KFD BOs in the amdgpu_gem_object_mmap callback.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I think it's fine for the whole amdgpu stack, my concern is more about
radeon, nouveau and the ARM stacks which are using this as well.
That blew up so nicely the last time we tried to change it and I know of at
least one case where radeon was/is used with BOs in a child process.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I'm way late and burried again, but I think it'd be good to be consistent</pre>
</blockquote>
</blockquote>
</blockquote>
<p><br>
</p>
<p>I had committed this change into our amd-staging-drm-next
branch last week after I got the ACK and RB from Felix and
Christian.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:YcOQN%2Fl7W66W%2FX0f@phenom.ffwll.local">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">here across drivers. Or at least across drm drivers. And we've had the vma
open/close refcounting to make fork work since forever.
I think if we do this we should really only do this for mmap() where this
applies, but reading through the thread here I'm honestly confused why
this is a problem. If CRIU can't handle forked mmaps it needs to be
thought that, not hacked around. Or at least I'm not understanding why
this shouldn't work ...
-Daniel
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Hi Daniel
In the v2
<a class="moz-txt-link-freetext" href="https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fa1a865f5-ad2c-29c8-cbe4-2635d53eceb6%40amd.com%2FT%2F&amp;data=04%7C01%7Crajneesh.bhardwaj%40amd.com%7Ce4634a16c37149da173408d9c58d1338%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637758031981907821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=h0z4sO19bsJecMqeHGdz%2BHZElKuyzK%2BW%2FMbLWA79I10%3D&amp;reserved=0" moz-do-not-send="true">https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fa1a865f5-ad2c-29c8-cbe4-2635d53eceb6%40amd.com%2FT%2F&amp;data=04%7C01%7Crajneesh.bhardwaj%40amd.com%7Ce4634a16c37149da173408d9c58d1338%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637758031981907821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=h0z4sO19bsJecMqeHGdz%2BHZElKuyzK%2BW%2FMbLWA79I10%3D&amp;reserved=0</a>
I pretty much limited the scope of the change to KFD BOs on mmap. Regarding
CRIU, I think its not a CRIU problem as CRIU on restore, only tries to
recreate all the child processes and then mmaps all the VMAs it sees (as per
checkpoint snapshot) in the new process address space after the VMA
placements are finalized in the position independent code phase. Since the
inherited VMAs don't have access rights the criu mmap fails.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Still sounds funky. I think minimally we should have an ack from CRIU
developers that this is officially the right way to solve this problem. I
really don't want to have random one-off hacks that don't work across the
board, for a problem where we (drm subsystem) really shouldn't be the only
one with this problem. Where "this problem" means that the mmap space is
per file description, and not per underlying inode or real device or
whatever. That part sounds like a CRIU problem, and I expect CRIU folks
want a consistent solution across the board for this. Hence please grab an
ack from them.
Cheers, Daniel</pre>
</blockquote>
<p><br>
</p>
<p>Maybe Adrian can share his views on this. <br>
</p>
<p>Hi Adrian - For the context, on CRIU restore we see mmap
failures ( in PIE restore phase) due to permission issues on the
(render node) VMAs that were inherited since the application
that check pointed had forked. The VMAs ideally should not be
in the child process but the smaps file shows these VMAs in the
child address space. We didn't want to use madvise to avoid this
copy and rather change in the kernel mode to limit the impact to
our user space library thunk. Based on my understanding, during
PIE restore phase, after the VMA placements are finalized, CRIU
does a sys_mmap on all the VMA it sees in the VmaEntry list and
I think its not an issue as per CRIU design but do you think we
could handle this corner case better inside CRIU? <span style="color: rgb(201, 209, 217); font-family: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, "Liberation Mono", monospace; font-size: 12px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(13, 17, 23); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"></span></p>
<p><br>
</p>
<blockquote type="cite" cite="mid:YcOQN%2Fl7W66W%2FX0f@phenom.ffwll.local">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Regards,
Rajneesh
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Regards,
Christian.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Regards,
Felix
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Regards,
Christian.
Am 09.12.21 um 16:29 schrieb Bhardwaj, Rajneesh:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Sounds good. I will send a v2 with only ttm_bo_mmap_obj change. Thank
you!
On 12/9/2021 10:27 AM, Christian König wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi Rajneesh,
yes, separating this from the drm_gem_mmap_obj() change is certainly
a good idea.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The child cannot access the BOs mapped by the parent anyway with
access restrictions applied
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Am 08.12.21 um 21:53 schrieb Rajneesh Bhardwaj:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Cc: Felix Kuehling <a class="moz-txt-link-rfc2396E" href="mailto:Felix.Kuehling@amd.com" moz-do-not-send="true"><Felix.Kuehling@amd.com></a>
Signed-off-by: David Yat Sin <a class="moz-txt-link-rfc2396E" href="mailto:david.yatsin@amd.com" moz-do-not-send="true"><david.yatsin@amd.com></a>
Signed-off-by: Rajneesh Bhardwaj <a class="moz-txt-link-rfc2396E" href="mailto:rajneesh.bhardwaj@amd.com" moz-do-not-send="true"><rajneesh.bhardwaj@amd.com></a>
---
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;
}
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>