Re: [PATCH v3] drm/shmem-helper: Fix Map huge page mapping in fault handler

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

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

> Hi
> 
> Am 16.03.26 um 10:28 schrieb Boris Brezillon:
> [...]
> >>> -static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> >>> +static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, bool try_pmd)  
> >> Please write two functions. One for huge pages and one for regular page
> >> faults.  
> > That's actually what the first version of the patch was doing, and I
> > strongly disagree with that. In my opinion, it's more error-prone to
> > have two distinct implementations duplicating most of the logic
> > (locking, bookkeeping, ...) than having both handled in the same
> > function and have dummy wrappers to decide the kind of vmf_insert_pfn
> > to do (_pmd vs regular). Think about the folio_mark_accessed() you
> > recently added, and how easy it would have been to add it to
> > drm_gem_shmem_fault() and forget it in the huge_fault handler.  
> 
> But that would be a _simple_ one-time fix.  All these branching any 
> parameterization will stay for _all_ later changes to that code.

But you'll have to deal with these two distinct fault cases in the long
run anyway. Again, look at the access tracking you've added, it has to
apply to huge folios too, so why have two completely distinct functions
that do exactly the same except for the kind of page they insert into
the page table.

> 
> There's little to be gained from cramming things into single functions. 
> I'd say that we have two distinct fault callbacks for huge pages and 
> regular ones already support this point.

The bulk of the code is the same, the only bits that differ are the
function we use to insert the pfn. Again, if we were to duplicate it'd
be more error-prone, because then you have to update two places every
time you fix/add something. And it's not like we'd be the only ones to
do this sort of multiplexing, see [1][2]. So I'm still very much in
favor of this common drm_gem_shmem_any_fault(), and then have
fault/huge_fault implementation call that thing with the proper
parameters.

[1]https://elixir.bootlin.com/linux/v6.19.8/source/fs/xfs/xfs_file.c#L1889
[2]https://elixir.bootlin.com/linux/v6.19.8/source/fs/fuse/dax.c#L796