Skip to contentSkip to conversation

Do not shatter hugezeropage on wp-fault

PATCH Superseded Dev Jain 3 participants 10 messages Sep 24, 2024 – Sep 25, 2024

Subsystems (2)
Memory management THP

v5 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Dev Jain

author reviewer
#0 lore raw

v5 cover Do not shatter hugezeropage on wp-fault

It was observed at [1] and [2] that the current kernel behaviour of shattering a hugezeropage is inconsistent and suboptimal. For a VMA with a THP allowable order, when we write-fault on it, the kernel installs a PMD-mapped THP. On the other hand, if we first get a read fault, we get a PMD pointing to the hugezeropage; subsequent write will trigger a write-protection fault, shattering the hugezeropage into one writable page, and all the other PTEs write-protected. The conclusion being, as compared to the case of a single write-fault, applications have to suffer 512 extra page faults if they were to use the VMA as such, plus we get the overhead of khugepaged trying to replace that area with a THP anyway.

Instead, replace the hugezeropage with a THP on wp-fault.

[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/

The patchset has been rebased on the mm-unstable branch.

v4->v5:
- Directly return VM_FAULT_FALLBACK in case of !folio

v3->v4:
- Renames: pmd_thp_fault_alloc -> vma_alloc_anon_folio_pmd, map_pmd_thp -> map_anon_folio_pmd
- Instead of passing around, compute haddr at various places, similar with gfp flags
- Pass haddr to update_mmu_cache_pmd() instead of unaligned address
- Do not pass vmf to map_anon_folio_pmd
- Do declarations in reverse xmas tree order
- Drop a new line which was introduced accidentally
- Call __pmd_thp_fault_success_stats from map_anon_folio_pmd
- Correctly return NULL from vma_alloc_anon_folio_pmd
- Initialize pgtable to NULL in __do_huge_pmd_anonymous_page, to prevent freeing pgtable when not even allocated
- Drop if conditions from map_anon_folio_pmd, let the caller handle that

v2->v3:
- Drop foliop and order parameters, prefix the thp functions with pmd_
- First allocate THP, then pgtable, not vice-versa
- Move pgtable_trans_huge_deposit() from map_pmd_thp() to caller
- Drop exposing functions in include/linux/huge_mm.h
- Open code do_huge_zero_wp_pmd_locked()
- Release folio in case of pmd change after taking the lock, or check_stable_address_space() returning VM_FAULT_SIGBUS
- Drop uffd-wp preservation. Looking at page_table_check_pmd_flags(), preserving uffd-wp on a writable entry is invalid. Looking at mfill_atomic(), uffd_copy() is a null operation when pmd is marked uffd-wp.

v1->v2:
- Wrap do_huge_zero_wp_pmd_locked() around lock and unlock
- Call thp_fault_alloc() before do_huge_zero_wp_pmd_locked() to avoid
- calling sleeping function from spinlock context

Dev Jain (2):
mm: Abstract THP allocation
mm: Allocate THP on hugezeropage wp-fault

 mm/huge_memory.c | 142 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 100 insertions(+), 42 deletions(-)
Signature · 1 line
2.30.2
author reviewer
#1 lore raw

PATCH v5 1/2 mm: Abstract THP allocation

In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the faulting case when no page is present.

There should be no functional change as a result of applying this patch, except that, as David notes at [1], a PMD-aligned address should be passed to update_mmu_cache_pmd().

[1]: https://lore.kernel.org/all/[email protected]/

Acked-by David Hildenbrand <[email protected]>
Signed-off-by Dev Jain <[email protected]>
---
 mm/huge_memory.c | 98 ++++++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 41 deletions(-)
mm/huge_memory.c +57 -41
index 4e34b7f89daf..bdbf67c18f6c 100644
@@ -1148,47 +1148,81 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
 }
 EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-			struct page *page, gfp_t gfp)
+static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct *vma,
+					      unsigned long addr)
 {
-	struct vm_area_struct *vma = vmf->vma;
-	struct folio *folio = page_folio(page);
-	pgtable_t pgtable;
-	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
-	vm_fault_t ret = 0;
+	unsigned long haddr = addr & HPAGE_PMD_MASK;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	const int order = HPAGE_PMD_ORDER;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		goto out;
+	}
 
+	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
 	if (mem_cgroup_charge(folio, vma->vm_mm, gfp)) {
 		folio_put(folio);
 		count_vm_event(THP_FAULT_FALLBACK);
 		count_vm_event(THP_FAULT_FALLBACK_CHARGE);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
-		return VM_FAULT_FALLBACK;
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
+		return NULL;
 	}
 	folio_throttle_swaprate(folio, gfp);
 
-	pgtable = pte_alloc_one(vma->vm_mm);
-	if (unlikely(!pgtable)) {
-		ret = VM_FAULT_OOM;
-		goto release;
-	}
-
-	folio_zero_user(folio, vmf->address);
+	folio_zero_user(folio, addr);
 	/*
 	 * The memory barrier inside __folio_mark_uptodate makes sure that
 	 * folio_zero_user writes become visible before the set_pmd_at()
 	 * write.
 	 */
 	__folio_mark_uptodate(folio);
+out:
+	return folio;
+}
+
+static void map_anon_folio_pmd(struct folio *folio, pmd_t *pmd,
+			       struct vm_area_struct *vma, unsigned long haddr)
+{
+	pmd_t entry;
+
+	entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
+	entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+	folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
+	folio_add_lru_vma(folio, vma);
+	set_pmd_at(vma->vm_mm, haddr, pmd, entry);
+	update_mmu_cache_pmd(vma, haddr, pmd);
+	add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio;
+	pgtable_t pgtable;
+	vm_fault_t ret = 0;
+
+	folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+	if (unlikely(!folio))
+		return VM_FAULT_FALLBACK;
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
 
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 	if (unlikely(!pmd_none(*vmf->pmd))) {
 		goto unlock_release;
 	} else {
-		pmd_t entry;
-
 		ret = check_stable_address_space(vma->vm_mm);
 		if (ret)
 			goto unlock_release;

@@ -1202,21 +1236,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 			VM_BUG_ON(ret & VM_FAULT_FALLBACK);
 			return ret;
 		}
-
-		entry = mk_huge_pmd(page, vma->vm_page_prot);
-		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-		folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
-		folio_add_lru_vma(folio, vma);
 		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
-		set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
-		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
-		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+		map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
 		mm_inc_nr_ptes(vma->vm_mm);
 		deferred_split_folio(folio, false);
 		spin_unlock(vmf->ptl);
-		count_vm_event(THP_FAULT_ALLOC);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
-		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
 	}
 
 	return 0;

@@ -1283,8 +1307,6 @@ static void set_huge_zero_folio(pgtable_t pgtable, struct mm_struct *mm,
 vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
-	gfp_t gfp;
-	struct folio *folio;
 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
 	vm_fault_t ret;
 

@@ -1335,14 +1357,8 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 		}
 		return ret;
 	}
-	gfp = vma_thp_gfp_mask(vma);
-	folio = vma_alloc_folio(gfp, HPAGE_PMD_ORDER, vma, haddr, true);
-	if (unlikely(!folio)) {
-		count_vm_event(THP_FAULT_FALLBACK);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK);
-		return VM_FAULT_FALLBACK;
-	}
-	return __do_huge_pmd_anonymous_page(vmf, &folio->page, gfp);
+
+	return __do_huge_pmd_anonymous_page(vmf);
 }
 
 static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
Signature · 1 line
2.30.2
Reviewed-by on v5 1/2
#2 lore raw

Re: [PATCH v5 1/2] mm: Abstract THP allocation

Quoted 28 lines · Dev Jain

In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the faulting case when no page is present.

There should be no functional change as a result of applying this patch, except that, as David notes at [1], a PMD-aligned address should be passed to update_mmu_cache_pmd().

[1]: https://lore.kernel.org/all/[email protected]/

Acked-by David Hildenbrand <[email protected]>
Signed-off-by Dev Jain <[email protected]>
---
  mm/huge_memory.c | 98 ++++++++++++++++++++++++++++--------------------
  1 file changed, 57 insertions(+), 41 deletions(-)
mm/huge_memory.c +2 -2
index 4e34b7f89daf..bdbf67c18f6c 100644
@@ -1148,47 +1148,81 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-			struct page *page, gfp_t gfp)
+static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct *vma,
+					      unsigned long addr)

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 1157

Quoted 10 lines
mm/huge_memory.c +4 -5
  {
-	struct vm_area_struct *vma = vmf->vma;
-	struct folio *folio = page_folio(page);
-	pgtable_t pgtable;
-	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
-	vm_fault_t ret = 0;
+	unsigned long haddr = addr & HPAGE_PMD_MASK;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	const int order = HPAGE_PMD_ORDER;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

There is a warning without NUMA,

../mm/huge_memory.c: In function ‘vma_alloc_anon_folio_pmd’:
../mm/huge_memory.c:1154:16: warning: unused variable ‘haddr’ [-Wunused-variable]

 1154 |  unsigned long haddr = addr & HPAGE_PMD_MASK;
      |                ^~~~~
mm/huge_memory.c +2 -2
index c584e77efe10..147a6e069c71 100644
@@ -1151,11 +1151,11 @@ EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct *vma,
                                              unsigned long addr)
 {
-       unsigned long haddr = addr & HPAGE_PMD_MASK;
        gfp_t gfp = vma_thp_gfp_mask(vma);
        const int order = HPAGE_PMD_ORDER;
-       struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
+       struct folio *folio;
 
+       folio = vma_alloc_folio(gfp, order, vma, addr & HPAGE_PMD_MASK, true);
        if (unlikely(!folio)) {
                count_vm_event(THP_FAULT_FALLBACK);
                count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 1161

Quoted 6 lines
mm/huge_memory.c +4 -1
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		goto out;

Maybe return NULL to omit the out?

Reviewed-by Kefeng Wang <[email protected]>
Quoted 127 lines
mm/huge_memory.c +47 -33
+	}
 
+	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
  	if (mem_cgroup_charge(folio, vma->vm_mm, gfp)) {
  		folio_put(folio);
  		count_vm_event(THP_FAULT_FALLBACK);
  		count_vm_event(THP_FAULT_FALLBACK_CHARGE);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
-		return VM_FAULT_FALLBACK;
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
+		return NULL;
  	}
  	folio_throttle_swaprate(folio, gfp);
 
-	pgtable = pte_alloc_one(vma->vm_mm);
-	if (unlikely(!pgtable)) {
-		ret = VM_FAULT_OOM;
-		goto release;
-	}
-
-	folio_zero_user(folio, vmf->address);
+	folio_zero_user(folio, addr);
  	/*
  	 * The memory barrier inside __folio_mark_uptodate makes sure that
  	 * folio_zero_user writes become visible before the set_pmd_at()
  	 * write.
  	 */
  	__folio_mark_uptodate(folio);
+out:
+	return folio;
+}
+
+static void map_anon_folio_pmd(struct folio *folio, pmd_t *pmd,
+			       struct vm_area_struct *vma, unsigned long haddr)
+{
+	pmd_t entry;
+
+	entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
+	entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+	folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
+	folio_add_lru_vma(folio, vma);
+	set_pmd_at(vma->vm_mm, haddr, pmd, entry);
+	update_mmu_cache_pmd(vma, haddr, pmd);
+	add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio;
+	pgtable_t pgtable;
+	vm_fault_t ret = 0;
+
+	folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+	if (unlikely(!folio))
+		return VM_FAULT_FALLBACK;
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
 
  	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
  	if (unlikely(!pmd_none(*vmf->pmd))) {
  		goto unlock_release;
  	} else {
-		pmd_t entry;
-
  		ret = check_stable_address_space(vma->vm_mm);
  		if (ret)
  			goto unlock_release;

@@ -1202,21 +1236,11 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  			VM_BUG_ON(ret & VM_FAULT_FALLBACK);
  			return ret;
  		}
-
-		entry = mk_huge_pmd(page, vma->vm_page_prot);
-		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-		folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
-		folio_add_lru_vma(folio, vma);
  		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
-		set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
-		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
-		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+		map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
  		mm_inc_nr_ptes(vma->vm_mm);
  		deferred_split_folio(folio, false);
  		spin_unlock(vmf->ptl);
-		count_vm_event(THP_FAULT_ALLOC);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
-		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
  	}
 
  	return 0;

@@ -1283,8 +1307,6 @@ static void set_huge_zero_folio(pgtable_t pgtable, struct mm_struct *mm,
  vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
  {
  	struct vm_area_struct *vma = vmf->vma;
-	gfp_t gfp;
-	struct folio *folio;
  	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
  	vm_fault_t ret;
 

@@ -1335,14 +1357,8 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
  		}
  		return ret;
  	}
-	gfp = vma_thp_gfp_mask(vma);
-	folio = vma_alloc_folio(gfp, HPAGE_PMD_ORDER, vma, haddr, true);
-	if (unlikely(!folio)) {
-		count_vm_event(THP_FAULT_FALLBACK);
-		count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK);
-		return VM_FAULT_FALLBACK;
-	}
-	return __do_huge_pmd_anonymous_page(vmf, &folio->page, gfp);
+
+	return __do_huge_pmd_anonymous_page(vmf);
  }
 
  static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
author reviewer
#3 lore raw

Re: [PATCH v5 1/2] mm: Abstract THP allocation

Quoted 46 lines · Kefeng Wang
Quoted 30 lines · Dev Jain

In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the faulting case when no page is present.

There should be no functional change as a result of applying this patch, except that, as David notes at [1], a PMD-aligned address should be passed to update_mmu_cache_pmd().

[1]: https://lore.kernel.org/all/[email protected]/

Acked-by David Hildenbrand <[email protected]>
Signed-off-by Dev Jain <[email protected]>

---
  mm/huge_memory.c | 98 ++++++++++++++++++++++++++++--------------------

  1 file changed, 57 insertions(+), 41 deletions(-)
mm/huge_memory.c +2 -1
index 4e34b7f89daf..bdbf67c18f6c 100644
@@ -1148,47 +1148,81 @@ unsigned long thp_get_unmapped_area(struct 
 file *filp, unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-            struct page *page, gfp_t gfp)
+static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct 
 *vma,
+                          unsigned long addr)
   {

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct · line 1160

Quoted 10 lines
mm/huge_memory.c +4 -5
-    struct vm_area_struct *vma = vmf->vma;
-    struct folio *folio = page_folio(page);
-    pgtable_t pgtable;
-    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
-    vm_fault_t ret = 0;
+    unsigned long haddr = addr & HPAGE_PMD_MASK;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+    const int order = HPAGE_PMD_ORDER;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, 
 true);

There is a warning without NUMA,

Quoted 5 lines

../mm/huge_memory.c: In function ‘vma_alloc_anon_folio_pmd’:
../mm/huge_memory.c:1154:16: warning: unused variable ‘haddr’ [-Wunused-variable] 1154 |  unsigned long haddr = addr & HPAGE_PMD_MASK;
      |                ^~~~~

But why is this happening?

Quoted 22 lines
mm/huge_memory.c +2 -2
index c584e77efe10..147a6e069c71 100644
@@ -1151,11 +1151,11 @@ EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
  static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct *vma,
                                               unsigned long addr)
  {
-       unsigned long haddr = addr & HPAGE_PMD_MASK;
         gfp_t gfp = vma_thp_gfp_mask(vma);
         const int order = HPAGE_PMD_ORDER;
-       struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, 
 true);
+       struct folio *folio;
 
+       folio = vma_alloc_folio(gfp, order, vma, addr & 
 HPAGE_PMD_MASK, true);
         if (unlikely(!folio)) {

                count_vm_event(THP_FAULT_FALLBACK); count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);

Quoted 7 lines

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct · line 1165

Quoted 5 lines
mm/huge_memory.c +4 -0
   - VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+    if (unlikely(!folio)) {
+        count_vm_event(THP_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        goto out;

Maybe return NULL to omit the out?

Ah sorry, I have made a mess of unnecessary delayed returns :)

Quoted 3 lines
Reviewed-by Kefeng Wang <[email protected]>

Thanks!

Quoted 129 lines
Quoted 128 lines
mm/huge_memory.c +43 -11
+    }
   +    VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
       if (mem_cgroup_charge(folio, vma->vm_mm, gfp)) {
           folio_put(folio);
           count_vm_event(THP_FAULT_FALLBACK);
           count_vm_event(THP_FAULT_FALLBACK_CHARGE);
-        count_mthp_stat(HPAGE_PMD_ORDER, 
 MTHP_STAT_ANON_FAULT_FALLBACK);
-        count_mthp_stat(HPAGE_PMD_ORDER, 
 MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
-        return VM_FAULT_FALLBACK;
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
+        return NULL;
       }
       folio_throttle_swaprate(folio, gfp);
   -    pgtable = pte_alloc_one(vma->vm_mm);
-    if (unlikely(!pgtable)) {
-        ret = VM_FAULT_OOM;
-        goto release;
-    }
-
-    folio_zero_user(folio, vmf->address);
+    folio_zero_user(folio, addr);
       /*
        * The memory barrier inside __folio_mark_uptodate makes sure that
        * folio_zero_user writes become visible before the set_pmd_at()
        * write.
        */
       __folio_mark_uptodate(folio);
+out:
+    return folio;
+}
+
+static void map_anon_folio_pmd(struct folio *folio, pmd_t *pmd,
+                   struct vm_area_struct *vma, unsigned long haddr)
+{
+    pmd_t entry;
+
+    entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
+    entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+    folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
+    folio_add_lru_vma(folio, vma);
+    set_pmd_at(vma->vm_mm, haddr, pmd, entry);
+    update_mmu_cache_pmd(vma, haddr, pmd);
+    add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+    count_vm_event(THP_FAULT_ALLOC);
+    count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
+    count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+    struct vm_area_struct *vma = vmf->vma;
+    struct folio *folio;
+    pgtable_t pgtable;
+    vm_fault_t ret = 0;
+
+    folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+    if (unlikely(!folio))
+        return VM_FAULT_FALLBACK;
+
+    pgtable = pte_alloc_one(vma->vm_mm);
+    if (unlikely(!pgtable)) {
+        ret = VM_FAULT_OOM;
+        goto release;
+    }
         vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
       if (unlikely(!pmd_none(*vmf->pmd))) {
           goto unlock_release;
       } else {
-        pmd_t entry;
-

          ret = check_stable_address_space(vma->vm_mm); if (ret)
              goto unlock_release;

mm/huge_memory.c +1 -13
@@ -1202,21 +1236,11 @@ static vm_fault_t 
 __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
               VM_BUG_ON(ret & VM_FAULT_FALLBACK);
               return ret;
           }
-
-        entry = mk_huge_pmd(page, vma->vm_page_prot);
-        entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-        folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
-        folio_add_lru_vma(folio, vma);
           pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
-        set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
-        update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
-        add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
+        map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
           mm_inc_nr_ptes(vma->vm_mm);
           deferred_split_folio(folio, false);
           spin_unlock(vmf->ptl);
-        count_vm_event(THP_FAULT_ALLOC);
-        count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_ALLOC);
-        count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
       }
         return 0;

@@ -1283,8 +1307,6 @@ static void set_huge_zero_folio(pgtable_t 
 pgtable, struct mm_struct *mm,
   vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
   {
       struct vm_area_struct *vma = vmf->vma;
-    gfp_t gfp;
-    struct folio *folio;
       unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
       vm_fault_t ret;

  @@ -1335,14 +1357,8 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf) }
          return ret;
      }
-    gfp = vma_thp_gfp_mask(vma);
-    folio = vma_alloc_folio(gfp, HPAGE_PMD_ORDER, vma, haddr, true); -    if (unlikely(!folio)) {
-        count_vm_event(THP_FAULT_FALLBACK);
-        count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK);
-        return VM_FAULT_FALLBACK;
-    }
-    return __do_huge_pmd_anonymous_page(vmf, &folio->page, gfp);
+
+    return __do_huge_pmd_anonymous_page(vmf);
  }
    static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,

#4 lore raw

Re: [PATCH v5 1/2] mm: Abstract THP allocation

Quoted 48 lines · Dev Jain
Quoted 46 lines · Kefeng Wang
Quoted 30 lines · Dev Jain

In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the faulting case when no page is present.

There should be no functional change as a result of applying this patch, except that, as David notes at [1], a PMD-aligned address should be passed to update_mmu_cache_pmd().

[1]: https://lore.kernel.org/all/ddd3fcd2-48b3-4170- [email protected]/

Acked-by David Hildenbrand <[email protected]>
Signed-off-by Dev Jain <[email protected]>

---
  mm/huge_memory.c | 98 ++++++++++++++++++++++++++++--------------------

  1 file changed, 57 insertions(+), 41 deletions(-)
mm/huge_memory.c +2 -1
index 4e34b7f89daf..bdbf67c18f6c 100644
@@ -1148,47 +1148,81 @@ unsigned long thp_get_unmapped_area(struct 
 file *filp, unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-            struct page *page, gfp_t gfp)
+static struct folio *vma_alloc_anon_folio_pmd(struct vm_area_struct 
 *vma,
+                          unsigned long addr)
   {

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct · line 1160

Quoted 10 lines
mm/huge_memory.c +4 -5
-    struct vm_area_struct *vma = vmf->vma;
-    struct folio *folio = page_folio(page);
-    pgtable_t pgtable;
-    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
-    vm_fault_t ret = 0;
+    unsigned long haddr = addr & HPAGE_PMD_MASK;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+    const int order = HPAGE_PMD_ORDER;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, 
 true);

There is a warning without NUMA,

Quoted 7 lines
Quoted 5 lines

../mm/huge_memory.c: In function ‘vma_alloc_anon_folio_pmd’:
../mm/huge_memory.c:1154:16: warning: unused variable ‘haddr’ [- Wunused-variable] 1154 |  unsigned long haddr = addr & HPAGE_PMD_MASK;
      |                ^~~~~

But why is this happening?

If no CONFIG_NUMA, vma_alloc_folio(...) = folio_alloc_noprof(gfp, order), it won't use haddr.

author reviewer
#5 lore raw

Re: [PATCH v5 1/2] mm: Abstract THP allocation

Quoted 58 lines · Kefeng Wang
Quoted 55 lines · Dev Jain
Quoted 52 lines · Kefeng Wang
Quoted 31 lines · Dev Jain

In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the faulting case when no page is present.

There should be no functional change as a result of applying this patch, except that, as David notes at [1], a PMD-aligned address should be passed to update_mmu_cache_pmd().

[1]: https://lore.kernel.org/all/ddd3fcd2-48b3-4170- [email protected]/

Acked-by David Hildenbrand <[email protected]>
Signed-off-by Dev Jain <[email protected]>

---
  mm/huge_memory.c | 98
++++++++++++++++++++++++++++--------------------

  1 file changed, 57 insertions(+), 41 deletions(-)
mm/huge_memory.c +2 -1
index 4e34b7f89daf..bdbf67c18f6c 100644
@@ -1148,47 +1148,81 @@ unsigned long thp_get_unmapped_area(struct 
 file *filp, unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-            struct page *page, gfp_t gfp)
+static struct folio *vma_alloc_anon_folio_pmd(struct 
 vm_area_struct *vma,
+                          unsigned long addr)
   {

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct · line 1160

Quoted 10 lines
mm/huge_memory.c +4 -5
-    struct vm_area_struct *vma = vmf->vma;
-    struct folio *folio = page_folio(page);
-    pgtable_t pgtable;
-    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
-    vm_fault_t ret = 0;
+    unsigned long haddr = addr & HPAGE_PMD_MASK;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+    const int order = HPAGE_PMD_ORDER;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, 
 true);

There is a warning without NUMA,

../mm/huge_memory.c: In function ‘vma_alloc_anon_folio_pmd’:
../mm/huge_memory.c:1154:16: warning: unused variable ‘haddr’ [- Wunused-variable] 1154 |  unsigned long haddr = addr & HPAGE_PMD_MASK;
      |                ^~~~~

Quoted 4 lines
Quoted 1 line

But why is this happening?

If no CONFIG_NUMA, vma_alloc_folio(...) = folio_alloc_noprof(gfp, order), it won't use haddr.

Ah got it, thanks, I missed the ifdeffery in include/linux/gfp.h.

Quoted 2 lines
author reviewer
#6 lore raw

PATCH v5 2/2 mm: Allocate THP on hugezeropage wp-fault

In reply to Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Remember to flush TLB entry corresponding to the hugezeropage. In case of failure, fallback to splitting the PMD.

Signed-off-by Dev Jain <[email protected]>
---
 mm/huge_memory.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)
mm/huge_memory.c +43 -1
index bdbf67c18f6c..fbb195bc2038 100644
@@ -1790,6 +1790,41 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
 	spin_unlock(vmf->ptl);
 }
 
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf)
+{
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	struct vm_area_struct *vma = vmf->vma;
+	struct mmu_notifier_range range;
+	struct folio *folio;
+	vm_fault_t ret = 0;
+
+	folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+	if (unlikely(!folio)) {
+		ret = VM_FAULT_FALLBACK;
+		goto out;
+	}
+
+	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, haddr,
+				haddr + HPAGE_PMD_SIZE);
+	mmu_notifier_invalidate_range_start(&range);
+	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
+	if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd)))
+		goto release;
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto release;
+	(void)pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+	map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
+	goto unlock;
+release:
+	folio_put(folio);
+unlock:
+	spin_unlock(vmf->ptl);
+	mmu_notifier_invalidate_range_end(&range);
+out:
+	return ret;
+}
+
 vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
 {
 	const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1802,8 +1837,15 @@ 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))
+	if (is_huge_zero_pmd(orig_pmd)) {
+		vm_fault_t ret = do_huge_zero_wp_pmd(vmf);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
 		goto fallback;
+	}
 
 	spin_lock(vmf->ptl);
Signature · 1 line
2.30.2
Acked-by on v5 2/2
#7 lore raw

Re: [PATCH v5 2/2] mm: Allocate THP on hugezeropage wp-fault

Quoted 6 lines · Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Remember to flush TLB entry corresponding to the hugezeropage. In case of failure, fallback to splitting the PMD.

Signed-off-by Dev Jain <[email protected]>

Nothing jumped at me and it looks much cleaner now

Acked-by David Hildenbrand <[email protected]>
Signature · 3 lines
Cheers,

David / dhildenb
Reviewed-by on v5 2/2
#8 lore raw

Re: [PATCH v5 2/2] mm: Allocate THP on hugezeropage wp-fault

In reply to Dev Jain
Quoted 19 lines · Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Remember to flush TLB entry corresponding to the hugezeropage. In case of failure, fallback to splitting the PMD.

Signed-off-by Dev Jain <[email protected]>
---
  mm/huge_memory.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 43 insertions(+), 1 deletion(-)
mm/huge_memory.c +1 -0
index bdbf67c18f6c..fbb195bc2038 100644
@@ -1790,6 +1790,41 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
  	spin_unlock(vmf->ptl);
  }
 
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf)

mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1803

Quoted 10 lines
mm/huge_memory.c +10 -0
+{
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	struct vm_area_struct *vma = vmf->vma;
+	struct mmu_notifier_range range;
+	struct folio *folio;
+	vm_fault_t ret = 0;
+
+	folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+	if (unlikely(!folio)) {
+		ret = VM_FAULT_FALLBACK;

I'd like to return VM_FAULT_FALLBACK if you re-post.

Reviewed-by Kefeng Wang <[email protected]>
Quoted 44 lines
mm/huge_memory.c +32 -1
+		goto out;
+	}
+
+	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, haddr,
+				haddr + HPAGE_PMD_SIZE);
+	mmu_notifier_invalidate_range_start(&range);
+	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
+	if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd)))
+		goto release;
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto release;
+	(void)pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+	map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
+	goto unlock;
+release:
+	folio_put(folio);
+unlock:
+	spin_unlock(vmf->ptl);
+	mmu_notifier_invalidate_range_end(&range);
+out:
+	return ret;
+}
+
  vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
  {
  	const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1802,8 +1837,15 @@ 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))
+	if (is_huge_zero_pmd(orig_pmd)) {
+		vm_fault_t ret = do_huge_zero_wp_pmd(vmf);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
  		goto fallback;
+	}
 
  	spin_lock(vmf->ptl);
author reviewer
#9 lore raw

Re: [PATCH v5 2/2] mm: Allocate THP on hugezeropage wp-fault

Quoted 25 lines · Kefeng Wang
Quoted 22 lines · Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Remember to flush TLB entry corresponding to the hugezeropage. In case of failure, fallback to splitting the PMD.

Signed-off-by Dev Jain <[email protected]>

---
  mm/huge_memory.c | 44 +++++++++++++++++++++++++++++++++++++++++++-

  1 file changed, 43 insertions(+), 1 deletion(-)
mm/huge_memory.c +4 -0
index bdbf67c18f6c..fbb195bc2038 100644
@@ -1790,6 +1790,41 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
       spin_unlock(vmf->ptl);
   }
   +static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf)
+{
+    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+    struct vm_area_struct *vma = vmf->vma;
+    struct mmu_notifier_range range;
Quoted 8 lines

mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1803

Quoted 6 lines
mm/huge_memory.c +6 -0
+    struct folio *folio;
+    vm_fault_t ret = 0;
+
+    folio = vma_alloc_anon_folio_pmd(vma, vmf->address);
+    if (unlikely(!folio)) {
+        ret = VM_FAULT_FALLBACK;

I'd like to return VM_FAULT_FALLBACK if you re-post.

That's cleaner. I need to keep my "goto" obsession in control...

Quoted 2 lines
Reviewed-by Kefeng Wang <[email protected]>

Thanks!

Quoted 46 lines
Quoted 44 lines
mm/huge_memory.c +24 -0
+        goto out;
+    }
+
+    mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, 
 haddr,
+                haddr + HPAGE_PMD_SIZE);
+    mmu_notifier_invalidate_range_start(&range);
+    vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
+    if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd)))
+        goto release;
+    ret = check_stable_address_space(vma->vm_mm);
+    if (ret)
+        goto release;
+    (void)pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+    map_anon_folio_pmd(folio, vmf->pmd, vma, haddr);
+    goto unlock;
+release:
+    folio_put(folio);
+unlock:
+    spin_unlock(vmf->ptl);
+    mmu_notifier_invalidate_range_end(&range);
+out:
+    return ret;
+}
+
   vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
   {

      const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

mm/huge_memory.c +8 -0
@@ -1802,8 +1837,15 @@ 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))
+    if (is_huge_zero_pmd(orig_pmd)) {
+        vm_fault_t ret = do_huge_zero_wp_pmd(vmf);
+
+        if (!(ret & VM_FAULT_FALLBACK))
+            return ret;
+
+        /* Fallback to splitting PMD if THP cannot be allocated */
           goto fallback;
+    }
         spin_lock(vmf->ptl);