Re: [PATCH v3 5/6] drm/gem-shmem: Track folio accessed/dirty status in mmap

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 9 Feb 2026 15:46:21 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi Boris,
> 
> thanks for reviewing the series.
> 
> Am 09.02.26 um 15:23 schrieb Boris Brezillon:
> > On Mon,  9 Feb 2026 14:27:14 +0100
> > Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >  
> >> Invoke folio_mark_accessed() in mmap page faults to add the folio to
> >> the memory manager's LRU list. Userspace invokes mmap to get the memory
> >> for software rendering. Compositors do the same when creating the final
> >> on-screen image, so keeping the pages in LRU makes sense. Avoids paging
> >> out graphics buffers when under memory pressure.
> >>
> >> In pfn_mkwrite, further invoke the folio_mark_dirty() to add the folio
> >> for writeback should the underlying file be paged out from system memory.
> >> This rarely happens in practice, yet it would corrupt the buffer content.
> >>
> >> This has little effect on a system's hardware-accelerated rendering, which
> >> only mmaps for an initial setup of textures, meshes, shaders, etc.
> >>
> >> v3:
> >> - rewrite for VM_PFNMAP
> >> v2:
> >> - adapt to changes in drm_gem_shmem_try_mmap_pmd()
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> >> ---
> >>   drivers/gpu/drm/drm_gem_shmem_helper.c | 20 ++++++++++++++++++++
> >>   1 file changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index c3a054899ba3..0c86ad40a049 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -598,6 +598,9 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> >>   	if (ret != VM_FAULT_NOPAGE)
> >>   		ret = vmf_insert_pfn(vma, vmf->address, pfn);
> >>   
> >> +	if (likely(!(ret & VM_FAULT_ERROR)))  
> > Can't we just go
> >
> > 	if (ret == VM_FAULT_NOPAGE)
> >
> > here?  
> 
> After reviewing the code in vmf_insert_pfn, I think so. All we'll see is 
> _OOM and _SIGBUS; or _NOPAGE on success.  I'll change it then.
> 
> 
> >  
> >> +		folio_mark_accessed(folio);
> >> +
> >>   out:
> >>   	dma_resv_unlock(obj->resv);
> >>   
> >> @@ -638,10 +641,27 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
> >>   	drm_gem_vm_close(vma);
> >>   }
> >>   
> >> +static vm_fault_t drm_gem_shmem_pfn_mkwrite(struct vm_fault *vmf)
> >> +{
> >> +	struct vm_area_struct *vma = vmf->vma;
> >> +	struct drm_gem_object *obj = vma->vm_private_data;
> >> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> >> +	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within VMA */
> >> +	struct page *page = shmem->pages[page_offset];  
> > Should we have a
> >
> > 	if (WARN_ON(!shmem->pages ||
> > 		    page_offset <= (obj->size >> PAGE_SHIFT)))
> > 		return VM_FAULT_SIGBUS;
> >
> >
> > ?  
> 
> I left it out because it doesn't seem necessary.  In the fault handler 
> in drm_gem_shmem_fault(), I can see that we could get an OOB access. But 
> we only call pfn_mkwrite() after going through _fault() first. I don't 
> see a way of getting here unless we've already tested for the page in 
> _fault().

I agree it's not supposed to happen, but isn't it what WARN_ON()s are
for (catching unexpected situations)?