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

[Thomas Zimmermann]

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.

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.

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)