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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 16.03.26 um 10:50 schrieb Boris Brezillon:

>> Avoid this race by adding a huge_fault callback to drm_gem_shmem_vm_ops so that
>> PMD-sized mappings are handled through the appropriate huge page fault path.

This patch will conflict heavily with the folio-tracking patches that 
has been merged into upstream DRM. How do we deal with that?

Best regards
Thomas

>>
>> Fixes: 211b9a39f261 ("drm/shmem-helper: Map huge pages in fault handler")
>> Signed-off-by: Pedro Demarchi Gomes <pedrodemargomes@gmail.com>
>> ---
>>
>> Changes in v3:
>>   - Pass a try_pmd boolean parameter to drm_gem_shmem_any_fault
>>   - Compile drm_gem_shmem_huge_fault only if CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>> is defined to avoid a build warning
>>
>> Changes in v2: https://lore.kernel.org/dri-devel/20260313141719.3949700-1-pedrodemargomes@gmail.com/
>>   - Keep the #ifdef unindented
>>   - Create drm_gem_shmem_any_fault to handle faults of any order and use
>> drm_gem_shmem_[huge_]fault() as wrappers
>>
>> v1: https://lore.kernel.org/all/20260312155027.1682606-1-pedrodemargomes@gmail.com/
>>
>> ---
>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 63 ++++++++++++++------------
>>   1 file changed, 35 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 7b5a49935ae4..9428c3ab7283 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -550,36 +550,18 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>>
>> -static bool drm_gem_shmem_try_map_pmd(struct vm_fault *vmf, unsigned long addr,
>> -				      struct page *page)
>> -{
>> -#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>> -	unsigned long pfn = page_to_pfn(page);
>> -	unsigned long paddr = pfn << PAGE_SHIFT;
>> -	bool aligned = (addr & ~PMD_MASK) == (paddr & ~PMD_MASK);
>> -
>> -	if (aligned &&
>> -	    pmd_none(*vmf->pmd) &&
>> -	    folio_test_pmd_mappable(page_folio(page))) {
>> -		pfn &= PMD_MASK >> PAGE_SHIFT;
>> -		if (vmf_insert_pfn_pmd(vmf, pfn, false) == VM_FAULT_NOPAGE)
>> -			return true;
>> -	}
>> -#endif
>> -
>> -	return false;
>> -}
>> -
>> -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)
>>   {
>>   	struct vm_area_struct *vma = vmf->vma;
>>   	struct drm_gem_object *obj = vma->vm_private_data;
>>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>   	loff_t num_pages = obj->size >> PAGE_SHIFT;
>> -	vm_fault_t ret;
>> +	vm_fault_t ret = VM_FAULT_FALLBACK;
>>   	struct page **pages = shmem->pages;
>>   	pgoff_t page_offset;
>>   	unsigned long pfn;
>> +	unsigned long paddr;
>> +	bool aligned;
> Those two variables can be moved to the `if (try_pmd)` branch.
>
>>   	/* Offset to faulty address in the VMA. */
>>   	page_offset = vmf->pgoff - vma->vm_pgoff;
>> @@ -593,13 +575,19 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>>   		goto out;
>>   	}
>>
>> -	if (drm_gem_shmem_try_map_pmd(vmf, vmf->address, pages[page_offset])) {
>> -		ret = VM_FAULT_NOPAGE;
>> -		goto out;
>> -	}
>> -
>>   	pfn = page_to_pfn(pages[page_offset]);
>> -	ret = vmf_insert_pfn(vma, vmf->address, pfn);
>> +	if (try_pmd) {
>> +		paddr = pfn << PAGE_SHIFT;
>> +		aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
>> +
>> +		if (aligned &&
>> +		    folio_test_pmd_mappable(page_folio(pages[page_offset]))) {
>> +			pfn &= PMD_MASK >> PAGE_SHIFT;
>> +			ret = vmf_insert_pfn_pmd(vmf, pfn, false);
>> +		}
>> +	} else {
>> +		ret = vmf_insert_pfn(vma, vmf->address, pfn);
>> +	}
> Hm, if we get rid of drm_gem_shmem_try_map_pmd(), we probably
> need something like
>
> 	if (!try_pmd) {
> 		ret = vmf_insert_pfn(vma, vmf->address, pfn);
> 	} else {
> #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
> 		unsigned long paddr = pfn << PAGE_SHIFT;
> 		bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
>
> 		if (aligned &&
> 		    folio_test_pmd_mappable(page_folio(pages[page_offset]))) {
> 			pfn &= PMD_MASK >> PAGE_SHIFT;
> 			ret = vmf_insert_pfn_pmd(vmf, pfn, false);
> 		}
> #else
> 		ret = VM_FAULT_FALLBACK;
> #endif
> 	}
>
> to make sure we're not calling helpers that might not
> exist in some configs (the compile might optimize the
> try_pmd==true branch out already, but better safe than sorry).
>
>>    out:
>>   	dma_resv_unlock(shmem->base.resv);
>> @@ -607,6 +595,22 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>>   	return ret;
>>   }
>>
>> +#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>> +static vm_fault_t drm_gem_shmem_huge_fault(struct vm_fault *vmf,
>> +					   unsigned int order)
>> +{
>> +	if (order != PMD_ORDER)
>> +		return VM_FAULT_FALLBACK;
>> +
>> +	return drm_gem_shmem_any_fault(vmf, true);
>> +}
>> +#endif
>> +
>> +static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>> +{
>> +	return drm_gem_shmem_any_fault(vmf, false);
>> +}
>> +
>>   static void drm_gem_shmem_vm_open(struct vm_area_struct *vma)
>>   {
>>   	struct drm_gem_object *obj = vma->vm_private_data;
>> @@ -643,6 +647,9 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>>
>>   const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>>   	.fault = drm_gem_shmem_fault,
>> +#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>> +	.huge_fault = drm_gem_shmem_huge_fault,
>> +#endif
>>   	.open = drm_gem_shmem_vm_open,
>>   	.close = drm_gem_shmem_vm_close,
>>   };
>> --
>> 2.47.3
>>

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