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

Thomas Zimmermann
2026-03-16T10:22:56Z

Hi

Am 16.03.26 um 10:36 schrieb Boris Brezillon:
[...]
>> 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)

This style mixes conditions from different branching depths. The first
outermost branch uses pmd_insert to compute ret.  Any then both have
changed places, so that ret is in the outer branch and pmd_insert is in
the inner branch.  This is hard to maintain. It is already confusing now
and will be even more so to anyone locking at that code later on.

Best regards
Thomas

> folio_mark_dirty(folio);
> }

--
--
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)