On Mon, 16 Mar 2026 11:22:49 +0100
Thomas Zimmermann <[email protected]> wrote:
> 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.
I guess that's another occurrence of us disagreeing on what's
easy/uneasy to maintain :P. I find it way easier to group things by
functionality (here that would be folio state tracking) at the cost of
having conditionals repeated (I trust the compiler to do the proper
optimization) than having multiple paths doing exactly the same thing.
The latter easily leads to one path being updated while the other path
is left behind when new features/fixes are proposed.
