Re: huge zero page confusion

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

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.

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))) {
 		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));
-- 
  Kiryl Shutsemau / Kirill A. Shutemov