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

[Thomas Zimmermann <tzimmermann@suse.de>]

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.

Best regards
Thomas

>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)