Am 16.11.20 um 21:42 schrieb Andrey Grodzovsky:
On 11/16/20 3:36 PM, Christian König wrote:Am 16.11.20 um 20:00 schrieb Andrey Grodzovsky:On 11/16/20 4:48 AM, Christian König wrote:Am 15.11.20 um 07:34 schrieb Andrey Grodzovsky:On 11/14/20 4:51 AM, Daniel Vetter wrote:On Sat, Nov 14, 2020 at 9:41 AM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> wrote:Well amdgpu doesn't clear buffers by default, so indeed you guys are aAm 13.11.20 um 21:52 schrieb Andrey Grodzovsky:On 6/22/20 1:50 PM, Daniel Vetter wrote:On Mon, Jun 22, 2020 at 7:45 PM Christian König <christian.koenig@xxxxxxx> wrote:Am 22.06.20 um 16:32 schrieb Andrey Grodzovsky:Yeah, but this is only for a very very small corner cases. What we need to prevent is increasing the memory usage during normal operation toOn 6/22/20 9:18 AM, Christian König wrote:Global dummy page is good for read access, what do you do on write access ? My first approach was indeed to map at first global dummy page as read only and mark the vma->vm_flags as !VM_SHARED assumingAm 21.06.20 um 08:03 schrieb Andrey Grodzovsky:While the per file case still looks acceptable this is a clear NAKWill be used to reroute CPU mapped BO's page faults once device is removed. Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx> --- drivers/gpu/drm/drm_file.c | 8 ++++++++ drivers/gpu/drm/drm_prime.c | 10 ++++++++++ include/drm/drm_file.h | 2 ++ include/drm/drm_gem.h | 2 ++ 4 files changed, 22 insertions(+)diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.cindex c4c704e..67c0770 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) goto out_prime_destroy; }+ file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);+ if (!file->dummy_page) { + ret = -ENOMEM; + goto out_prime_destroy; + } + return file; out_prime_destroy: @@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file) if (dev->driver->postclose) dev->driver->postclose(dev, file); + __free_page(file->dummy_page); + drm_prime_destroy_file_private(&file->prime); WARN_ON(!list_empty(&file->event_list)); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 1de2cde..c482e9c 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, ret = drm_prime_add_buf_handle(&file_priv->prime, dma_buf, *handle); + + if (!ret) {+ obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);+ if (!obj->dummy_page) + ret = -ENOMEM; + } +since it will massively increase the memory needed for a prime exported object.I think that this is quite overkill in the first place and for thehot unplug case we can just use the global dummy page as well. Christian.that this would trigger Copy On Write flow in core mm(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7-rc7%2Fsource%2Fmm%2Fmemory.c%23L3977&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d88882ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224016377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=kghiG3VpCJod6YefExoDVPl9X03zNhw3SN5GAxgbnmU%3D&reserved=0)on the next page fault to same address triggered by a write access but then i realized a new COW page will be allocated for each such mapping and this is much more wasteful then having a dedicated page per GEMobject.much.Using memory during the unplug is completely unproblematic because we just released quite a bunch of it by releasing all those system memorybuffers.And I'm pretty sure that COWed pages are correctly accounted towardsthe used memory of a process.So I think if that approach works as intended and the COW pages are released again on unmapping it would be the perfect solution to theproblem. Daniel what do you think?If COW works, sure sounds reasonable. And if we can make sure we managed to drop all the system allocations (otherwise suddenly 2x memory usage, worst case). But I have no idea whether we canretroshoehorn that into an established vma, you might have fun stufflike a mkwrite handler there (which I thought is the COW handler thing, but really no idea). If we need to massively change stuff then I think rw dummy page,allocated on first fault after hotunplug (maybe just make it one per object, that's simplest) seems like the much safer option. Much lesscode that can go wrong. -DanielRegarding COW, i was looking into how to properly implement it fromwithin the fault handler (i.e. ttm_bo_vm_fault) and the main obstacle I hit is that of exclusive access to the vm_area_struct, i need to be able to modifyvma->vm_flags (and vm_page_prot) to remove VM_SHARED bit so COW canbe triggered on subsequent write access fault (herehttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fmm%2Fmemory.c%23L4128&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d88882ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224016377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ziHJtqyHuLrlb0uYKhoWCWhUAZnX0JquE%2BkBJ5Fx%2BNo%3D&reserved=0)but core mm takes only read side mm_sem (here for examplehttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fiommu%2Famd%2Fiommu_v2.c%23L488&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d88882ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224016377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=h360c75Upl3%2FW7im7M1%2BxY%2FXy4gxin%2BkCF1Ui2zFXMs%3D&reserved=0) and so I am not supposed to modify vm_area_struct in this case. I amnot sure if it's legit to write lock tthe mm_sem from this point. I found some discussions about this herehttps://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flkml.iu.edu%2Fhypermail%2Flinux%2Fkernel%2F1909.1%2F02754.html&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d88882ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224021379%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=sx6s1lH%2FvxbIZajc4Yr49vFhxvPEnBHZlTt52D8qvZA%3D&reserved=0 but itwasn't really clear to me what's the solution.In any case, seems to me that easier and more memory saving solutionwould be to just switch to per ttm bo dumy rw page thatwould be allocated on demand as you suggested here. This should alsotake care of imported BOs and flink cases.Then i can drop the per device FD and per GEM object FD dummy BO and the ugly loop i am using in patch 2 to match faulting BO to the rightdummy page. Does this makes sense ?I still don't see the information leak as much of a problem, but if Daniel insists we should probably do this.lot more laissez-faire here. But in general we really don't do thatkind of leaking. Iirc there's even radeonsi bugs because else clears,and radeonsi happily displays gunk :-)But could we at least have only one page per client instead of per BO?I think you can do one page per file descriptor or something like that. But gets annoying with shared bo, especially with dma_buf_mmap forwarding. -DanielChristian - is your concern more with too much page allocations or with extra pointer membercluttering TTM BO struct ?Yes, that is one problem.Because we can allocate the dummy page on demand only whenneeded. It's just seems to me that keeping it per BO streamlines the code as I don't need tohave different handling for local vs imported BOs.Why should you have a difference between local vs imported BOs?For local BO seems like Daniel's suggestion to use vm_area_struct->vm_file->private_data should work as this points to drm_file. For imported BOs private_data will point to dma_buf structure since each imported BO is backed by a pseudo file (created in dma_buf_getfile).Oh, good point. But we could easily fix that now. That should make the mapping code less complex as well.Can you clarify what fix u have in mind ? I assume it's not by altering file->private_data to point to something else as we need to retrieve dmabuf (e.g. dma_buf_mmap_internal)
Ah, crap. You are right that is really tricky because vma->vm_file doesn't point to something useful in this situation.
I was talking about the new vma_set_file() function I've just pushed to drm-misc-next, but that stuff can't be used here.
I still don't see the need to use more than the global dummy page even if that means information leak between processes on unplug.
Christian.
AndreyRegards, Christian.If so,where should we store the dummy RW BO in this case ? In current implementation it's stored in drm_gem_object.P.S For FLINK case it seems to me the handling should be no different then with local BO as the FD used for mmap in this case is still the same one associated with the DRM file.AndreyChristian.Andrey
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
