Re: huge zero page confusion

[Ryan Roberts <ryan.roberts@arm.com>]

On 04/07/2024 12:41, Kirill A. Shutemov wrote:
> On Wed, Jul 03, 2024 at 06:37:48PM +0100, Ryan Roberts wrote:
>> Hi Kirill, Hugh, Mel,
>>
>> We recently had a problem reported at [1] that due to aarch64 arch requiring
>> that atomic RMW instructions raise a read fault, followed by a write fault, this
>> causes a huge zero page to be faulted in during the read fault, then the write
>> fault shatters the huge zero page, installing small zero pages for every PTE in
>> the PMD region, except the faulting address which gets a writable private page.
>>
>> A number of ways were discussed to solve that problem. But it got me wondering
>> why we have this behaviour in general for huge zero page? This seems like odd
>> behaviour to me. Surely it would be less effort and more aligned with the app's
>> expectations to notice the huge zero page in the PMD, remove it, and install a
>> THP, as would have been done if pmd_none() was true? Or if there is a reason to
>> shatter on write, why not do away with the huge zero page and save some memory,
>> and just install a PMD's worth of small zero pages on fault?
>>
>> Perhaps replacing the huge zero page with a huge THP on write fault would have
>> been a better behavior at the time, but perhaps changing that behaviour now
>> risks a memory bloat regression in some workloads?
> 
> Yeah, I agree that WP fault on zero page page should give THP. I think
> treating zero page as none PMD on write page fault should be safe and
> reasonable.

So you're not concerned about potential for memory bloat regressions in apps
that are deployed today? I'm a bit nervous to make the change without a bunch of
testing...

> 
> Untested patch is below.
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 2aa986a5cd1b..04c252303951 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -552,6 +552,11 @@ static inline bool thp_migration_supported(void)
>  }
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  
> +static inline bool pmd_none_or_zero_folio(pmd_t pmd)
> +{
> +	return pmd_none(pmd) || is_huge_zero_pmd(pmd);
> +}
> +
>  static inline int split_folio_to_list_to_order(struct folio *folio,
>  		struct list_head *list, int new_order)
>  {
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 89932fd0f62e..fdd5236004bc 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -951,7 +951,7 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
>  	__folio_mark_uptodate(folio);
>  
>  	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
> -	if (unlikely(!pmd_none(*vmf->pmd))) {
> +	if (unlikely(!pmd_none_or_zero_folio(*vmf->pmd))) {

Hmm not sure about this; Wouldn't we need to "uninstall" the huge zero page
somehow? I'm guessing TLB invalidation and ref count decrement on the zero page
(assuming its ref counted... perhaps its not).

>  		goto unlock_release;
>  	} else {
>  		pmd_t entry;
> @@ -1536,8 +1536,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
>  	vmf->ptl = pmd_lockptr(vma->vm_mm, vmf->pmd);
>  	VM_BUG_ON_VMA(!vma->anon_vma, vma);
>  
> -	if (is_huge_zero_pmd(orig_pmd))
> -		goto fallback;
> +	VM_BUG_ON(is_huge_zero_pmd(orig_pmd));
>  
>  	spin_lock(vmf->ptl);
>  
> @@ -1606,7 +1605,6 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
>  unlock_fallback:
>  	folio_unlock(folio);
>  	spin_unlock(vmf->ptl);
> -fallback:
>  	__split_huge_pmd(vma, vmf->pmd, vmf->address, false, NULL);
>  	return VM_FAULT_FALLBACK;
>  }
> diff --git a/mm/memory.c b/mm/memory.c
> index 0f47a533014e..cc12deeb0593 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5488,15 +5488,15 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
>  	if (pud_trans_unstable(vmf.pud))
>  		goto retry_pud;
>  
> -	if (pmd_none(*vmf.pmd) &&
> +	vmf.orig_pmd = pmdp_get_lockless(vmf.pmd);
> +
> +	if (pmd_none_or_zero_folio(vmf.orig_pmd) &&
>  	    thp_vma_allowable_order(vma, vm_flags,
>  				TVA_IN_PF | TVA_ENFORCE_SYSFS, PMD_ORDER)) {
>  		ret = create_huge_pmd(&vmf);
>  		if (!(ret & VM_FAULT_FALLBACK))
>  			return ret;
>  	} else {
> -		vmf.orig_pmd = pmdp_get_lockless(vmf.pmd);
> -
>  		if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
>  			VM_BUG_ON(thp_migration_supported() &&
>  					  !is_pmd_migration_entry(vmf.orig_pmd));