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
