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

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

On Mon, 16 Mar 2026 09:45:49 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi Boris,
> 
> thanks for investigating this problem.
> 
> Am 13.03.26 um 18:45 schrieb Boris Brezillon:
> > On Fri, 13 Mar 2026 13:55:21 +0100
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >  
> >> On Fri, 13 Mar 2026 13:43:28 +0100
> >> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>  
> >>> On Fri, 13 Mar 2026 13:18:35 +0100
> >>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>>      
> >>>> On Fri, 13 Mar 2026 12:04:25 +0000
> >>>> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >>>>        
> >>>>>> -----Original Message-----
> >>>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Boris Brezillon
> >>>>>> Sent: 13 March 2026 11:57
> >>>>>> Subject: Re: [PATCH v4 5/6] drm/gem-shmem: Track folio accessed/dirty status in mmap
> >>>>>>
> >>>>>> On Fri, 13 Mar 2026 11:29:47 +0100
> >>>>>> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>>>            
> >>>>>>> Hi
> >>>>>>>
> >>>>>>> Am 13.03.26 um 11:18 schrieb Boris Brezillon:
> >>>>>>> [...]  
> >>>>>>>>>>>> +	if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >= num_pages))
> >>>>>>>>>>>> +		return VM_FAULT_SIGBUS;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +	file_update_time(vma->vm_file);
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +	folio_mark_dirty(page_folio(shmem->pages[page_offset]));  
> >>>>>>>> Do we need a folio_mark_dirty_lock() here?  
> >>>>>>> There is a helper for that with some documentation. [1]  
> >>>>>> This [1] seems to solve the problem for me. Still unsure about the folio_mark_dirty_lock vs
> >>>>>> folio_mark_dirty though.
> >>>>>>
> >>>>>> [1]https://yhbt.net/lore/dri-devel/20260312155027.1682606-1-pedrodemargomes@gmail.com/  
> >>>>> FYI, I used folio_mark_dirty_lock() still it does not solve the issue with weston hang.  
> >>>> The patch I pointed to has nothing to do with folio_mark_dirty_lock(),
> >>>> It's a bug caused by huge page mapping changes.  
> >>> Scratch that. I had a bunch of other changes on top, and it hangs again
> >>> now that I dropped those.  
> >> Seems like it's the combination of huge pages and mkwrite that's
> >> causing issues, if I disable huge pages, it doesn't hang...  
> > I managed to have it working with the following diff. I still need to
> > check why the "map-RO-split+RW-on-demand" approach doesn't work (races
> > between huge_fault and pfn_mkwrite?), but I think it's okay to map the
> > real thing writable on the first attempt anyway (we're not trying to do
> > CoW here, since we're always pointing to the same page, it's just the
> > permissions that change). Note that there's still the race fixed by
> > https://yhbt.net/lore/dri-devel/20260312155027.1682606-1-pedrodemargomes@gmail.com/
> > in this diff, I just tried to keep the diffstat minimal.
> >  
> > --->8---  
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 4500deef4127..4efdce5a60f0 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -561,9 +561,8 @@ static vm_fault_t drm_gem_shmem_try_insert_pfn_pmd(struct vm_fault *vmf, unsigne
> >          bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
> >   
> >          if (aligned && pmd_none(*vmf->pmd)) {
> > -               /* Read-only mapping; split upon write fault */
> >                  pfn &= PMD_MASK >> PAGE_SHIFT;
> > -               return vmf_insert_pfn_pmd(vmf, pfn, false);
> > +               return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);
> >          }
> >   #endif
> >   
> > @@ -597,8 +596,12 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> >   
> >          pfn = page_to_pfn(page);
> >   
> > -       if (folio_test_pmd_mappable(folio))
> > +       if (folio_test_pmd_mappable(folio)) {
> >                  ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
> > +               if (ret == VM_FAULT_NOPAGE && vmf->flags & FAULT_FLAG_WRITE)
> > +                       folio_mark_dirty(folio);
> > +       }
> > +
> >          if (ret != VM_FAULT_NOPAGE)
> >                  ret = vmf_insert_pfn(vma, vmf->address, pfn);  
> 
> All these branches with NOPAGE seem confusing. Can we change the overall 
> logic here? Something like:
> 
> if (folio_test_pmd_mappable()) {
>      ret = try_insert_pfn_pmd()
>      if (ret == VM_FAULT_NOPAGE) {
>          folio_mark_accessed()
>          if (flags & FLAG_WRITE)
>              folio_mark_dirty()
>          goto out;
>      }
> }
> 
> ret = vmf_insert_pfn()
> if (ret == NOPAGE)
>      folio_mark_accesed()
> 
> out:
>    ...
> 
> 
> This would keep the huge-page code within the first branch. And if it 
> fails, it still does the regular page fault.

Well, in practice that's not what we want anyway (see the other fix for
the huge_fault vs regular fault race), so I'd still be inclined to have
the folio_mark_accessed() handled in the common path and have the pmd
vs regular pfn insertion in some if/else branches. Something like that:

	if (pmd_insert)
		ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
	else
		ret = vmf_insert_pfn(vma, vmf->address, pfn);

	if (ret == VM_FAULT_NOPAGE) {
		folio_mark_accesed(folio);

		/* Normal pages are mapped RO, and remapped RW afterwards. */
		if (pmd_insert && vmf->flags & FAULT_FLAG_WRITE)
			folio_mark_dirty(folio);
	}