Do not shatter hugezeropage on wp-fault

PATCH Superseded Dev Jain 4 participants 22 messages Sep 4, 2024 – Sep 6, 2024

Subsystems (3)

Memory management MM core THP

lore

Share & tools

lore

Dev Jain

author reviewer

2024-09-04 10:09 UTC #0 lore raw

v2 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.

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

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

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

 include/linux/huge_mm.h |   6 ++
 mm/huge_memory.c        | 171 +++++++++++++++++++++++++++++-----------
 mm/memory.c             |   5 +-
 3 files changed, 136 insertions(+), 46 deletions(-)

Signature · 1 line

2.30.2

Dev Jain

author reviewer

2024-09-04 10:09 UTC #1 lore raw

PATCH v2 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.

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

---
 mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 43 deletions(-)

mm/huge_memory.c +67 ~~-43~~

index 67c86a5d64a6..58125fbcc532 100644
@@ -943,47 +943,89 @@ 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 vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+				  unsigned long haddr, struct folio **foliop,
+				  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;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	*foliop = folio;
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		return VM_FAULT_FALLBACK;
+	}
 
+	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);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
 		return VM_FAULT_FALLBACK;
 	}
 	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);
+	return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+			struct vm_area_struct *vma, unsigned long haddr,
+			pgtable_t pgtable)
+{
+	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);
+	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);
+	mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio = NULL;
+	pgtable_t pgtable;
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret = 0;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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;

@@ -997,20 +1039,9 @@ 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);
-		mm_inc_nr_ptes(vma->vm_mm);
+		map_pmd_thp(folio, vmf, vma, haddr, pgtable);
 		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);
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
 	}
 
 	return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 release:
 	if (pgtable)
 		pte_free(vma->vm_mm, pgtable);
-	folio_put(folio);
+	if (folio)
+		folio_put(folio);
 	return ret;
 
 }

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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

Kirill A. Shutemov

2024-09-05 08:21 UTC #2 lore raw

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

Quoted 17 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.
Signed-off-by Dev Jain <[email protected]>
---
 mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644

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

Quoted 8 lines

mm/huge_memory.c +3 -2

@@ -943,47 +943,89 @@ 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 vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+				  unsigned long haddr, struct folio **foliop,
+				  unsigned long addr)

foliop is awkward.

Why not return folio? NULL would indicate to the caller to fallback.

Quoted 79 lines

mm/huge_memory.c +47 ~~-15~~

 {
-	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;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	*foliop = folio;
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		return VM_FAULT_FALLBACK;
+	}
 
+	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);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
 		return VM_FAULT_FALLBACK;
 	}
 	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);
+	return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+			struct vm_area_struct *vma, unsigned long haddr,
+			pgtable_t pgtable)
+{
+	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);
+	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);
+	mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio = NULL;
+	pgtable_t pgtable;
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret = 0;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+

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

Quoted 10 lines

mm/huge_memory.c +10 -0

+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		goto release;

THP page allocation has higher probability to fail than pgtable allocation. It is better to allocate it first, before pgtable and do less work on error path.

Quoted 72 lines

mm/huge_memory.c +7 ~~-26~~

 	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;

@@ -997,20 +1039,9 @@ 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);
-		mm_inc_nr_ptes(vma->vm_mm);
+		map_pmd_thp(folio, vmf, vma, haddr, pgtable);
 		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);
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
 	}
 
 	return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 release:
 	if (pgtable)
 		pte_free(vma->vm_mm, pgtable);
-	folio_put(folio);
+	if (folio)
+		folio_put(folio);
 	return ret;
 
 }

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

Dev Jain

author reviewer

2024-09-05 08:45 UTC #3 lore raw

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

Quoted 21 lines · Kirill A. Shutemov

Quoted 20 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.
Signed-off-by Dev Jain <[email protected]>
---
  mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
  1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644
@@ -943,47 +943,89 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 

Quoted 8 lines

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 948
Quoted 5 lines
mm/huge_memory.c +3 -2
-static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-			struct page *page, gfp_t gfp)
+static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+				  unsigned long haddr, struct folio **foliop,
+				  unsigned long addr)
foliop is awkward.
Why not return folio? NULL would indicate to the caller to fallback.

I took inspiration from other call sites like mfill_atomic_pte_copy() which have a double pointer to the folio. If we do as you say, then in thp_fault_alloc(), we will have to do all the fallback stat computation, and return the folio, then check in the caller whether that is NULL, then set ret = VM_FAULT_FALLBACK, then goto out/release. Basically, stat computation for fallback, and the actual setting of return value to fallback would get separated. Currently, I can simultaneously set the folio pointer and the return value.

Quoted 85 lines

mm/huge_memory.c +52 ~~-15~~

  {
-	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;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	*foliop = folio;
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		return VM_FAULT_FALLBACK;
+	}
 
+	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);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
  		return VM_FAULT_FALLBACK;
  	}
  	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);
+	return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+			struct vm_area_struct *vma, unsigned long haddr,
+			pgtable_t pgtable)
+{
+	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);
+	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);
+	mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio = NULL;
+	pgtable_t pgtable;
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret = 0;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}

Quoted 8 lines

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 1022
Quoted 5 lines
mm/huge_memory.c +5 -0
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		goto release;
THP page allocation has higher probability to fail than pgtable allocation. It is better to allocate it first, before pgtable and do less work on error path.

Thanks, makes sense.

Ryan Roberts

reviewer

2024-09-05 11:08 UTC #4 lore raw

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

In reply to Dev Jain

Quoted 17 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.
Signed-off-by Dev Jain <[email protected]>
---
 mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644

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

Quoted 6 lines

mm/huge_memory.c +1 -2

@@ -943,47 +943,89 @@ 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 vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,

Is there a reason for specifying order as a parameter? Previously it was hardcoded to HPAGE_PMD_ORDER. But now, thp_fault_alloc() and __thp_fault_success_stats() both take order and map_pmd_thp() is still implicitly mapping a PMD-sized block. Unless there is a reason you need this parameter in the next patch (I don't think there is?) I suggest simplifying.

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

Quoted 1 line

mm/huge_memory.c +1 -0

+				  unsigned long haddr, struct folio **foliop,

FWIW, I agree with Kirill's suggestion to just return folio* and drop the output param.

Thanks,
Ryan

Quoted 160 lines

mm/huge_memory.c +65 ~~-41~~

+				  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;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	*foliop = folio;
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		return VM_FAULT_FALLBACK;
+	}
 
+	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);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
 		return VM_FAULT_FALLBACK;
 	}
 	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);
+	return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+			struct vm_area_struct *vma, unsigned long haddr,
+			pgtable_t pgtable)
+{
+	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);
+	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);
+	mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio = NULL;
+	pgtable_t pgtable;
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret = 0;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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;

@@ -997,20 +1039,9 @@ 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);
-		mm_inc_nr_ptes(vma->vm_mm);
+		map_pmd_thp(folio, vmf, vma, haddr, pgtable);
 		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);
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
 	}
 
 	return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 release:
 	if (pgtable)
 		pte_free(vma->vm_mm, pgtable);
-	folio_put(folio);
+	if (folio)
+		folio_put(folio);
 	return ret;
 
 }

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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,

Dev Jain

author reviewer

2024-09-06 05:42 UTC #5 lore raw

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

Quoted 21 lines · Ryan Roberts

Quoted 20 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.
Signed-off-by Dev Jain <[email protected]>
---
  mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
  1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644
@@ -943,47 +943,89 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 

Quoted 8 lines

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 946
Quoted 3 lines
mm/huge_memory.c +1 -2
-static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
-			struct page *page, gfp_t gfp)
+static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
Is there a reason for specifying order as a parameter? Previously it was hardcoded to HPAGE_PMD_ORDER. But now, thp_fault_alloc() and __thp_fault_success_stats() both take order and map_pmd_thp() is still implicitly mapping a PMD-sized block. Unless there is a reason you need this parameter in the next patch (I don't think there is?) I suggest simplifying.

If I am not wrong, thp_fault_alloc() and __thp_fault_success_stats() will remain the same in case of mTHP? I chose to pass order so that these functions can be used by others in the future. Therefore, these two functions can be generically used in the future while map_pmd_thp() (as the name suggests) maps only a PMD-mappable THP.

Quoted 4 lines

mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 947
Quoted 2 lines
mm/huge_memory.c +1 -0
+				  unsigned long haddr, struct folio **foliop,
FWIW, I agree with Kirill's suggestion to just return folio* and drop the output param.

As I replied to Kirill, I do not think that is a good idea. If you do a git grep on the tree for "foliop", you will find several places where that is being used, for example, check out alloc_charge_folio() in mm/khugepaged.c. The author intends to do the stat computation and setting *foliop in the function itself, so that the caller is only concerned with the return value.
Also, if we return the folio instead of VM_FAULT_FALLBACK from thp_fault_alloc(), then you will have two "if (unlikely(!folio))" branches, the first to do the stat computation in the function, and the second branch in the caller to set ret = VM_FAULT_FALLBACK and then goto out/release.
And, it is already inconsistent to break away the stat computation and the return value setting, when the stat computation name is of the form "count_vm_event(THP_FAULT_FALLBACK)" and friends, at which point it would just be better to open code the function.

If I am missing something and your suggestion can be implemented neatly, please guide.

Quoted 164 lines

Thanks,
Ryan

Quoted 160 lines

mm/huge_memory.c +65 ~~-41~~

+				  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;
+	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
 
-	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+	*foliop = folio;
+	if (unlikely(!folio)) {
+		count_vm_event(THP_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		return VM_FAULT_FALLBACK;
+	}
 
+	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);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
  		return VM_FAULT_FALLBACK;
  	}
  	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);
+	return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+	count_vm_event(THP_FAULT_ALLOC);
+	count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+			struct vm_area_struct *vma, unsigned long haddr,
+			pgtable_t pgtable)
+{
+	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);
+	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);
+	mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio = NULL;
+	pgtable_t pgtable;
+	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret = 0;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+
+	pgtable = pte_alloc_one(vma->vm_mm);
+	if (unlikely(!pgtable)) {
+		ret = VM_FAULT_OOM;
+		goto release;
+	}
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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;

@@ -997,20 +1039,9 @@ 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);
-		mm_inc_nr_ptes(vma->vm_mm);
+		map_pmd_thp(folio, vmf, vma, haddr, pgtable);
  		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);
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
  	}
 
  	return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  release:
  	if (pgtable)
  		pte_free(vma->vm_mm, pgtable);
-	folio_put(folio);
+	if (folio)
+		folio_put(folio);
  	return ret;
 
  }

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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,

Ryan Roberts

reviewer

2024-09-06 08:28 UTC #6 lore raw

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

Quoted 33 lines · Dev Jain

Quoted 22 lines · Ryan Roberts
Quoted 21 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.
Signed-off-by Dev Jain <[email protected]>
---
mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
  1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644
@@ -943,47 +943,89 @@ 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,
Quoted 8 lines
mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, · line 948
Quoted 3 lines
mm/huge_memory.c +1 -1
-            struct page *page, gfp_t gfp)
+static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct
 vm_area_struct *vma,
Is there a reason for specifying order as a parameter? Previously it was hardcoded to HPAGE_PMD_ORDER. But now, thp_fault_alloc() and __thp_fault_success_stats() both take order and map_pmd_thp() is still implicitly mapping a PMD-sized block. Unless there is a reason you need this parameter in the next patch (I don't think there is?) I suggest simplifying.

Quoted 2 lines

If I am not wrong, thp_fault_alloc() and __thp_fault_success_stats() will remain the same in case of mTHP?

No, its a bit different for smaller-than-pmd THP - that's handled in alloc_anon_folio() in memory.c. It deals with fallback to smaller orders (inc order-0. Potentially alloc_anon_folio() could be refactored to use your new functions in future but I'm sure there would be a number of subtlties to consider and in any case you would want to make that a separate change. The order param would be added as part of that future change.

Quoted 2 lines

I chose to pass order so that these
functions can be used by others in the future.

It's not valuable to have an internal interface with no users. My advice is to keep it simple and only extend it when a user pops up.

Quoted 6 lines

Therefore, these two functions
can be generically used in the future while map_pmd_thp() (as the name suggests) maps only a PMD-mappable THP.
Quoted 2 lines
Quoted 2 lines
mm/huge_memory.c +1 -0
+                  unsigned long haddr, struct folio **foliop,

Quoted 8 lines

Quoted 2 lines
FWIW, I agree with Kirill's suggestion to just return folio* and drop the output param.
As I replied to Kirill, I do not think that is a good idea. If you do a git grep on the tree for "foliop", you will find several places where that is being used, for example, check out alloc_charge_folio() in mm/khugepaged.c. The author intends to do the stat computation and setting *foliop in the function itself, so that the caller is only concerned with the return value.

By having 2 return params, you open up the possibility of returning an invalid combination (e.g. ret==FALLBACK, folio!=NULL). You avoid that by having a single return param, which is either NULL (failure) or a valid pointer.

Quoted 5 lines

Also, if we return the folio instead of VM_FAULT_FALLBACK from thp_fault_alloc(), then you will have two "if (unlikely(!folio))" branches, the first to do the stat computation in the function, and the second branch in the caller to set ret = VM_FAULT_FALLBACK
and then goto out/release.

There are already 2 conditionals, that doesn't change.

Quoted 5 lines

And, it is already inconsistent to break away the stat computation and the return value
setting, when the stat computation name is of the form "count_vm_event(THP_FAULT_FALLBACK)"
and friends, at which point it would just be better to open code the function.

I don't understand this.

Quoted 3 lines

If I am missing something and your suggestion can be implemented neatly, please guide.

This looks cleaner/simpler to my eye (also removing the order parameter):

static folio *thp_fault_alloc(gfp_t gfp, struct vm_area_struct *vma,
unsigned long haddr, unsigned long addr)

{
	const int order = HPAGE_PMD_ORDER;
	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

	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(order, MTHP_STAT_ANON_FAULT_FALLBACK);
		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
		goto out;
	}
	folio_throttle_swaprate(folio, gfp);

	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 vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

{
	vm_fault_t ret = VM_FAULT_FALLBACK;
	...

	folio = thp_fault_alloc(gfp, vma, haddr, vmf->address);
	if (unlikely(!folio))
		goto release;
	...
}

Thanks,
Ryan

Quoted 161 lines

Thanks,
Ryan

Quoted 156 lines

mm/huge_memory.c +60 ~~-28~~

+                  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;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
   -    VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+    *foliop = folio;
+    if (unlikely(!folio)) {
+        count_vm_event(THP_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        return VM_FAULT_FALLBACK;
+    }
   +    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);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
           return VM_FAULT_FALLBACK;
       }
       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);
+    return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+    count_vm_event(THP_FAULT_ALLOC);
+    count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+    count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+            struct vm_area_struct *vma, unsigned long haddr,
+            pgtable_t pgtable)
+{
+    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);
+    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);
+    mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    struct folio *folio = NULL;
+    pgtable_t pgtable;
+    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+    vm_fault_t ret = 0;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+
+    pgtable = pte_alloc_one(vma->vm_mm);
+    if (unlikely(!pgtable)) {
+        ret = VM_FAULT_OOM;
+        goto release;
+    }
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
+    if (ret)
+        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;

@@ -997,20 +1039,9 @@ 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);
-        mm_inc_nr_ptes(vma->vm_mm);
+        map_pmd_thp(folio, vmf, vma, haddr, pgtable);
           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);
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
       }
         return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct
 vm_fault *vmf,

release:
      if (pgtable)
          pte_free(vma->vm_mm, pgtable);
-    folio_put(folio);
+    if (folio)
+        folio_put(folio);
      return ret;
}

mm/huge_memory.c +0 -2

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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,

Dev Jain

author reviewer

2024-09-06 08:45 UTC #7 lore raw

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

Quoted 32 lines · Ryan Roberts

Quoted 31 lines · Dev Jain
Quoted 30 lines · Ryan Roberts
Quoted 17 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.
Signed-off-by Dev Jain <[email protected]>
---
   mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
   1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644
mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, · line 948
Quoted 7 lines
mm/huge_memory.c +1 -1
@@ -943,47 +943,89 @@ 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 vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct
 vm_area_struct *vma,
Is there a reason for specifying order as a parameter? Previously it was hardcoded to HPAGE_PMD_ORDER. But now, thp_fault_alloc() and __thp_fault_success_stats() both take order and map_pmd_thp() is still implicitly mapping a PMD-sized block. Unless there is a reason you need this parameter in the next patch (I don't think there is?) I suggest simplifying.

Quoted 8 lines

Quoted 2 lines
If I am not wrong, thp_fault_alloc() and __thp_fault_success_stats() will remain the same in case of mTHP?
No, its a bit different for smaller-than-pmd THP - that's handled in alloc_anon_folio() in memory.c. It deals with fallback to smaller orders (inc order-0. Potentially alloc_anon_folio() could be refactored to use your new functions in future but I'm sure there would be a number of subtlties to consider and in any case you would want to make that a separate change. The order param would be added as part of that future change.

Okay.

Quoted 5 lines

Quoted 3 lines
I chose to pass order so that these
functions can be used by others in the future.
It's not valuable to have an internal interface with no users. My advice is to keep it simple and only extend it when a user pops up.

Makes sense. In that case, should I call it pmd_thp_fault_alloc() to enforce that this is not a non-PMD THP?

Quoted 8 lines

Quoted 8 lines
Therefore, these two functions
can be generically used in the future while map_pmd_thp() (as the name suggests) maps only a PMD-mappable THP.
Quoted 3 lines
mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 947
Quoted 1 line
mm/huge_memory.c +1 -0
+                  unsigned long haddr, struct folio **foliop,
FWIW, I agree with Kirill's suggestion to just return folio* and drop the output param.

Quoted 8 lines

Quoted 5 lines
As I replied to Kirill, I do not think that is a good idea. If you do a git grep on the tree for "foliop", you will find several places where that is being used, for example, check out alloc_charge_folio() in mm/khugepaged.c. The author intends to do the stat computation and setting *foliop in the function itself, so that the caller is only concerned with the return value.
By having 2 return params, you open up the possibility of returning an invalid combination (e.g. ret==FALLBACK, folio!=NULL). You avoid that by having a single return param, which is either NULL (failure) or a valid pointer.

Okay, I agree with this in a generic sense.

Quoted 55 lines

Quoted 6 lines
Also, if we return the folio instead of VM_FAULT_FALLBACK from thp_fault_alloc(), then you will have two "if (unlikely(!folio))" branches, the first to do the stat computation in the function, and the second branch in the caller to set ret = VM_FAULT_FALLBACK
and then goto out/release.
There are already 2 conditionals, that doesn't change.
Quoted 5 lines
And, it is already inconsistent to break away the stat computation and the return value
setting, when the stat computation name is of the form "count_vm_event(THP_FAULT_FALLBACK)"
and friends, at which point it would just be better to open code the function.
I don't understand this.
Quoted 2 lines
If I am missing something and your suggestion can be implemented neatly, please guide.
This looks cleaner/simpler to my eye (also removing the order parameter):
static folio *thp_fault_alloc(gfp_t gfp, struct vm_area_struct *vma,
unsigned long haddr, unsigned long addr)
{
	const int order = HPAGE_PMD_ORDER;
	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

	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(order, MTHP_STAT_ANON_FAULT_FALLBACK);
		count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
		goto out;
	}
	folio_throttle_swaprate(folio, gfp);

	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 vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

Quoted 10 lines

{
	vm_fault_t ret = VM_FAULT_FALLBACK;
	...

	folio = thp_fault_alloc(gfp, vma, haddr, vmf->address);
	if (unlikely(!folio))
		goto release;
	...
}

Thanks for your help. I will switch to this, just that, ret should still be initialized to zero in case we spot the pmd changes after taking the lock. So I'll set it in the case of !folio.

Quoted 160 lines

Thanks,
Ryan

Quoted 156 lines

mm/huge_memory.c +62 ~~-31~~

+                  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;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
   -    VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+    *foliop = folio;
+    if (unlikely(!folio)) {
+        count_vm_event(THP_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        return VM_FAULT_FALLBACK;
+    }
   +    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);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
           return VM_FAULT_FALLBACK;
       }
       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);
+    return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+    count_vm_event(THP_FAULT_ALLOC);
+    count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+    count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+            struct vm_area_struct *vma, unsigned long haddr,
+            pgtable_t pgtable)
+{
+    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);
+    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);
+    mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    struct folio *folio = NULL;
+    pgtable_t pgtable;
+    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+    vm_fault_t ret = 0;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+
+    pgtable = pte_alloc_one(vma->vm_mm);
+    if (unlikely(!pgtable)) {
+        ret = VM_FAULT_OOM;
+        goto release;
+    }
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
+    if (ret)
+        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;

@@ -997,20 +1039,9 @@ 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);
-        mm_inc_nr_ptes(vma->vm_mm);
+        map_pmd_thp(folio, vmf, vma, haddr, pgtable);
           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);
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
       }
         return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct
 vm_fault *vmf,
   release:
       if (pgtable)
           pte_free(vma->vm_mm, pgtable);
-    folio_put(folio);
+    if (folio)
+        folio_put(folio);
       return ret;
     }

@@ -1077,8 +1109,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;

@@ -1129,14 +1159,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,

Ryan Roberts

reviewer

2024-09-06 09:00 UTC #8 lore raw

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

Quoted 46 lines · Dev Jain

Quoted 32 lines · Ryan Roberts
Quoted 31 lines · Dev Jain
Quoted 30 lines · Ryan Roberts
Quoted 17 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.
Signed-off-by Dev Jain <[email protected]>
---
mm/huge_memory.c | 110 +++++++++++++++++++++++++++++------------------
   1 file changed, 67 insertions(+), 43 deletions(-)
mm/huge_memory.c +0 -0
index 67c86a5d64a6..58125fbcc532 100644
mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, · line 948
Quoted 7 lines
mm/huge_memory.c +1 -1
@@ -943,47 +943,89 @@ 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 vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct
 vm_area_struct *vma,
Is there a reason for specifying order as a parameter? Previously it was hardcoded to HPAGE_PMD_ORDER. But now, thp_fault_alloc() and __thp_fault_success_stats() both take order and map_pmd_thp() is still implicitly mapping a PMD-sized block. Unless there is a reason you need this parameter in the next patch (I don't think there is?) I suggest simplifying.
Quoted 8 lines
Quoted 2 lines
If I am not wrong, thp_fault_alloc() and __thp_fault_success_stats() will remain the same in case of mTHP?
No, its a bit different for smaller-than-pmd THP - that's handled in alloc_anon_folio() in memory.c. It deals with fallback to smaller orders (inc order-0. Potentially alloc_anon_folio() could be refactored to use your new functions in future but I'm sure there would be a number of subtlties to consider and in any case you would want to make that a separate change. The order param would be added as part of that future change.
Okay.
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I chose to pass order so that these
functions can be used by others in the future.
It's not valuable to have an internal interface with no users. My advice is to keep it simple and only extend it when a user pops up.
Makes sense. In that case, should I call it pmd_thp_fault_alloc() to enforce that this is not a non-PMD THP?

Yeah, that works for me.

Quoted 92 lines

Quoted 18 lines
Quoted 10 lines
Therefore, these two functions
can be generically used in the future while map_pmd_thp() (as the name suggests) maps only a PMD-mappable THP.
Quoted 4 lines
mm/huge_memory.c unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, · line 947
Quoted 1 line
mm/huge_memory.c +1 -0
+                  unsigned long haddr, struct folio **foliop,
FWIW, I agree with Kirill's suggestion to just return folio* and drop the output
param.
Quoted 8 lines
As I replied to Kirill, I do not think that is a good idea. If you do a git grep on
the tree for "foliop", you will find several places where that is being used, for
example, check out alloc_charge_folio() in mm/khugepaged.c. The author intends to
do the stat computation and setting *foliop in the function itself, so that the caller is only concerned with the return value.
Quoted 3 lines
By having 2 return params, you open up the possibility of returning an invalid combination (e.g. ret==FALLBACK, folio!=NULL). You avoid that by having a single return param, which is either NULL (failure) or a valid pointer.
Okay, I agree with this in a generic sense.
Quoted 61 lines
Quoted 8 lines
Also, if we return the folio instead of VM_FAULT_FALLBACK from thp_fault_alloc(),
then you will have two "if (unlikely(!folio))" branches, the first to do the stat
computation in the function, and the second branch in the caller to set ret = VM_FAULT_FALLBACK
and then goto out/release.
There are already 2 conditionals, that doesn't change.
Quoted 5 lines
And, it is already inconsistent to break away the stat computation and the return value
setting, when the stat computation name is of the form "count_vm_event(THP_FAULT_FALLBACK)"
and friends, at which point it would just be better to open code the function.
I don't understand this.
Quoted 2 lines
If I am missing something and your suggestion can be implemented neatly, please guide.
This looks cleaner/simpler to my eye (also removing the order parameter):
static folio *thp_fault_alloc(gfp_t gfp, struct vm_area_struct *vma, unsigned long haddr, unsigned long addr)
{
    const int order = HPAGE_PMD_ORDER;
    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
    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(order, MTHP_STAT_ANON_FAULT_FALLBACK); count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE); goto out;
    }
    folio_throttle_swaprate(folio, gfp);
    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 vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf) {
    vm_fault_t ret = VM_FAULT_FALLBACK;
    ...
Quoted 6 lines
    folio = thp_fault_alloc(gfp, vma, haddr, vmf->address); if (unlikely(!folio))
        goto release;
    ...
}

Quoted 3 lines

Thanks for your help. I will switch to this, just that, ret should still be initialized to zero in case we spot the pmd changes after taking the lock. So I'll set it in the case of !folio.

ret = thp_fault_alloc() will have already set it back to zero so there isn't a bug. But if you prefer to explicitly set it in the "if" to make the code clearer to read, that's fine by me.

Thanks,
Ryan

Quoted 163 lines

Thanks,
Ryan

Quoted 158 lines

mm/huge_memory.c +58 ~~-15~~

+                  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;
+    struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
    -    VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+    *foliop = folio;
+    if (unlikely(!folio)) {
+        count_vm_event(THP_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        return VM_FAULT_FALLBACK;
+    }
    +    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);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK);
+        count_mthp_stat(order, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
            return VM_FAULT_FALLBACK;
        }
        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);
+    return 0;
+}
+
+static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
+{
+    count_vm_event(THP_FAULT_ALLOC);
+    count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
+    count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+}
+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+            struct vm_area_struct *vma, unsigned long haddr,
+            pgtable_t pgtable)
+{
+    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);
+    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);
+    mm_inc_nr_ptes(vma->vm_mm);
+}
+
+static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    struct folio *folio = NULL;
+    pgtable_t pgtable;
+    unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+    vm_fault_t ret = 0;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+
+    pgtable = pte_alloc_one(vma->vm_mm);
+    if (unlikely(!pgtable)) {
+        ret = VM_FAULT_OOM;
+        goto release;
+    }
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
+    if (ret)
+        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 +2 ~~-13~~

@@ -997,20 +1039,9 @@ 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);
-        mm_inc_nr_ptes(vma->vm_mm);
+        map_pmd_thp(folio, vmf, vma, haddr, pgtable);
            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);
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
        }
          return 0;

@@ -1019,7 +1050,8 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct
 vm_fault *vmf,

   release:
       if (pgtable)
           pte_free(vma->vm_mm, pgtable);
-    folio_put(folio);
+    if (folio)
+        folio_put(folio);
       return ret;
   }

mm/huge_memory.c +0 -2

@@ -1077,8 +1109,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;

   @@ -1129,14 +1159,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,

Dev Jain

author reviewer

2024-09-04 10:10 UTC #9 lore raw

PATCH v2 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. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.

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

---
 include/linux/huge_mm.h |  6 ++++
 mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
 mm/memory.c             |  5 +--
 3 files changed, 78 insertions(+), 12 deletions(-)

include/linux/huge_mm.h +6 -0

index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
 #include <linux/kobject.h>
 
 vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);
 int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
 		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +69 ~~-10~~

113 lines not on this page · raw message

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
 	if (vma_is_anonymous(vma)) {
 		if (likely(!unshare) &&
 		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
 				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
 		}
 		return do_huge_pmd_wp_page(vmf);
 	}

Signature · 1 line

2.30.2

Kirill A. Shutemov

2024-09-05 08:27 UTC #10 lore raw

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

Quoted 18 lines · Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
 include/linux/huge_mm.h |  6 ++++
 mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
 mm/memory.c             |  5 +--
 3 files changed, 78 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +0 -0
index e25d9ebfdf89..fdd2cf473a3c 100644

include/linux/huge_mm.h line 17

Quoted 9 lines

include/linux/huge_mm.h +6 -0

@@ -9,6 +9,12 @@
 #include <linux/kobject.h>
 
 vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);

Why? I don't see users outside huge_memory.c.

Quoted 57 lines

include/linux/huge_mm.h +0 -0

 int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
 		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +17 -9

index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
 }
 EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-				  unsigned long haddr, struct folio **foliop,
-				  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr)
 {
 	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
 	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
 }
 
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-			struct vm_area_struct *vma, unsigned long haddr,
-			pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable)
 {
-	pmd_t entry;
+	pmd_t entry, old_pmd;
+	bool is_pmd_none = pmd_none(*vmf->pmd);
 
 	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);
-	pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+	if (!is_pmd_none) {
+		old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+		if (pmd_uffd_wp(old_pmd))
+			entry = pmd_mkuffd_wp(entry);
+	}
+	if (pgtable)
+		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);
-	mm_inc_nr_ptes(vma->vm_mm);
+	if (is_pmd_none)
+		mm_inc_nr_ptes(vma->vm_mm);
 }
 
 static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

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

Quoted 6 lines

mm/huge_memory.c +3 -0

@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
 	spin_unlock(vmf->ptl);
 }
 
+static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+					     unsigned long haddr,
+					     struct folio *folio)

Why the helper is needed? Cannot it be just opencodded in do_huge_zero_wp_pmd()?

Quoted 81 lines

mm/huge_memory.c +49 -1

+{
+	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
+
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto out;
+	map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+	return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long haddr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	struct mmu_notifier_range range;
+	struct folio *folio = NULL;
+	vm_fault_t ret = 0;
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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 unlock;
+	ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+	if (!ret)
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+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;

@@ -1588,8 +1640,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, haddr);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
 		goto fallback;
+	}
 
 	spin_lock(vmf->ptl);

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
 	if (vma_is_anonymous(vma)) {
 		if (likely(!unshare) &&
 		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
 				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
 		}
 		return do_huge_pmd_wp_page(vmf);
 	}

Signature · 1 line

2.30.2

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

Dev Jain

author reviewer

2024-09-05 08:52 UTC #11 lore raw

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

Quoted 21 lines · Kirill A. Shutemov

Quoted 20 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
  include/linux/huge_mm.h |  6 ++++
  mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
  mm/memory.c             |  5 +--
  3 files changed, 78 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +0 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
  #include <linux/kobject.h>
 

Quoted 8 lines

include/linux/huge_mm.h line 17

Quoted 7 lines

include/linux/huge_mm.h +6 -0

  vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);

Why? I don't see users outside huge_memory.c.

Ah sorry! When I started out on this I may have planned to use it in mm/memory.c, but then completely forgot to drop this.

Quoted 61 lines

include/linux/huge_mm.h +0 -0

  int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
  		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
  		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +17 -9

index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-				  unsigned long haddr, struct folio **foliop,
-				  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr)
  {
  	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
  	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
  }
 
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-			struct vm_area_struct *vma, unsigned long haddr,
-			pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable)
  {
-	pmd_t entry;
+	pmd_t entry, old_pmd;
+	bool is_pmd_none = pmd_none(*vmf->pmd);
 
  	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);
-	pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+	if (!is_pmd_none) {
+		old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+		if (pmd_uffd_wp(old_pmd))
+			entry = pmd_mkuffd_wp(entry);
+	}
+	if (pgtable)
+		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);
-	mm_inc_nr_ptes(vma->vm_mm);
+	if (is_pmd_none)
+		mm_inc_nr_ptes(vma->vm_mm);
  }
 
  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
  	spin_unlock(vmf->ptl);
  }

Quoted 5 lines

mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1589
Quoted 3 lines
mm/huge_memory.c +3 -0
+static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+					     unsigned long haddr,
+					     struct folio *folio)
Why the helper is needed? Cannot it be just opencodded in do_huge_zero_wp_pmd()?

I was taking inspiration from split_huge_pud_locked() and the like. Although I guess I should open code it, since do_huge_zero_wp_pmd_locked() is small anyways.

Quoted 82 lines

mm/huge_memory.c +49 -1

+{
+	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
+
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto out;
+	map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+	return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long haddr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	struct mmu_notifier_range range;
+	struct folio *folio = NULL;
+	vm_fault_t ret = 0;
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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 unlock;
+	ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+	if (!ret)
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+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;

@@ -1588,8 +1640,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, haddr);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
  		goto fallback;
+	}
 
  	spin_lock(vmf->ptl);

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
  	if (vma_is_anonymous(vma)) {
  		if (likely(!unshare) &&
  		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
  				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
  		}
  		return do_huge_pmd_wp_page(vmf);
  	}

Signature · 1 line

2.30.2

Kefeng Wang

2024-09-05 09:41 UTC #12 lore raw

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

In reply to Kirill A. Shutemov

Quoted 6 lines · Kirill A. Shutemov

Quoted 5 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.

We met same issue, and a very similar function in our kernel,

Quoted 121 lines

Quoted 13 lines

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

---
  include/linux/huge_mm.h |  6 ++++
  mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
  mm/memory.c             |  5 +--
  3 files changed, 78 insertions(+), 12 deletions(-)

include/linux/huge_mm.h +0 -0

index e25d9ebfdf89..fdd2cf473a3c 100644

include/linux/huge_mm.h line 17

Quoted 9 lines

include/linux/huge_mm.h +6 -0

@@ -9,6 +9,12 @@
  #include <linux/kobject.h>
 
  vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);

Why? I don't see users outside huge_memory.c.

Quoted 57 lines

include/linux/huge_mm.h +0 -0

  int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
  		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
  		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +17 -9

index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-				  unsigned long haddr, struct folio **foliop,
-				  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr)
  {
  	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
  	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
  }
 
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-			struct vm_area_struct *vma, unsigned long haddr,
-			pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable)
  {
-	pmd_t entry;
+	pmd_t entry, old_pmd;
+	bool is_pmd_none = pmd_none(*vmf->pmd);
 
  	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);
-	pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+	if (!is_pmd_none) {
+		old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+		if (pmd_uffd_wp(old_pmd))
+			entry = pmd_mkuffd_wp(entry);
+	}
+	if (pgtable)
+		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);
-	mm_inc_nr_ptes(vma->vm_mm);
+	if (is_pmd_none)
+		mm_inc_nr_ptes(vma->vm_mm);
  }
 
  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

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

Quoted 6 lines

mm/huge_memory.c +3 -0

@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
  	spin_unlock(vmf->ptl);
  }
 
+static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+					     unsigned long haddr,
+					     struct folio *folio)

Why the helper is needed? Cannot it be just opencodded in do_huge_zero_wp_pmd()?

Quoted 29 lines

mm/huge_memory.c +29 -0

+{
+	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
+
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto out;
+	map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+	return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long haddr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	struct mmu_notifier_range range;
+	struct folio *folio = NULL;
+	vm_fault_t ret = 0;
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
+	if (ret)
+		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);

Quoted 8 lines

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

Quoted 8 lines

mm/huge_memory.c +8 -0

+	if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd)))
+		goto unlock;
+	ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+	if (!ret)
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+	spin_unlock(vmf->ptl);
+	mmu_notifier_invalidate_range_end(&range);

the folio need to released when !pmd_same() and check_stable_address_space() return VM_FAULT_SIGBUS.

Quoted 45 lines

Quoted 44 lines

mm/huge_memory.c +12 -1

+out:
+	return ret;
+}
+
  vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
  {
  	const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1588,8 +1640,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, haddr);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
  		goto fallback;
+	}
 
  	spin_lock(vmf->ptl);

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
  	if (vma_is_anonymous(vma)) {
  		if (likely(!unshare) &&
  		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
  				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
  		}
  		return do_huge_pmd_wp_page(vmf);
  	}

Signature · 1 line

2.30.2

Dev Jain

author reviewer

2024-09-05 09:53 UTC #13 lore raw

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

Quoted 133 lines · Kefeng Wang

Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
We met same issue, and a very similar function in our kernel,
Quoted 122 lines
Quoted 14 lines
Signed-off-by Dev Jain <[email protected]>
---
include/linux/huge_mm.h | 6 ++++
mm/huge_memory.c | 79
+++++++++++++++++++++++++++++++++++------
mm/memory.c | 5 +--
  3 files changed, 78 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +0 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
include/linux/huge_mm.h line 17
Quoted 9 lines
include/linux/huge_mm.h +6 -0
@@ -9,6 +9,12 @@
   #include <linux/kobject.h>
     vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct 
 vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable);
Why? I don't see users outside huge_memory.c.
Quoted 54 lines
include/linux/huge_mm.h +0 -0
   int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
             pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
             struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);
mm/huge_memory.c +3 -2
index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file 
 *filp, unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-                  unsigned long haddr, struct folio **foliop,
-                  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct 
 vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr)
   {
      struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order) count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
}
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-            struct vm_area_struct *vma, unsigned long haddr,
-            pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable)
{
-    pmd_t entry;
+    pmd_t entry, old_pmd;
+    bool is_pmd_none = pmd_none(*vmf->pmd);
      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);
-    pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+    if (!is_pmd_none) {
+        old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+        if (pmd_uffd_wp(old_pmd))
+            entry = pmd_mkuffd_wp(entry);
+    }
+    if (pgtable)
+        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);
-    mm_inc_nr_ptes(vma->vm_mm);
+    if (is_pmd_none)
+        mm_inc_nr_ptes(vma->vm_mm);
}
static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1588
Quoted 5 lines
mm/huge_memory.c +2 -0
@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
       spin_unlock(vmf->ptl);
   }
   +static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+                         unsigned long haddr,
+                         struct folio *folio)
Why the helper is needed? Cannot it be just opencodded in do_huge_zero_wp_pmd()?
Quoted 33 lines
mm/huge_memory.c +31 -0
+{
+    struct vm_area_struct *vma = vmf->vma;
+    vm_fault_t ret = 0;
+
+    ret = check_stable_address_space(vma->vm_mm);
+    if (ret)
+        goto out;
+    map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+    return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, 
 unsigned long haddr)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+    struct mmu_notifier_range range;
+    struct folio *folio = NULL;
+    vm_fault_t ret = 0;
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
+    if (ret)
+        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 unlock;

Quoted 8 lines

Quoted 6 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1626
Quoted 6 lines
mm/huge_memory.c +6 -0
+    ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+    if (!ret)
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+    spin_unlock(vmf->ptl);
+    mmu_notifier_invalidate_range_end(&range);
the folio need to released when !pmd_same() and check_stable_address_space() return VM_FAULT_SIGBUS.

Yes, thanks.

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
mm/huge_memory.c +4 -0
+out:
+    return ret;
+}
+
 

Ryan Roberts

reviewer

2024-09-05 13:14 UTC #14 lore raw

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

In reply to Dev Jain

Quoted 18 lines · Dev Jain

Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
 include/linux/huge_mm.h |  6 ++++
 mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
 mm/memory.c             |  5 +--
 3 files changed, 78 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +0 -0
index e25d9ebfdf89..fdd2cf473a3c 100644

include/linux/huge_mm.h line 17

Quoted 9 lines

include/linux/huge_mm.h +6 -0

@@ -9,6 +9,12 @@
 #include <linux/kobject.h>
 
 vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);

I don't think you are using either of these outside of huge_memory.c, so not sure you need to declare them here or make them non-static?

Quoted 34 lines

include/linux/huge_mm.h +0 -0

 int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
 		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +8 -7

index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
 }
 EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-				  unsigned long haddr, struct folio **foliop,
-				  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr)
 {
 	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
 	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
 }
 
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-			struct vm_area_struct *vma, unsigned long haddr,
-			pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable)
 {
-	pmd_t entry;
+	pmd_t entry, old_pmd;
+	bool is_pmd_none = pmd_none(*vmf->pmd);

mm/huge_memory.c static void __thp_fault_success_stats(struct vm_area_struct *vma, int order) · line 1001

Quoted 10 lines

mm/huge_memory.c +4 -1

 
 	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);
-	pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+	if (!is_pmd_none) {
+		old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+		if (pmd_uffd_wp(old_pmd))
+			entry = pmd_mkuffd_wp(entry);

I don't really get this; entry is writable, so I wouldn't expect to also be setting uffd-wp here? That combination is not allowed and is checked for in page_table_check_pte_flags().

It looks like you expect to get here in the uffd-wp-async case, which used to cause the pmd to be split to ptes. I'm guessing (but don't know for sure) that would cause the uffd-wp bit to be set in each of the new ptes, then during fallback to handling the wp fault on the pte, uffd would handle it?

mm/huge_memory.c static void __thp_fault_success_stats(struct vm_area_struct *vma, int order) · line 1004

Quoted 3 lines

mm/huge_memory.c +3 -0

+	}
+	if (pgtable)
+		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);

Should this call be moved outside of here? It doesn't really feel like it belongs. Could it be called before calling map_pmd_thp() for the site that has a pgtable?

Quoted 28 lines

mm/huge_memory.c +17 -1

 	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);
-	mm_inc_nr_ptes(vma->vm_mm);
+	if (is_pmd_none)
+		mm_inc_nr_ptes(vma->vm_mm);
 }
 
 static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
 	spin_unlock(vmf->ptl);
 }
 
+static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+					     unsigned long haddr,
+					     struct folio *folio)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
+
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto out;
+	map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+	return ret;
+}
+

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

Quoted 10 lines

mm/huge_memory.c +10 -0

+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long haddr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	gfp_t gfp = vma_thp_gfp_mask(vma);
+	struct mmu_notifier_range range;
+	struct folio *folio = NULL;
+	vm_fault_t ret = 0;
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);

Just checking: the PTE table was already allocated during the read fault, right? So we don't have to allocate it here.

Quoted 5 lines

mm/huge_memory.c +5 -0

+	if (ret)
+		goto out;
+
+	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, haddr,
+				haddr + HPAGE_PMD_SIZE);

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

Quoted 10 lines

mm/huge_memory.c +10 -0

+	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 unlock;
+	ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+	if (!ret)
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+	spin_unlock(vmf->ptl);
+	mmu_notifier_invalidate_range_end(&range);

I'll confess I don't understand all the mmu notifier rules. But the doc at Documentation/mm/mmu_notifier.rst implies that the notification must be done while holding the PTL. Although that's not how wp_page_copy(). Are you confident what you have done is correct?

Thanks,
Ryan

Quoted 41 lines

mm/huge_memory.c +12 -1

+out:
+	return ret;
+}
+
 vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
 {
 	const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1588,8 +1640,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, haddr);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
 		goto fallback;
+	}
 
 	spin_lock(vmf->ptl);

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
 	if (vma_is_anonymous(vma)) {
 		if (likely(!unshare) &&
 		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
 				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
 		}
 		return do_huge_pmd_wp_page(vmf);
 	}

Dev Jain

author reviewer

2024-09-06 07:05 UTC #15 lore raw

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

Quoted 21 lines · Ryan Roberts

Quoted 20 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
  include/linux/huge_mm.h |  6 ++++
  mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
  mm/memory.c             |  5 +--
  3 files changed, 78 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +0 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
  #include <linux/kobject.h>
 

Quoted 9 lines

include/linux/huge_mm.h line 17

Quoted 7 lines

include/linux/huge_mm.h +6 -0

  vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable);

I don't think you are using either of these outside of huge_memory.c, so not sure you need to declare them here or make them non-static?

As pointed out by Kirill, you are right, I forgot to drop these from my previous approach.

Quoted 49 lines

Quoted 35 lines

include/linux/huge_mm.h +0 -0

  int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
  		  pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
  		  struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);

mm/huge_memory.c +8 -7

index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
  }
  EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
 
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-				  unsigned long haddr, struct folio **foliop,
-				  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+			   unsigned long haddr, struct folio **foliop,
+			   unsigned long addr)
  {
  	struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);

@@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
  	count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
  }
 
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-			struct vm_area_struct *vma, unsigned long haddr,
-			pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+		 struct vm_area_struct *vma, unsigned long haddr,
+		 pgtable_t pgtable)
  {
-	pmd_t entry;
+	pmd_t entry, old_pmd;
+	bool is_pmd_none = pmd_none(*vmf->pmd);

mm/huge_memory.c static void __thp_fault_success_stats(struct vm_area_struct *vma, int order) · line 1001

Quoted 10 lines

mm/huge_memory.c +4 -1

 
  	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);
-	pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
+	if (!is_pmd_none) {
+		old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd);
+		if (pmd_uffd_wp(old_pmd))
+			entry = pmd_mkuffd_wp(entry);

I don't really get this; entry is writable, so I wouldn't expect to also be setting uffd-wp here? That combination is not allowed and is checked for in page_table_check_pte_flags().

Quoted 4 lines

It looks like you expect to get here in the uffd-wp-async case, which used to cause the pmd to be split to ptes. I'm guessing (but don't know for sure) that would cause the uffd-wp bit to be set in each of the new ptes, then during fallback to handling the wp fault on the pte, uffd would handle it?

I guess you are correct; I missed the WARN_ON() in page_table_check_pmd_flags(), but I did see, if I read the uffd code correctly, that mfill_atomic() will just return in case of pmd_trans_huge(*dst_pmd) while doing a uffd_copy to the destination location. So preserving pmd_uffd_wp is useless in case a THP is mapped, but I did not know that in fact it is supposed to be an invalid combination. So, I will drop it, unless someone has some other objection.

Quoted 7 lines

mm/huge_memory.c static void __thp_fault_success_stats(struct vm_area_struct *vma, int order) · line 1004
Quoted 4 lines
mm/huge_memory.c +3 -0
+	}
+	if (pgtable)
+		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
Should this call be moved outside of here? It doesn't really feel like it belongs. Could it be called before calling map_pmd_thp() for the site that has a pgtable?

Every other place I checked, they are doing this: make the entry -> deposit pgtable -> set_pmd_at(). I guess the general flow is to do the deposit based on the old pmd, before setting the new one. Which implies: I should at least move this check before I call pmdp_huge_clear_flush(). And, since vmf->pmd and creating the new entry has no relation, I am inclined to do what you are saying.

Quoted 33 lines

mm/huge_memory.c +21 -1

  	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);
-	mm_inc_nr_ptes(vma->vm_mm);
+	if (is_pmd_none)
+		mm_inc_nr_ptes(vma->vm_mm);
  }
 
  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)

@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
  	spin_unlock(vmf->ptl);
  }
 
+static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+					     unsigned long haddr,
+					     struct folio *folio)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
+
+	ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		goto out;
+	map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+	return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long haddr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	gfp_t gfp = vma_thp_gfp_mask(vma);

Quoted 8 lines

mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1611
Quoted 6 lines
mm/huge_memory.c +6 -0
+	struct mmu_notifier_range range;
+	struct folio *folio = NULL;
+	vm_fault_t ret = 0;
+
+	ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+			      vmf->address);
Just checking: the PTE table was already allocated during the read fault, right? So we don't have to allocate it here.

Correct, that happens in set_huge_zero_folio(). Thanks for checking.

Quoted 17 lines

Quoted 6 lines

mm/huge_memory.c +5 -0

+	if (ret)
+		goto out;
+
+	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, haddr,
+				haddr + HPAGE_PMD_SIZE);

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

Quoted 10 lines

mm/huge_memory.c +10 -0

+	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 unlock;
+	ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+	if (!ret)
+		__thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+	spin_unlock(vmf->ptl);
+	mmu_notifier_invalidate_range_end(&range);

I'll confess I don't understand all the mmu notifier rules.

I confess the same :)

Quoted 4 lines

But the doc at
Documentation/mm/mmu_notifier.rst implies that the notification must be done while holding the PTL. Although that's not how wp_page_copy(). Are you confident what you have done is correct?

Everywhere else, invalidate_range_end() is getting called after dropping the lock, one reason is that it has a might_sleep(), and therefore we cannot call it while holding a spinlock. I still don't know what exactly these calls mean...but I think what I have done is correct.

Quoted 45 lines

Thanks,
Ryan

Quoted 41 lines

mm/huge_memory.c +12 -1

+out:
+	return ret;
+}
+
  vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
  {
  	const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1588,8 +1640,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, haddr);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+
+		/* Fallback to splitting PMD if THP cannot be allocated */
  		goto fallback;
+	}
 
  	spin_lock(vmf->ptl);

mm/memory.c +3 -2

index 3c01d68065be..c081a25f5173 100644
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
  	if (vma_is_anonymous(vma)) {
  		if (likely(!unshare) &&
  		    userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-			if (userfaultfd_wp_async(vmf->vma))
+			if (!userfaultfd_wp_async(vmf->vma))
+				return handle_userfault(vmf, VM_UFFD_WP);
+			if (!is_huge_zero_pmd(vmf->orig_pmd))
  				goto split;
-			return handle_userfault(vmf, VM_UFFD_WP);
  		}
  		return do_huge_pmd_wp_page(vmf);
  	}

Ryan Roberts

reviewer

2024-09-06 08:43 UTC #16 lore raw

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

Quoted 88 lines · Dev Jain

Quoted 14 lines · Ryan Roberts
Quoted 13 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
include/linux/huge_mm.h | 6 ++++
mm/huge_memory.c | 79 +++++++++++++++++++++++++++++++++++------ mm/memory.c | 5 +--
  3 files changed, 78 insertions(+), 12 deletions(-)
Quoted 15 lines
include/linux/huge_mm.h line 16
Quoted 13 lines
include/linux/huge_mm.h +6 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
   #include <linux/kobject.h>
     vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable);
I don't think you are using either of these outside of huge_memory.c, so not sure you need to declare them here or make them non-static?
As pointed out by Kirill, you are right, I forgot to drop these from my previous approach.
Quoted 48 lines
Quoted 36 lines
include/linux/huge_mm.h +0 -0
   int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
             pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
             struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);
mm/huge_memory.c +0 -0
index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp,
 unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
-static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-                  unsigned long haddr, struct folio **foliop,
-                  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma, +               unsigned long haddr, struct folio **foliop,
+               unsigned long addr)
{
      struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true); @@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
      count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
}
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf, -            struct vm_area_struct *vma, unsigned long haddr,
-            pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable)
{
-    pmd_t entry;
+    pmd_t entry, old_pmd;
+    bool is_pmd_none = pmd_none(*vmf->pmd);
      entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
Quoted 8 lines
      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); +    if (!is_pmd_none) {
+        old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd); +        if (pmd_uffd_wp(old_pmd))
+            entry = pmd_mkuffd_wp(entry);
I don't really get this; entry is writable, so I wouldn't expect to also be setting uffd-wp here? That combination is not allowed and is checked for in page_table_check_pte_flags().
Quoted 4 lines
It looks like you expect to get here in the uffd-wp-async case, which used to cause the pmd to be split to ptes. I'm guessing (but don't know for sure) that would cause the uffd-wp bit to be set in each of the new ptes, then during fallback to handling the wp fault on the pte, uffd would handle it?

Quoted 8 lines

I guess you are correct; I missed the WARN_ON() in page_table_check_pmd_flags(), but I did see, if I read the uffd code correctly, that mfill_atomic() will just return in case of pmd_trans_huge(*dst_pmd) while doing a uffd_copy to the destination
location. So preserving pmd_uffd_wp is useless in case a THP is mapped, but I did not
know that in fact it is supposed to be an invalid combination. So, I will drop it, unless someone has some other objection.

So what's the correct way to handle uffd-wp-async in wp_huge_pmd()? Just split it? If so, you can revert your changes in memory.c.

Quoted 9 lines

Quoted 8 lines
Quoted 5 lines
+    }
+    if (pgtable)
+        pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
Should this call be moved outside of here? It doesn't really feel like it belongs. Could it be called before calling map_pmd_thp() for the site that has a pgtable?

Quoted 8 lines

Every other place I checked, they are doing this: make the entry -> deposit pgtable ->
set_pmd_at(). I guess the general flow is to do the deposit based on the old pmd, before
setting the new one. Which implies: I should at least move this check before I call pmdp_huge_clear_flush(). And, since vmf->pmd and creating the new entry has no relation,
I am inclined to do what you are saying.

pgtable_trans_huge_deposit() is just adding the pgtable to a list so that if the THP needs to be split in future, then we have preallocated the pte pgtable so the operation can't fail. And enqueing it is just under the protection of the PTL as far as I can tell. So I think the only ordering requirement is that you both set the pmd and deposit the pgtable under the lock (without dropping it in between). So you can deposit either before or after map_pmd_thp(). And pmdp_huge_clear_flush() is irrelavent, I think?

Quoted 69 lines

Quoted 33 lines
Quoted 33 lines
      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); -    mm_inc_nr_ptes(vma->vm_mm);
+    if (is_pmd_none)
+        mm_inc_nr_ptes(vma->vm_mm);
}
static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
mm/huge_memory.c +18 -0
@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
       spin_unlock(vmf->ptl);
   }
   +static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+                         unsigned long haddr,
+                         struct folio *folio)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    vm_fault_t ret = 0;
+
+    ret = check_stable_address_space(vma->vm_mm);
+    if (ret)
+        goto out;
+    map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+    return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long
 haddr)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
Quoted 8 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1611
Quoted 6 lines
mm/huge_memory.c +6 -0
+    struct mmu_notifier_range range;
+    struct folio *folio = NULL;
+    vm_fault_t ret = 0;
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
Just checking: the PTE table was already allocated during the read fault, right? So we don't have to allocate it here.
Correct, that happens in set_huge_zero_folio(). Thanks for checking.
Quoted 17 lines
Quoted 6 lines
mm/huge_memory.c +5 -0
+    if (ret)
+        goto out;
+
+    mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, haddr,
+                haddr + HPAGE_PMD_SIZE);
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1626
Quoted 10 lines
mm/huge_memory.c +10 -0
+    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 unlock;
+    ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+    if (!ret)
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+    spin_unlock(vmf->ptl);
+    mmu_notifier_invalidate_range_end(&range);
I'll confess I don't understand all the mmu notifier rules.
I confess the same :)
Quoted 4 lines
But the doc at
Documentation/mm/mmu_notifier.rst implies that the notification must be done while holding the PTL. Although that's not how wp_page_copy(). Are you confident what you have done is correct?

Quoted 4 lines

Everywhere else, invalidate_range_end() is getting called after dropping the lock, one reason is that it has a might_sleep(), and therefore we cannot call it while holding a spinlock. I still don't know what exactly these calls mean...but I think what I have done is correct.

High level; they are notifying secondary MMUs (e.g. IOMMU) of a change so the tables of those secondary MMUs can be kept in sync. I don't understand all the ordering requirement details though.

I think what you have is probably correct, would be good for someone that knows what they are talking about to confirm though :)

Quoted 44 lines

Quoted 44 lines
Thanks,
Ryan
Quoted 39 lines
mm/huge_memory.c +4 -0
+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
@@ -1588,8 +1640,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, haddr);
+
+        if (!(ret & VM_FAULT_FALLBACK))
+            return ret;
+
+        /* Fallback to splitting PMD if THP cannot be allocated */
           goto fallback;
+    }
         spin_lock(vmf->ptl);
diff --git a/mm/memory.c b/mm/memory.c
index 3c01d68065be..c081a25f5173 100644
mm/memory.c +0 -0
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault
 *vmf)
       if (vma_is_anonymous(vma)) {
          if (likely(!unshare) && userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) { -            if (userfaultfd_wp_async(vmf->vma))
+            if (!userfaultfd_wp_async(vmf->vma)) +                return handle_userfault(vmf, VM_UFFD_WP); +            if (!is_huge_zero_pmd(vmf->orig_pmd)) goto split;
-            return handle_userfault(vmf, VM_UFFD_WP); }
          return do_huge_pmd_wp_page(vmf);
      }

Dev Jain

author reviewer

2024-09-06 09:00 UTC #17 lore raw

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

Quoted 93 lines · Ryan Roberts

Quoted 84 lines · Dev Jain
Quoted 14 lines · Ryan Roberts
Quoted 13 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
   include/linux/huge_mm.h |  6 ++++
   mm/huge_memory.c        | 79 +++++++++++++++++++++++++++++++++++------
   mm/memory.c             |  5 +--
   3 files changed, 78 insertions(+), 12 deletions(-)
Quoted 15 lines
include/linux/huge_mm.h line 16
Quoted 13 lines
include/linux/huge_mm.h +6 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
   #include <linux/kobject.h>
     vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable);
I don't think you are using either of these outside of huge_memory.c, so not sure you need to declare them here or make them non-static?
As pointed out by Kirill, you are right, I forgot to drop these from my previous approach.
Quoted 47 lines
Quoted 35 lines
include/linux/huge_mm.h +0 -0
   int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
             pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
             struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);
mm/huge_memory.c +3 -2
index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp,
 unsigned long addr,
   }
   EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct
 vm_area_struct *vma,
-                  unsigned long haddr, struct folio **foliop,
-                  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr)
   {
       struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true);
   @@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct
vm_area_struct *vma, int order)
      count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
}
-static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
-            struct vm_area_struct *vma, unsigned long haddr, -            pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf, +         struct vm_area_struct *vma, unsigned long haddr, +         pgtable_t pgtable)
{
-    pmd_t entry;
+    pmd_t entry, old_pmd;
+    bool is_pmd_none = pmd_none(*vmf->pmd);
      entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
Quoted 8 lines
      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); +    if (!is_pmd_none) {
+        old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd); +        if (pmd_uffd_wp(old_pmd))
+            entry = pmd_mkuffd_wp(entry);
I don't really get this; entry is writable, so I wouldn't expect to also be setting uffd-wp here? That combination is not allowed and is checked for in page_table_check_pte_flags().
Quoted 4 lines
It looks like you expect to get here in the uffd-wp-async case, which used to cause the pmd to be split to ptes. I'm guessing (but don't know for sure) that would cause the uffd-wp bit to be set in each of the new ptes, then during fallback to handling the wp fault on the pte, uffd would handle it?
Quoted 8 lines
I guess you are correct; I missed the WARN_ON() in page_table_check_pmd_flags(), but I did see, if I read the uffd code correctly, that mfill_atomic() will just return in case of pmd_trans_huge(*dst_pmd) while doing a uffd_copy to the destination
location. So preserving pmd_uffd_wp is useless in case a THP is mapped, but I did not
know that in fact it is supposed to be an invalid combination. So, I will drop it, unless someone has some other objection.

Quoted 2 lines

So what's the correct way to handle uffd-wp-async in wp_huge_pmd()? Just split it? If so, you can revert your changes in memory.c.

I think so.

Quoted 15 lines

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
+    }
+    if (pgtable)
+        pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
Should this call be moved outside of here? It doesn't really feel like it belongs. Could it be called before calling map_pmd_thp() for the site that has a pgtable?
Quoted 8 lines
Every other place I checked, they are doing this: make the entry -> deposit pgtable ->
set_pmd_at(). I guess the general flow is to do the deposit based on the old pmd, before
setting the new one. Which implies: I should at least move this check before I call pmdp_huge_clear_flush(). And, since vmf->pmd and creating the new entry has no relation,
I am inclined to do what you are saying.

Quoted 3 lines

pgtable_trans_huge_deposit() is just adding the pgtable to a list so that if the THP needs to be split in future, then we have preallocated the pte pgtable so the operation can't fail.

Yes.

Quoted 4 lines

And enqueing it is just under the protection of the
PTL as far as I can tell. So I think the only ordering requirement is that you both set the pmd and deposit the pgtable under the lock (without dropping it in between). So you can deposit either before or after map_pmd_thp().

Yes I'll do that before.

Quoted 2 lines

And
pmdp_huge_clear_flush() is irrelavent, I think?

You mean, in this context? Everywhere, pgtable deposit uses the old pmd value to be replaced as its input, that is, it is called before set_pmd_at(). So calling pgtable deposit after clear_flush() will violate this ordering. I do not think this ordering is really required but I'd rather be safe :)

Quoted 68 lines

Quoted 60 lines
Quoted 32 lines
Quoted 32 lines
      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);
-    mm_inc_nr_ptes(vma->vm_mm);
+    if (is_pmd_none)
+        mm_inc_nr_ptes(vma->vm_mm);
}
static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
mm/huge_memory.c +18 -0
@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
       spin_unlock(vmf->ptl);
   }
   +static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+                         unsigned long haddr,
+                         struct folio *folio)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    vm_fault_t ret = 0;
+
+    ret = check_stable_address_space(vma->vm_mm);
+    if (ret)
+        goto out;
+    map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+    return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long
 haddr)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
Quoted 8 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1611
Quoted 6 lines
mm/huge_memory.c +6 -0
+    struct mmu_notifier_range range;
+    struct folio *folio = NULL;
+    vm_fault_t ret = 0;
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
Just checking: the PTE table was already allocated during the read fault, right? So we don't have to allocate it here.
Correct, that happens in set_huge_zero_folio(). Thanks for checking.
Quoted 8 lines
Quoted 8 lines
mm/huge_memory.c +8 -0
+    if (ret)
+        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)))
Quoted 8 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1626
Quoted 7 lines
mm/huge_memory.c +7 -0
+        goto unlock;
+    ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+    if (!ret)
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+    spin_unlock(vmf->ptl);
+    mmu_notifier_invalidate_range_end(&range);
I'll confess I don't understand all the mmu notifier rules.
I confess the same :)
Quoted 8 lines
Quoted 4 lines
But the doc at
Documentation/mm/mmu_notifier.rst implies that the notification must be done while holding the PTL. Although that's not how wp_page_copy(). Are you confident what you have done is correct?
Everywhere else, invalidate_range_end() is getting called after dropping the lock, one reason is that it has a might_sleep(), and therefore we cannot call it while holding a spinlock. I still don't know what exactly these calls mean...but I think what I have done is correct.

Quoted 6 lines

High level; they are notifying secondary MMUs (e.g. IOMMU) of a change so the tables of those secondary MMUs can be kept in sync. I don't understand all the ordering requirement details though.
I think what you have is probably correct, would be good for someone that knows what they are talking about to confirm though :)

Exactly.

Quoted 43 lines

Thanks,
Ryan

Quoted 39 lines

mm/huge_memory.c +12 -0

+out:
+    return ret;
+}
+
   vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
   {
       const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;

@@ -1588,8 +1640,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, haddr);
+
+        if (!(ret & VM_FAULT_FALLBACK))
+            return ret;
+
+        /* Fallback to splitting PMD if THP cannot be allocated */
           goto fallback;
+    }
         spin_lock(vmf->ptl);
   diff --git a/mm/memory.c b/mm/memory.c

index 3c01d68065be..c081a25f5173 100644

mm/memory.c +3 -2

@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault
 *vmf)
       if (vma_is_anonymous(vma)) {
           if (likely(!unshare) &&
               userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) {
-            if (userfaultfd_wp_async(vmf->vma))
+            if (!userfaultfd_wp_async(vmf->vma))
+                return handle_userfault(vmf, VM_UFFD_WP);
+            if (!is_huge_zero_pmd(vmf->orig_pmd))
                   goto split;
-            return handle_userfault(vmf, VM_UFFD_WP);
           }
           return do_huge_pmd_wp_page(vmf);

}

Ryan Roberts

reviewer

2024-09-06 09:31 UTC #18 lore raw

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

Quoted 131 lines · Dev Jain

Quoted 94 lines · Ryan Roberts
Quoted 84 lines · Dev Jain
Quoted 14 lines · Ryan Roberts
Quoted 13 lines · Dev Jain
Introduce do_huge_zero_wp_pmd() to handle wp-fault on a hugezeropage and replace it with a PMD-mapped THP. Change the helpers introduced in the previous patch to flush TLB entry corresponding to the hugezeropage, and preserve PMD uffd-wp marker. In case of failure, fallback to splitting the PMD.
Signed-off-by Dev Jain <[email protected]>
---
include/linux/huge_mm.h | 6 ++++
mm/huge_memory.c | 79 +++++++++++++++++++++++++++++++++++------ mm/memory.c | 5 +--
   3 files changed, 78 insertions(+), 12 deletions(-)
Quoted 15 lines
include/linux/huge_mm.h line 16
Quoted 13 lines
include/linux/huge_mm.h +6 -0
index e25d9ebfdf89..fdd2cf473a3c 100644
@@ -9,6 +9,12 @@
    #include <linux/kobject.h>
      vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf);
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
+               unsigned long haddr, struct folio **foliop,
+               unsigned long addr);
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable);
I don't think you are using either of these outside of huge_memory.c, so not sure you need to declare them here or make them non-static?
As pointed out by Kirill, you are right, I forgot to drop these from my previous approach.
Quoted 47 lines
Quoted 35 lines
include/linux/huge_mm.h +0 -0
    int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
              pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
              struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);
mm/huge_memory.c +0 -0
index 58125fbcc532..150163ad77d3 100644
@@ -943,9 +943,9 @@ unsigned long thp_get_unmapped_area(struct file *filp,
 unsigned long addr,
    }
    EXPORT_SYMBOL_GPL(thp_get_unmapped_area);
   -static vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma,
-                  unsigned long haddr, struct folio **foliop,
-                  unsigned long addr)
+vm_fault_t thp_fault_alloc(gfp_t gfp, int order, struct vm_area_struct *vma, +               unsigned long haddr, struct folio **foliop,
+               unsigned long addr)
   {
       struct folio *folio = vma_alloc_folio(gfp, order, vma, haddr, true); @@ -984,21 +984,29 @@ static void __thp_fault_success_stats(struct vm_area_struct *vma, int order)
       count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
   }
   -static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf, -            struct vm_area_struct *vma, unsigned long haddr,
-            pgtable_t pgtable)
+void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+         struct vm_area_struct *vma, unsigned long haddr,
+         pgtable_t pgtable)
   {
-    pmd_t entry;
+    pmd_t entry, old_pmd;
+    bool is_pmd_none = pmd_none(*vmf->pmd);
         entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
Quoted 8 lines
       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); +    if (!is_pmd_none) {
+        old_pmd = pmdp_huge_clear_flush(vma, haddr, vmf->pmd); +        if (pmd_uffd_wp(old_pmd))
+            entry = pmd_mkuffd_wp(entry);
I don't really get this; entry is writable, so I wouldn't expect to also be setting uffd-wp here? That combination is not allowed and is checked for in page_table_check_pte_flags().
Quoted 4 lines
It looks like you expect to get here in the uffd-wp-async case, which used to cause the pmd to be split to ptes. I'm guessing (but don't know for sure) that would cause the uffd-wp bit to be set in each of the new ptes, then during fallback to handling the wp fault on the pte, uffd would handle it?
Quoted 9 lines
I guess you are correct; I missed the WARN_ON() in page_table_check_pmd_flags(), but I did see, if I read the uffd code correctly, that mfill_atomic() will just return in case of pmd_trans_huge(*dst_pmd) while doing a uffd_copy to the destination
location. So preserving pmd_uffd_wp is useless in case a THP is mapped, but I did not
know that in fact it is supposed to be an invalid combination. So, I will drop it,
unless someone has some other objection.
Quoted 2 lines
So what's the correct way to handle uffd-wp-async in wp_huge_pmd()? Just split it? If so, you can revert your changes in memory.c.
I think so.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
+    }
+    if (pgtable)
+        pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
Should this call be moved outside of here? It doesn't really feel like it belongs. Could it be called before calling map_pmd_thp() for the site that has a
pgtable?
Quoted 9 lines
Every other place I checked, they are doing this: make the entry -> deposit pgtable ->
set_pmd_at(). I guess the general flow is to do the deposit based on the old pmd, before
setting the new one. Which implies: I should at least move this check before I call
pmdp_huge_clear_flush(). And, since vmf->pmd and creating the new entry has no relation,
I am inclined to do what you are saying.
Quoted 3 lines
pgtable_trans_huge_deposit() is just adding the pgtable to a list so that if the THP needs to be split in future, then we have preallocated the pte pgtable so the operation can't fail.
Yes.
Quoted 4 lines
And enqueing it is just under the protection of the
PTL as far as I can tell. So I think the only ordering requirement is that you both set the pmd and deposit the pgtable under the lock (without dropping it in between). So you can deposit either before or after map_pmd_thp().
Yes I'll do that before.

Quoted 7 lines

Quoted 2 lines
And
pmdp_huge_clear_flush() is irrelavent, I think?
You mean, in this context? Everywhere, pgtable deposit uses the old pmd value to be replaced as its input, that is, it is called before set_pmd_at(). So calling pgtable deposit after clear_flush() will violate this ordering. I do not think this ordering is really required but I'd rather be safe :)

The pmd pointer is just used to get the pmd table (the pointer points to an entry inside the table so its just a case of backwards aligning the pointer). The pointer is never dereferenced, so the value of the entry is irrelevant.

Quoted 125 lines

Quoted 73 lines
Quoted 67 lines
Quoted 34 lines
Quoted 34 lines
       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); -    mm_inc_nr_ptes(vma->vm_mm);
+    if (is_pmd_none)
+        mm_inc_nr_ptes(vma->vm_mm);
   }
   static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf)
mm/huge_memory.c +19 -0
@@ -1576,6 +1584,50 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
        spin_unlock(vmf->ptl);
    }
    +static vm_fault_t do_huge_zero_wp_pmd_locked(struct vm_fault *vmf,
+                         unsigned long haddr,
+                         struct folio *folio)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    vm_fault_t ret = 0;
+
+    ret = check_stable_address_space(vma->vm_mm);
+    if (ret)
+        goto out;
+    map_pmd_thp(folio, vmf, vma, haddr, NULL);
+out:
+    return ret;
+}
+
+static vm_fault_t do_huge_zero_wp_pmd(struct vm_fault *vmf, unsigned long
 haddr)
+{
+    struct vm_area_struct *vma = vmf->vma;
+    gfp_t gfp = vma_thp_gfp_mask(vma);
+    struct mmu_notifier_range range;
Quoted 8 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1611
Quoted 5 lines
mm/huge_memory.c +5 -0
+    struct folio *folio = NULL;
+    vm_fault_t ret = 0;
+
+    ret = thp_fault_alloc(gfp, HPAGE_PMD_ORDER, vma, haddr, &folio,
+                  vmf->address);
Just checking: the PTE table was already allocated during the read fault, right?
So we don't have to allocate it here.
Correct, that happens in set_huge_zero_folio(). Thanks for checking.
Quoted 8 lines
Quoted 8 lines
mm/huge_memory.c +8 -0
+    if (ret)
+        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)))
Quoted 8 lines
mm/huge_memory.c void huge_pmd_set_accessed(struct vm_fault *vmf) · line 1626
Quoted 7 lines
mm/huge_memory.c +7 -0
+        goto unlock;
+    ret = do_huge_zero_wp_pmd_locked(vmf, haddr, folio);
+    if (!ret)
+        __thp_fault_success_stats(vma, HPAGE_PMD_ORDER);
+unlock:
+    spin_unlock(vmf->ptl);
+    mmu_notifier_invalidate_range_end(&range);
I'll confess I don't understand all the mmu notifier rules.
I confess the same :)
Quoted 5 lines
But the doc at
Documentation/mm/mmu_notifier.rst implies that the notification must be done while holding the PTL. Although that's not how wp_page_copy(). Are you confident
what you have done is correct?
Quoted 6 lines
Everywhere else, invalidate_range_end() is getting called after dropping the lock,
one reason is that it has a might_sleep(), and therefore we cannot call it while holding a spinlock. I still don't know what exactly these calls mean...but I think
what I have done is correct.
Quoted 6 lines
High level; they are notifying secondary MMUs (e.g. IOMMU) of a change so the tables of those secondary MMUs can be kept in sync. I don't understand all the ordering requirement details though.
I think what you have is probably correct, would be good for someone that knows what they are talking about to confirm though :)
Exactly.
Quoted 43 lines
Quoted 43 lines
Quoted 43 lines
Thanks,
Ryan
Quoted 39 lines
mm/huge_memory.c +4 -0
+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
@@ -1588,8 +1640,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, haddr);
+
+        if (!(ret & VM_FAULT_FALLBACK))
+            return ret;
+
+        /* Fallback to splitting PMD if THP cannot be allocated */
            goto fallback;
+    }
          spin_lock(vmf->ptl);
diff --git a/mm/memory.c b/mm/memory.c
index 3c01d68065be..c081a25f5173 100644
mm/memory.c +0 -0
@@ -5409,9 +5409,10 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault
 *vmf)
        if (vma_is_anonymous(vma)) {
           if (likely(!unshare) && userfaultfd_huge_pmd_wp(vma, vmf->orig_pmd)) { -            if (userfaultfd_wp_async(vmf->vma))
+            if (!userfaultfd_wp_async(vmf->vma))
+                return handle_userfault(vmf, VM_UFFD_WP); +            if (!is_huge_zero_pmd(vmf->orig_pmd)) goto split;
-            return handle_userfault(vmf, VM_UFFD_WP); }
           return do_huge_pmd_wp_page(vmf);
       }

Ryan Roberts

reviewer

2024-09-04 11:36 UTC #19 lore raw

In reply to Dev Jain

Hi Dev,

Quoted 21 lines · Dev Jain

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.
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
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/

Quoted 9 lines

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

 include/linux/huge_mm.h |   6 ++
 mm/huge_memory.c        | 171 +++++++++++++++++++++++++++++-----------
 mm/memory.c             |   5 +-
 3 files changed, 136 insertions(+), 46 deletions(-)

What is the base for this? It doesn't apply on top of mm-unstable.

Thanks,
Ryan

Dev Jain

author reviewer

2024-09-04 15:42 UTC #20 lore raw

Quoted 28 lines · Ryan Roberts

Hi Dev,
Quoted 25 lines · Dev Jain
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.
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
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
Dev Jain (2):
mm: Abstract THP allocation
mm: Allocate THP on hugezeropage wp-fault

Quoted 6 lines

Quoted 5 lines

  include/linux/huge_mm.h |   6 ++
  mm/huge_memory.c        | 171 +++++++++++++++++++++++++++++-----------
  mm/memory.c             |   5 +-
  3 files changed, 136 insertions(+), 46 deletions(-)

What is the base for this? It doesn't apply on top of mm-unstable.

Sorry, forgot to mention, it applies on v6.11-rc5.

Quoted 4 lines

Thanks,
Ryan

Ryan Roberts

reviewer

2024-09-04 16:01 UTC #21 lore raw

Quoted 30 lines · Dev Jain

Quoted 28 lines · Ryan Roberts
Hi Dev,
Quoted 25 lines · Dev Jain
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.
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
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
Dev Jain (2):
mm: Abstract THP allocation
mm: Allocate THP on hugezeropage wp-fault

Quoted 8 lines

Quoted 6 lines
Quoted 5 lines
include/linux/huge_mm.h | 6 ++
mm/huge_memory.c | 171 +++++++++++++++++++++++++++++----------- mm/memory.c | 5 +-
  3 files changed, 136 insertions(+), 46 deletions(-)
What is the base for this? It doesn't apply on top of mm-unstable.
Sorry, forgot to mention, it applies on v6.11-rc5.

Thanks, I'll give it a review tomorrow. Although I suspect that Andrew will want it based against mm-unstable once it gets into shape for merging.

Quoted 5 lines

Quoted 5 lines
Thanks,
Ryan