mm: userspace hugepage collapse

When scanning an anon pmd to see if it's eligible for collapse, return SCAN_PMD_MAPPED if the pmd already maps a THP. Note that SCAN_PMD_MAPPED is different from SCAN_PAGE_COMPOUND used in the file-collapse path, since the latter might identify pte-mapped compound pages. This is required by MADV_COLLAPSE which necessarily needs to know what hugepage-aligned/sized regions are already pmd-mapped.

Signed-off-by Zach O'Keefe <[email protected]>

Reported-by kernel test robot <[email protected]>

---
 include/trace/events/huge_memory.h |  3 ++-
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 30 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 +++++++++++++--
 4 files changed, 42 insertions(+), 7 deletions(-)

include/trace/events/huge_memory.h +0 -0

index d651f3437367..9faa678e0a5b 100644

include/trace/events/huge_memory.h +0 -0

 #undef EM
 #undef EMe

mm/internal.h +1 -0

index 0667abd57634..51ae9f71a2a3 100644
@@ -172,6 +172,7 @@ extern void reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason
 /*
  * in mm/rmap.c:
  */
+pmd_t *mm_find_pmd_raw(struct mm_struct *mm, unsigned long address);
 extern pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address);
 
 /*

mm/khugepaged.c +8 -0

index ba8dbd1825da..2933b13fc975 100644
@@ -51,6 +51,7 @@ enum scan_result {
 	SCAN_CGROUP_CHARGE_FAIL,
 	SCAN_TRUNCATED,
 	SCAN_PAGE_HAS_PRIVATE,
+	SCAN_PMD_MAPPED,
 };
 
 #define CREATE_TRACE_POINTS

@@ -987,6 +988,29 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
 	return 0;
 }
 
+static int find_pmd_or_thp_or_none(struct mm_struct *mm,
+				   unsigned long address,
+				   pmd_t **pmd)
+{
+	pmd_t pmde;
+
+	*pmd = mm_find_pmd_raw(mm, address);

mm/khugepaged.c +8 -4

+		return SCAN_PMD_NULL;
+	if (pmd_trans_huge(pmde))
+		return SCAN_PMD_MAPPED;
+	return SCAN_SUCCEED;
+}
+
 /*
  * Bring missing pages in from swap, to complete THP collapse.
  * Only done if khugepaged_scan_pmd believes it is worthwhile.

@@ -1238,11 +1262,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-	pmd = mm_find_pmd(mm, address);
-	if (!pmd) {
-		result = SCAN_PMD_NULL;
+	result = find_pmd_or_thp_or_none(mm, address, &pmd);
+	if (result != SCAN_SUCCEED)
 		goto out;
-	}
 
 	memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
 	pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +13 -2

index 61e63db5dc6f..49817f35e65c 100644
@@ -759,13 +759,12 @@ unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 	return vma_address(page, vma);
 }
 
-pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
+pmd_t *mm_find_pmd_raw(struct mm_struct *mm, unsigned long address)
 {
 	pgd_t *pgd;
 	p4d_t *p4d;
 	pud_t *pud;
 	pmd_t *pmd = NULL;
-	pmd_t pmde;
 
 	pgd = pgd_offset(mm, address);
 	if (!pgd_present(*pgd))

@@ -780,6 +779,18 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
 		goto out;
 
 	pmd = pmd_offset(pud, address);
+out:
+	return pmd;
+}
+
+pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
+{
+	pmd_t pmde;
+	pmd_t *pmd;
+
+	pmd = mm_find_pmd_raw(mm, address);
+	if (!pmd)
+		goto out;
 	/*
 	 * Some THP functions use the sequence pmdp_huge_clear_flush(), set_pmd_at()
 	 * without holding anon_vma lock for write.  So when looking for a

Signature · 1 line

2.36.0.rc2.479.g8af0fa9b8e-goog

Hi, Zach,

Quoted 7 lines · Zach O'Keefe

When scanning an anon pmd to see if it's eligible for collapse, return SCAN_PMD_MAPPED if the pmd already maps a THP. Note that SCAN_PMD_MAPPED is different from SCAN_PAGE_COMPOUND used in the file-collapse path, since the latter might identify pte-mapped compound pages. This is required by MADV_COLLAPSE which necessarily needs to know what hugepage-aligned/sized regions are already pmd-mapped.

Quoted 2 lines

Signed-off-by Zach O'Keefe <[email protected]>

Reported-by kernel test robot <[email protected]>

IIUC we don't need to attach this reported-by if this is not a bugfix. I think you can simply fix all issues reported by the test bot and only attach the line if the patch is fixing the problem that the bot was reporting explicitly.

Quoted 12 lines

---
 include/trace/events/huge_memory.h |  3 ++-
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 30 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 +++++++++++++--
 4 files changed, 42 insertions(+), 7 deletions(-)

include/trace/events/huge_memory.h +0 -0

index d651f3437367..9faa678e0a5b 100644

include/trace/events/huge_memory.h line 37

Quoted 6 lines

include/trace/events/huge_memory.h +2 -1

@@ -33,7 +33,8 @@
      EM( SCAN_ALLOC_HUGE_PAGE_FAIL,  "alloc_huge_page_failed")       \
      EM( SCAN_CGROUP_CHARGE_FAIL,    "ccgroup_charge_failed")        \
      EM( SCAN_TRUNCATED,             "truncated")                    \
-     EMe(SCAN_PAGE_HAS_PRIVATE,      "page_has_private")             \
+     EM( SCAN_PAGE_HAS_PRIVATE,      "page_has_private")             \
+     EMe(SCAN_PMD_MAPPED,            "page_pmd_mapped")              \

Nit: IMHO it can be put even in the middle so we don't need to touch the EMe() every time. :)

Apart from that, it does sound proper to me to put SCAN_PMD_MAPPED to be right after SCAN_PMD_NULL anyway.

Quoted 43 lines

include/trace/events/huge_memory.h +0 -0

 #undef EM
 #undef EMe

mm/internal.h +1 -0

index 0667abd57634..51ae9f71a2a3 100644
@@ -172,6 +172,7 @@ extern void reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason
 /*
  * in mm/rmap.c:
  */
+pmd_t *mm_find_pmd_raw(struct mm_struct *mm, unsigned long address);
 extern pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address);
 
 /*

mm/khugepaged.c +13 -0

index ba8dbd1825da..2933b13fc975 100644
@@ -51,6 +51,7 @@ enum scan_result {
      SCAN_CGROUP_CHARGE_FAIL,
      SCAN_TRUNCATED,
      SCAN_PAGE_HAS_PRIVATE,
+     SCAN_PMD_MAPPED,
 };
 
 #define CREATE_TRACE_POINTS

@@ -987,6 +988,29 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
      return 0;
 }
 
+static int find_pmd_or_thp_or_none(struct mm_struct *mm,
+                                unsigned long address,
+                                pmd_t **pmd)
+{
+     pmd_t pmde;
+
+     *pmd = mm_find_pmd_raw(mm, address);
+     if (!*pmd)
+             return SCAN_PMD_NULL;
+
+     pmde = pmd_read_atomic(*pmd);
+

mm/khugepaged.c static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address, · line 1007

Quoted 5 lines

mm/khugepaged.c +5 -0

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+     /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
+     barrier();
+#endif
+     if (!pmd_present(pmde) || pmd_none(pmde))

Could we drop the pmd_none() check? I assume !pmd_present() should have covered that case already?

Quoted 62 lines

mm/khugepaged.c +8 -4

+             return SCAN_PMD_NULL;
+     if (pmd_trans_huge(pmde))
+             return SCAN_PMD_MAPPED;
+     return SCAN_SUCCEED;
+}
+
 /*
  * Bring missing pages in from swap, to complete THP collapse.
  * Only done if khugepaged_scan_pmd believes it is worthwhile.

@@ -1238,11 +1262,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
      VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-     pmd = mm_find_pmd(mm, address);
-     if (!pmd) {
-             result = SCAN_PMD_NULL;
+     result = find_pmd_or_thp_or_none(mm, address, &pmd);
+     if (result != SCAN_SUCCEED)
              goto out;
-     }
 
      memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
      pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +13 -3

index 61e63db5dc6f..49817f35e65c 100644
@@ -759,13 +759,12 @@ unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
      return vma_address(page, vma);
 }
 
-pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
+pmd_t *mm_find_pmd_raw(struct mm_struct *mm, unsigned long address)
 {
      pgd_t *pgd;
      p4d_t *p4d;
      pud_t *pud;
      pmd_t *pmd = NULL;
-     pmd_t pmde;
 
      pgd = pgd_offset(mm, address);
      if (!pgd_present(*pgd))

@@ -780,6 +779,18 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
              goto out;
 
      pmd = pmd_offset(pud, address);
+out:
+     return pmd;
+}
+
+pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
+{
+     pmd_t pmde;
+     pmd_t *pmd;
+
+     pmd = mm_find_pmd_raw(mm, address);
+     if (!pmd)
+             goto out;
      /*
       * Some THP functions use the sequence pmdp_huge_clear_flush(), set_pmd_at()
       * without holding anon_vma lock for write.  So when looking for a
--

Quoted 2 lines

2.36.0.rc2.479.g8af0fa9b8e-goog

--
Peter Xu

Modularize hugepage collapse by introducing struct collapse_control. This structure serves to describe the properties of the requested collapse, as well as serve as a local scratch pad to use during the collapse itself.

Signed-off-by Zach O'Keefe <[email protected]>

---
 mm/khugepaged.c | 79 ++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 33 deletions(-)

mm/khugepaged.c +0 -0

index 2933b13fc975..9d42fa330812 100644

mm/khugepaged.c +2 -0

+	int last_target_node;
+};

mm/khugepaged.c +3 -6

@@ -829,28 +835,28 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
 }
 
 #ifdef CONFIG_NUMA
-static int khugepaged_find_target_node(void)
+static int khugepaged_find_target_node(struct collapse_control *cc)
 {
-	static int last_khugepaged_target_node = NUMA_NO_NODE;
 	int nid, target_node = 0, max_value = 0;
 
 	/* find first node with max normal pages hit */
 	for (nid = 0; nid < MAX_NUMNODES; nid++)
-		if (khugepaged_node_load[nid] > max_value) {
-			max_value = khugepaged_node_load[nid];
+		if (cc->node_load[nid] > max_value) {
+			max_value = cc->node_load[nid];
 			target_node = nid;
 		}
 
 	/* do some balance if several nodes have the same hit record */
-	if (target_node <= last_khugepaged_target_node)
-		for (nid = last_khugepaged_target_node + 1; nid < MAX_NUMNODES;

mm/khugepaged.c +1 -1

-	last_khugepaged_target_node = target_node;
+	cc->last_target_node = target_node;
 	return target_node;
 }

Quoted 4 lines · Zach O'Keefe

Modularize hugepage collapse by introducing struct collapse_control. This structure serves to describe the properties of the requested collapse, as well as serve as a local scratch pad to use during the collapse itself.

Reasonable, but IMHO worth mentioning that "we're introducing the context by first moving the per-node statistics into ...", or something like that.

Quoted 14 lines

Signed-off-by Zach O'Keefe <[email protected]>

---
 mm/khugepaged.c | 79 ++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 33 deletions(-)

mm/khugepaged.c +0 -0

index 2933b13fc975..9d42fa330812 100644
@@ -86,6 +86,14 @@ static struct kmem_cache *mm_slot_cache __read_mostly;
 
 #define MAX_PTE_MAPPED_THP 8
 

mm/khugepaged.c static struct kmem_cache *mm_slot_cache __read_mostly; · line 93

Quoted 5 lines

mm/khugepaged.c +5 -0

+struct collapse_control {
+     /* Num pages scanned per node */
+     int node_load[MAX_NUMNODES];
+
+     /* Last target selected in khugepaged_find_target_node() for this scan */

Not really important, but.. iiuc this is not only for this scan but for all scans of the khugepaged thread.

Quoted 2 lines

mm/khugepaged.c +2 -0

+     int last_target_node;
+};

[...]

Quoted 28 lines

mm/khugepaged.c +7 -8

@@ -829,28 +835,28 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
 }
 
 #ifdef CONFIG_NUMA
-static int khugepaged_find_target_node(void)
+static int khugepaged_find_target_node(struct collapse_control *cc)
 {
-     static int last_khugepaged_target_node = NUMA_NO_NODE;
      int nid, target_node = 0, max_value = 0;
 
      /* find first node with max normal pages hit */
      for (nid = 0; nid < MAX_NUMNODES; nid++)
-             if (khugepaged_node_load[nid] > max_value) {
-                     max_value = khugepaged_node_load[nid];
+             if (cc->node_load[nid] > max_value) {
+                     max_value = cc->node_load[nid];
                      target_node = nid;
              }
 
      /* do some balance if several nodes have the same hit record */
-     if (target_node <= last_khugepaged_target_node)
-             for (nid = last_khugepaged_target_node + 1; nid < MAX_NUMNODES;
-                             nid++)
-                     if (max_value == khugepaged_node_load[nid]) {
+     if (target_node <= cc->last_target_node)
+             for (nid = cc->last_target_node + 1; nid < MAX_NUMNODES;
+                  nid++) {
+                     if (max_value == cc->node_load[nid]) {

mm/khugepaged.c static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void) · line 857

Quoted 4 lines

mm/khugepaged.c +1 -0

                              target_node = nid;
                              break;
                      }
+             }

I'm not sure what's the coding style for this case, but IIUC we could either add both (to both outer "if" and "for") or none; adding one pair of brackets seems a bit odd.

mm/khugepaged.c static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void) · line 861

Quoted 5 lines

mm/khugepaged.c +1 -1

-     last_khugepaged_target_node = target_node;
+     cc->last_target_node = target_node;
      return target_node;
 }

--
Peter Xu

Add hugepage allocation context to struct collapse_context, allowing different collapse contexts to allocate hugepages differently. For example, khugepaged decides to allocate differently in NUMA and UMA configurations, and other collapse contexts shouldn't be coupled to this decision. Likewise for the gfp flags used for said allocation.

Additionally, move [pre]allocated hugepage pointer into struct collapse_context.

Signed-off-by Zach O'Keefe <[email protected]>

Reported-by kernel test robot <[email protected]>

---
 mm/khugepaged.c | 102 ++++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

mm/khugepaged.c +0 -0

index 9d42fa330812..c4962191d6e1 100644

mm/khugepaged.c +15 -15

 {
-	VM_BUG_ON_PAGE(*hpage, *hpage);
+	VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
 
-	*hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
-	if (unlikely(!*hpage)) {
+	cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
+	if (unlikely(!cc->hpage)) {
 		count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
-		*hpage = ERR_PTR(-ENOMEM);
+		cc->hpage = ERR_PTR(-ENOMEM);
 		return NULL;
 	}
 
-	prep_transhuge_page(*hpage);
+	prep_transhuge_page(cc->hpage);
 	count_vm_event(THP_COLLAPSE_ALLOC);
-	return *hpage;
+	return cc->hpage;
 }
 #else
 static int khugepaged_find_target_node(struct collapse_control *cc)

@@ -953,12 +958,12 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
 	return true;
 }
 
-static struct page *
-khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
+static struct page *khugepaged_alloc_page(struct collapse_control *cc,
+					  gfp_t gfp, int node)
 {
-	VM_BUG_ON(!*hpage);
+	VM_BUG_ON(!cc->hpage);
 
-	return  *hpage;
+	return cc->hpage;
 }
 #endif
 

@@ -1080,10 +1085,9 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
 	return true;
 }
 
-static void collapse_huge_page(struct mm_struct *mm,
-				   unsigned long address,
-				   struct page **hpage,
-				   int node, int referenced, int unmapped)
+static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
+			       struct collapse_control *cc, int referenced,
+			       int unmapped)
 {
 	LIST_HEAD(compound_pagelist);
 	pmd_t *pmd, _pmd;

@@ -1096,11 +1100,12 @@ static void collapse_huge_page(struct mm_struct *mm,
 	struct mmu_notifier_range range;
 	gfp_t gfp;
 	const struct cpumask *cpumask;
+	int node;
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
 	/* Only allocate from the target node */
-	gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+	gfp = cc->gfp() | __GFP_THISNODE;
 
 	/*
 	 * Before allocating the hugepage, release the mmap_lock read lock.

@@ -1110,13 +1115,14 @@ static void collapse_huge_page(struct mm_struct *mm,
 	 */
 	mmap_read_unlock(mm);

mm/khugepaged.c +26 -32

 	if (!new_page) {
 		result = SCAN_ALLOC_HUGE_PAGE_FAIL;
 		goto out_nolock;

@@ -1238,15 +1244,15 @@ static void collapse_huge_page(struct mm_struct *mm,
 	update_mmu_cache_pmd(vma, address, pmd);
 	spin_unlock(pmd_ptl);
 
-	*hpage = NULL;
+	cc->hpage = NULL;
 
 	khugepaged_pages_collapsed++;
 	result = SCAN_SUCCEED;
 out_up_write:
 	mmap_write_unlock(mm);
 out_nolock:
-	if (!IS_ERR_OR_NULL(*hpage))
-		mem_cgroup_uncharge(page_folio(*hpage));
+	if (!IS_ERR_OR_NULL(cc->hpage))
+		mem_cgroup_uncharge(page_folio(cc->hpage));
 	trace_mm_collapse_huge_page(mm, isolated, result);
 	return;
 }

@@ -1254,7 +1260,6 @@ static void collapse_huge_page(struct mm_struct *mm,
 static int khugepaged_scan_pmd(struct mm_struct *mm,
 			       struct vm_area_struct *vma,
 			       unsigned long address,
-			       struct page **hpage,
 			       struct collapse_control *cc)
 {
 	pmd_t *pmd;

@@ -1399,10 +1404,8 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 out_unmap:
 	pte_unmap_unlock(pte, ptl);
 	if (ret) {
-		node = khugepaged_find_target_node(cc);
 		/* collapse_huge_page will return with the mmap_lock released */
-		collapse_huge_page(mm, address, hpage, node,
-				referenced, unmapped);
+		collapse_huge_page(mm, address, cc, referenced, unmapped);
 	}
 out:
 	trace_mm_khugepaged_scan_pmd(mm, page, writable, referenced,

@@ -1667,8 +1670,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  * @mm: process address space where collapse happens
  * @file: file that collapse on
  * @start: collapse start address
- * @hpage: new allocated huge page for collapse
- * @node: appointed node the new huge page allocate from
+ * @cc: collapse context and scratchpad
  *
  * Basic scheme is simple, details are more complex:
  *  - allocate and lock a new huge page;

@@ -1686,8 +1688,8 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  *    + unlock and free huge page;
  */
 static void collapse_file(struct mm_struct *mm,
-		struct file *file, pgoff_t start,
-		struct page **hpage, int node)
+			  struct file *file, pgoff_t start,
+			  struct collapse_control *cc)
 {
 	struct address_space *mapping = file->f_mapping;
 	gfp_t gfp;

@@ -1697,15 +1699,16 @@ static void collapse_file(struct mm_struct *mm,
 	XA_STATE_ORDER(xas, &mapping->i_pages, start, HPAGE_PMD_ORDER);
 	int nr_none = 0, result = SCAN_SUCCEED;
 	bool is_shmem = shmem_file(file);
-	int nr;
+	int nr, node;
 
 	VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
 	VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
 
 	/* Only allocate from the target node */
-	gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+	gfp = cc->gfp() | __GFP_THISNODE;
+	node = khugepaged_find_target_node(cc);
 
-	new_page = khugepaged_alloc_page(hpage, gfp, node);
+	new_page = cc->alloc_hpage(cc, gfp, node);
 	if (!new_page) {
 		result = SCAN_ALLOC_HUGE_PAGE_FAIL;
 		goto out;

@@ -1998,7 +2001,7 @@ static void collapse_file(struct mm_struct *mm,
 		 * Remove pte page tables, so we can re-fault the page as huge.
 		 */
 		retract_page_tables(mapping, start);
-		*hpage = NULL;
+		cc->hpage = NULL;
 
 		khugepaged_pages_collapsed++;
 	} else {

@@ -2045,14 +2048,14 @@ static void collapse_file(struct mm_struct *mm,
 	unlock_page(new_page);
 out:
 	VM_BUG_ON(!list_empty(&pagelist));
-	if (!IS_ERR_OR_NULL(*hpage))
-		mem_cgroup_uncharge(page_folio(*hpage));
+	if (!IS_ERR_OR_NULL(cc->hpage))
+		mem_cgroup_uncharge(page_folio(cc->hpage));
 	/* TODO: tracepoints */
 }
 
 static void khugepaged_scan_file(struct mm_struct *mm,
-		struct file *file, pgoff_t start, struct page **hpage,
-		struct collapse_control *cc)
+				 struct file *file, pgoff_t start,
+				 struct collapse_control *cc)
 {
 	struct page *page = NULL;
 	struct address_space *mapping = file->f_mapping;

@@ -2125,8 +2128,7 @@ static void khugepaged_scan_file(struct mm_struct *mm,
 			result = SCAN_EXCEED_NONE_PTE;
 			count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
 		} else {
-			node = khugepaged_find_target_node(cc);
-			collapse_file(mm, file, start, hpage, node);
+			collapse_file(mm, file, start, cc);
 		}
 	}
 

@@ -2134,8 +2136,8 @@ static void khugepaged_scan_file(struct mm_struct *mm,
 }
 #else
 static void khugepaged_scan_file(struct mm_struct *mm,
-		struct file *file, pgoff_t start, struct page **hpage,
-		struct collapse_control *cc)
+				 struct file *file, pgoff_t start,
+				 struct collapse_control *cc)
 {
 	BUILD_BUG();
 }

@@ -2146,7 +2148,6 @@ static void khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
 #endif
 
 static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
-					    struct page **hpage,
 					    struct collapse_control *cc)
 	__releases(&khugepaged_mm_lock)
 	__acquires(&khugepaged_mm_lock)

@@ -2223,12 +2224,11 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
 
 				mmap_read_unlock(mm);
 				ret = 1;
-				khugepaged_scan_file(mm, file, pgoff, hpage, cc);
+				khugepaged_scan_file(mm, file, pgoff, cc);
 				fput(file);
 			} else {
 				ret = khugepaged_scan_pmd(mm, vma,
-						khugepaged_scan.address,
-						hpage, cc);
+						khugepaged_scan.address, cc);
 			}
 			/* move to next address */
 			khugepaged_scan.address += HPAGE_PMD_SIZE;

@@ -2286,15 +2286,15 @@ static int khugepaged_wait_event(void)
 
 static void khugepaged_do_scan(struct collapse_control *cc)
 {
-	struct page *hpage = NULL;
 	unsigned int progress = 0, pass_through_head = 0;
 	unsigned int pages = READ_ONCE(khugepaged_pages_to_scan);
 	bool wait = true;
 
+	cc->hpage = NULL;
 	lru_add_drain_all();
 
 	while (progress < pages) {
-		if (!khugepaged_prealloc_page(&hpage, &wait))
+		if (!khugepaged_prealloc_page(&cc->hpage, &wait))
 			break;
 
 		cond_resched();

@@ -2308,14 +2308,14 @@ static void khugepaged_do_scan(struct collapse_control *cc)
 		if (khugepaged_has_work() &&
 		    pass_through_head < 2)
 			progress += khugepaged_scan_mm_slot(pages - progress,
-							    &hpage, cc);
+							    cc);
 		else
 			progress = pages;
 		spin_unlock(&khugepaged_mm_lock);
 	}
 
-	if (!IS_ERR_OR_NULL(hpage))
-		put_page(hpage);
+	if (!IS_ERR_OR_NULL(cc->hpage))
+		put_page(cc->hpage);
 }
 
 static bool khugepaged_should_wakeup(void)

Quoted 6 lines · Zach O'Keefe

Add hugepage allocation context to struct collapse_context, allowing different collapse contexts to allocate hugepages differently. For example, khugepaged decides to allocate differently in NUMA and UMA configurations, and other collapse contexts shouldn't be coupled to this decision. Likewise for the gfp flags used for said allocation.

Quoted 5 lines

Additionally, move [pre]allocated hugepage pointer into struct collapse_context.

Signed-off-by Zach O'Keefe <[email protected]>

Reported-by kernel test robot <[email protected]>

(Please remember to remove this one too, and the rest ones.. :)

Quoted 10 lines

---
 mm/khugepaged.c | 102 ++++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

mm/khugepaged.c +0 -0

index 9d42fa330812..c4962191d6e1 100644
@@ -92,6 +92,11 @@ struct collapse_control {
 

mm/khugepaged.c struct collapse_control { · line 99

Quoted 7 lines

mm/khugepaged.c +5 -0

      /* Last target selected in khugepaged_find_target_node() for this scan */
      int last_target_node;
+
+     struct page *hpage;
+     gfp_t (*gfp)(void);
+     struct page* (*alloc_hpage)(struct collapse_control *cc, gfp_t gfp,
+                                 int node);

Nit: s/page* /page */ looks a bit more consistent..

mm/khugepaged.c static bool khugepaged_prealloc_page(struct page **hpage, bool *wait) · line 886

Quoted 11 lines

mm/khugepaged.c +2 -2

 };
 
 /**

@@ -877,21 +882,21 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
      return true;
 }
 
-static struct page *
-khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
+static struct page *khugepaged_alloc_page(struct collapse_control *cc,
+                                       gfp_t gfp, int node)

Nit: I'd suggest we put collapse_control* either at the start or at the end, and keep doing it when possible.

IIRC you used to put it always at the end, but now it's at the start. Not a big deal but it'll be nice to keep the code self-aligned.

Quoted 71 lines

mm/khugepaged.c +15 -15

 {
-     VM_BUG_ON_PAGE(*hpage, *hpage);
+     VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
 
-     *hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
-     if (unlikely(!*hpage)) {
+     cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
+     if (unlikely(!cc->hpage)) {
              count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
-             *hpage = ERR_PTR(-ENOMEM);
+             cc->hpage = ERR_PTR(-ENOMEM);
              return NULL;
      }
 
-     prep_transhuge_page(*hpage);
+     prep_transhuge_page(cc->hpage);
      count_vm_event(THP_COLLAPSE_ALLOC);
-     return *hpage;
+     return cc->hpage;
 }
 #else
 static int khugepaged_find_target_node(struct collapse_control *cc)

@@ -953,12 +958,12 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
      return true;
 }
 
-static struct page *
-khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
+static struct page *khugepaged_alloc_page(struct collapse_control *cc,
+                                       gfp_t gfp, int node)
 {
-     VM_BUG_ON(!*hpage);
+     VM_BUG_ON(!cc->hpage);
 
-     return  *hpage;
+     return cc->hpage;
 }
 #endif
 

@@ -1080,10 +1085,9 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
      return true;
 }
 
-static void collapse_huge_page(struct mm_struct *mm,
-                                unsigned long address,
-                                struct page **hpage,
-                                int node, int referenced, int unmapped)
+static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
+                            struct collapse_control *cc, int referenced,
+                            int unmapped)
 {
      LIST_HEAD(compound_pagelist);
      pmd_t *pmd, _pmd;

@@ -1096,11 +1100,12 @@ static void collapse_huge_page(struct mm_struct *mm,
      struct mmu_notifier_range range;
      gfp_t gfp;
      const struct cpumask *cpumask;
+     int node;
 
      VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
      /* Only allocate from the target node */
-     gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+     gfp = cc->gfp() | __GFP_THISNODE;
 
      /*
       * Before allocating the hugepage, release the mmap_lock read lock.

@@ -1110,13 +1115,14 @@ static void collapse_huge_page(struct mm_struct *mm,
       */
      mmap_read_unlock(mm);
 

mm/khugepaged.c static void collapse_huge_page(struct mm_struct *mm, · line 1125

Quoted 9 lines

mm/khugepaged.c +2 -1

+     node = khugepaged_find_target_node(cc);
      /* sched to specified node before huage page memory copy */
      if (task_node(current) != node) {
              cpumask = cpumask_of_node(node);
              if (!cpumask_empty(cpumask))
                      set_cpus_allowed_ptr(current, cpumask);
      }
-     new_page = khugepaged_alloc_page(hpage, gfp, node);
+     new_page = cc->alloc_hpage(cc, gfp, node);

AFAICT you removed all references of khugepaged_alloc_page() in this patch, then you'd better drop the function for both NUMA and UMA.

Said that, I think it's actually better to keep them, as they do things useful. For example,AFAICT this ->alloc_hpage() change can leak the hpage alloacted for UMA already so that's why I think keeping them makes sense, then iiuc the BUG_ON would trigger with UMA already.

I saw that you've moved khugepaged_find_target_node() into this function which looks definitely good, but if we could keep khugepaged_alloc_page() and then IMHO we could even move khugepaged_find_target_node() into that and drop the "node" parameter in khugepaged_alloc_page().

I'd even consider moving cc->gfp() all into it if possible, since the gfp seems to be always with __GFP_THISNODE anyways.

What do you think?

Quoted 190 lines

mm/khugepaged.c +26 -32

      if (!new_page) {
              result = SCAN_ALLOC_HUGE_PAGE_FAIL;
              goto out_nolock;

@@ -1238,15 +1244,15 @@ static void collapse_huge_page(struct mm_struct *mm,
      update_mmu_cache_pmd(vma, address, pmd);
      spin_unlock(pmd_ptl);
 
-     *hpage = NULL;
+     cc->hpage = NULL;
 
      khugepaged_pages_collapsed++;
      result = SCAN_SUCCEED;
 out_up_write:
      mmap_write_unlock(mm);
 out_nolock:
-     if (!IS_ERR_OR_NULL(*hpage))
-             mem_cgroup_uncharge(page_folio(*hpage));
+     if (!IS_ERR_OR_NULL(cc->hpage))
+             mem_cgroup_uncharge(page_folio(cc->hpage));
      trace_mm_collapse_huge_page(mm, isolated, result);
      return;
 }

@@ -1254,7 +1260,6 @@ static void collapse_huge_page(struct mm_struct *mm,
 static int khugepaged_scan_pmd(struct mm_struct *mm,
                             struct vm_area_struct *vma,
                             unsigned long address,
-                            struct page **hpage,
                             struct collapse_control *cc)
 {
      pmd_t *pmd;

@@ -1399,10 +1404,8 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 out_unmap:
      pte_unmap_unlock(pte, ptl);
      if (ret) {
-             node = khugepaged_find_target_node(cc);
              /* collapse_huge_page will return with the mmap_lock released */
-             collapse_huge_page(mm, address, hpage, node,
-                             referenced, unmapped);
+             collapse_huge_page(mm, address, cc, referenced, unmapped);
      }
 out:
      trace_mm_khugepaged_scan_pmd(mm, page, writable, referenced,

@@ -1667,8 +1670,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  * @mm: process address space where collapse happens
  * @file: file that collapse on
  * @start: collapse start address
- * @hpage: new allocated huge page for collapse
- * @node: appointed node the new huge page allocate from
+ * @cc: collapse context and scratchpad
  *
  * Basic scheme is simple, details are more complex:
  *  - allocate and lock a new huge page;

@@ -1686,8 +1688,8 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  *    + unlock and free huge page;
  */
 static void collapse_file(struct mm_struct *mm,
-             struct file *file, pgoff_t start,
-             struct page **hpage, int node)
+                       struct file *file, pgoff_t start,
+                       struct collapse_control *cc)
 {
      struct address_space *mapping = file->f_mapping;
      gfp_t gfp;

@@ -1697,15 +1699,16 @@ static void collapse_file(struct mm_struct *mm,
      XA_STATE_ORDER(xas, &mapping->i_pages, start, HPAGE_PMD_ORDER);
      int nr_none = 0, result = SCAN_SUCCEED;
      bool is_shmem = shmem_file(file);
-     int nr;
+     int nr, node;
 
      VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
      VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
 
      /* Only allocate from the target node */
-     gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+     gfp = cc->gfp() | __GFP_THISNODE;
+     node = khugepaged_find_target_node(cc);
 
-     new_page = khugepaged_alloc_page(hpage, gfp, node);
+     new_page = cc->alloc_hpage(cc, gfp, node);
      if (!new_page) {
              result = SCAN_ALLOC_HUGE_PAGE_FAIL;
              goto out;

@@ -1998,7 +2001,7 @@ static void collapse_file(struct mm_struct *mm,
               * Remove pte page tables, so we can re-fault the page as huge.
               */
              retract_page_tables(mapping, start);
-             *hpage = NULL;
+             cc->hpage = NULL;
 
              khugepaged_pages_collapsed++;
      } else {

@@ -2045,14 +2048,14 @@ static void collapse_file(struct mm_struct *mm,
      unlock_page(new_page);
 out:
      VM_BUG_ON(!list_empty(&pagelist));
-     if (!IS_ERR_OR_NULL(*hpage))
-             mem_cgroup_uncharge(page_folio(*hpage));
+     if (!IS_ERR_OR_NULL(cc->hpage))
+             mem_cgroup_uncharge(page_folio(cc->hpage));
      /* TODO: tracepoints */
 }
 
 static void khugepaged_scan_file(struct mm_struct *mm,
-             struct file *file, pgoff_t start, struct page **hpage,
-             struct collapse_control *cc)
+                              struct file *file, pgoff_t start,
+                              struct collapse_control *cc)
 {
      struct page *page = NULL;
      struct address_space *mapping = file->f_mapping;

@@ -2125,8 +2128,7 @@ static void khugepaged_scan_file(struct mm_struct *mm,
                      result = SCAN_EXCEED_NONE_PTE;
                      count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
              } else {
-                     node = khugepaged_find_target_node(cc);
-                     collapse_file(mm, file, start, hpage, node);
+                     collapse_file(mm, file, start, cc);
              }
      }
 

@@ -2134,8 +2136,8 @@ static void khugepaged_scan_file(struct mm_struct *mm,
 }
 #else
 static void khugepaged_scan_file(struct mm_struct *mm,
-             struct file *file, pgoff_t start, struct page **hpage,
-             struct collapse_control *cc)
+                              struct file *file, pgoff_t start,
+                              struct collapse_control *cc)
 {
      BUILD_BUG();
 }

@@ -2146,7 +2148,6 @@ static void khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot)
 #endif
 
 static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
-                                         struct page **hpage,
                                          struct collapse_control *cc)
      __releases(&khugepaged_mm_lock)
      __acquires(&khugepaged_mm_lock)

@@ -2223,12 +2224,11 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
 
                              mmap_read_unlock(mm);
                              ret = 1;
-                             khugepaged_scan_file(mm, file, pgoff, hpage, cc);
+                             khugepaged_scan_file(mm, file, pgoff, cc);
                              fput(file);
                      } else {
                              ret = khugepaged_scan_pmd(mm, vma,
-                                             khugepaged_scan.address,
-                                             hpage, cc);
+                                             khugepaged_scan.address, cc);
                      }
                      /* move to next address */
                      khugepaged_scan.address += HPAGE_PMD_SIZE;

@@ -2286,15 +2286,15 @@ static int khugepaged_wait_event(void)
 
 static void khugepaged_do_scan(struct collapse_control *cc)
 {
-     struct page *hpage = NULL;
      unsigned int progress = 0, pass_through_head = 0;
      unsigned int pages = READ_ONCE(khugepaged_pages_to_scan);
      bool wait = true;
 
+     cc->hpage = NULL;
      lru_add_drain_all();
 
      while (progress < pages) {
-             if (!khugepaged_prealloc_page(&hpage, &wait))
+             if (!khugepaged_prealloc_page(&cc->hpage, &wait))
                      break;
 
              cond_resched();

@@ -2308,14 +2308,14 @@ static void khugepaged_do_scan(struct collapse_control *cc)
              if (khugepaged_has_work() &&
                  pass_through_head < 2)
                      progress += khugepaged_scan_mm_slot(pages - progress,
-                                                         &hpage, cc);
+                                                         cc);
              else
                      progress = pages;
              spin_unlock(&khugepaged_mm_lock);
      }
 
-     if (!IS_ERR_OR_NULL(hpage))
-             put_page(hpage);
+     if (!IS_ERR_OR_NULL(cc->hpage))
+             put_page(cc->hpage);
 }
 

mm/khugepaged.c static int khugepaged(void *none) · line 2353

Quoted 7 lines

mm/khugepaged.c +2 -0

 static bool khugepaged_should_wakeup(void)

@@ -2349,6 +2349,8 @@ static int khugepaged(void *none)
      struct mm_slot *mm_slot;
      struct collapse_control cc = {
              .last_target_node = NUMA_NO_NODE,
+             .gfp = &alloc_hugepage_khugepaged_gfpmask,
+             .alloc_hpage = &khugepaged_alloc_page,

I'm also wondering whether we could avoid the gfp() hook, because logically that can be inlined into the ->alloc_hpage() hook, then we don't need gfp parameter anymore for ->alloc_hpage(), which seems to be a win-win?

--
Peter Xu

Quoted 4 lines

Quoted 4 lines

mm/khugepaged.c +0 -0

@@ -1110,13 +1115,14 @@ static void collapse_huge_page(struct mm_struct *mm,
       */
      mmap_read_unlock(mm);
 

Quoted 13 lines

mm/khugepaged.c static void collapse_huge_page(struct mm_struct *mm, · line 1125

Quoted 9 lines

mm/khugepaged.c +2 -1

+     node = khugepaged_find_target_node(cc);
      /* sched to specified node before huage page memory copy */
      if (task_node(current) != node) {
              cpumask = cpumask_of_node(node);
              if (!cpumask_empty(cpumask))
                      set_cpus_allowed_ptr(current, cpumask);
      }
-     new_page = khugepaged_alloc_page(hpage, gfp, node);
+     new_page = cc->alloc_hpage(cc, gfp, node);

AFAICT you removed all references of khugepaged_alloc_page() in this patch, then you'd better drop the function for both NUMA and UMA.

Sorry, I'm not sure I follow. In khugepaged context, logic WRT khugepaged_alloc_page() is unchanged - it's just called indirectly through ->alloc_hpage().

Quoted 11 lines

Said that, I think it's actually better to keep them, as they do things useful. For example,AFAICT this ->alloc_hpage() change can leak the hpage alloacted for UMA already so that's why I think keeping them makes sense, then iiuc the BUG_ON would trigger with UMA already.

I saw that you've moved khugepaged_find_target_node() into this function which looks definitely good, but if we could keep khugepaged_alloc_page() and then IMHO we could even move khugepaged_find_target_node() into that and drop the "node" parameter in khugepaged_alloc_page().

I actually had done this, until commit 4272063db38c ("mm/khugepaged:
sched to numa node when collapse huge page") which forced me to keep "node" visible in this function.

Quoted 4 lines

I'd even consider moving cc->gfp() all into it if possible, since the gfp seems to be always with __GFP_THISNODE anyways.

I would have liked to do this, but the gfp flags are referenced again when calling mem_cgroup_charge(), so I couldn't quite get rid of them from here.

I could (?) make .gfp of type gfp_t and just update it on every khugepaged scan (in case it changed) and also remove the gfp parameter for ->alloc_hpage().

Quoted 18 lines · Zach O'Keefe

Quoted 4 lines

Quoted 4 lines

mm/khugepaged.c +0 -0

@@ -1110,13 +1115,14 @@ static void collapse_huge_page(struct mm_struct *mm,
       */
      mmap_read_unlock(mm);
 

Quoted 13 lines

mm/khugepaged.c static void collapse_huge_page(struct mm_struct *mm, · line 1125

Quoted 9 lines

mm/khugepaged.c +2 -1

+     node = khugepaged_find_target_node(cc);
      /* sched to specified node before huage page memory copy */
      if (task_node(current) != node) {
              cpumask = cpumask_of_node(node);
              if (!cpumask_empty(cpumask))
                      set_cpus_allowed_ptr(current, cpumask);
      }
-     new_page = khugepaged_alloc_page(hpage, gfp, node);
+     new_page = cc->alloc_hpage(cc, gfp, node);

AFAICT you removed all references of khugepaged_alloc_page() in this patch, then you'd better drop the function for both NUMA and UMA.

Quoted 3 lines

Sorry, I'm not sure I follow. In khugepaged context, logic WRT khugepaged_alloc_page() is unchanged - it's just called indirectly through ->alloc_hpage().

Ah you're right, sorry for the confusion.

Quoted 12 lines

Quoted 11 lines

Said that, I think it's actually better to keep them, as they do things useful. For example,AFAICT this ->alloc_hpage() change can leak the hpage alloacted for UMA already so that's why I think keeping them makes sense, then iiuc the BUG_ON would trigger with UMA already.

I saw that you've moved khugepaged_find_target_node() into this function which looks definitely good, but if we could keep khugepaged_alloc_page() and then IMHO we could even move khugepaged_find_target_node() into that and drop the "node" parameter in khugepaged_alloc_page().

Quoted 3 lines

I actually had done this, until commit 4272063db38c ("mm/khugepaged:
sched to numa node when collapse huge page") which forced me to keep "node" visible in this function.

Right, I was looking at my local tree and that patch seems to be very lately added into -next.

I'm curious why it wasn't applying to file thps too if it is worthwhile, since if so that's also a suitable candidate to be moved into the same hook. I've asked in that thread instead.

Before that, feel free to keep your code as is.

Quoted 8 lines

Quoted 4 lines

I'd even consider moving cc->gfp() all into it if possible, since the gfp seems to be always with __GFP_THISNODE anyways.

I would have liked to do this, but the gfp flags are referenced again when calling mem_cgroup_charge(), so I couldn't quite get rid of them from here.

Yeah, maybe we could move mem_cgroup_charge() into the hook too? As below codes are duplicated between file & anon and IMHO they're good candidate to a new helper already anyway:

        /* Only allocate from the target node */
        gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;

        new_page = khugepaged_alloc_page(hpage, gfp, node);
        if (!new_page) {
                result = SCAN_ALLOC_HUGE_PAGE_FAIL;
                goto out;
        }

        if (unlikely(mem_cgroup_charge(page_folio(new_page), mm, gfp))) {
                result = SCAN_CGROUP_CHARGE_FAIL;
                goto out;
        }
        count_memcg_page_event(new_page, THP_COLLAPSE_ALLOC);

If we want to generalize it maybe we want to return the "result" instead of the new page though, so the newpage can be passed in as a pointer.

There's one mmap_read_unlock(mm) for the anonymous code but I think that can simply be moved above the chunk.

No strong opinion here, please feel free to think about what's the best approach for landing this series.

[...]

Quoted 4 lines

I could (?) make .gfp of type gfp_t and just update it on every khugepaged scan (in case it changed) and also remove the gfp parameter for ->alloc_hpage().

If that's the case I'd prefer you keep you code as-is; gfp() is perfectly fine and gfp() is light, I'm afraid that caching thing could make things complicated.

Thanks,

--
Peter Xu

Quoted 49 lines · Peter Xu

Quoted 18 lines · Zach O'Keefe

Quoted 4 lines

Quoted 4 lines

mm/khugepaged.c +0 -0

@@ -1110,13 +1115,14 @@ static void collapse_huge_page(struct mm_struct *mm,
       */
      mmap_read_unlock(mm);
 

Quoted 13 lines

mm/khugepaged.c static void collapse_huge_page(struct mm_struct *mm, · line 1125

Quoted 9 lines

mm/khugepaged.c +2 -1

+     node = khugepaged_find_target_node(cc);
      /* sched to specified node before huage page memory copy */
      if (task_node(current) != node) {
              cpumask = cpumask_of_node(node);
              if (!cpumask_empty(cpumask))
                      set_cpus_allowed_ptr(current, cpumask);
      }
-     new_page = khugepaged_alloc_page(hpage, gfp, node);
+     new_page = cc->alloc_hpage(cc, gfp, node);

AFAICT you removed all references of khugepaged_alloc_page() in this patch, then you'd better drop the function for both NUMA and UMA.

Quoted 3 lines

Sorry, I'm not sure I follow. In khugepaged context, logic WRT khugepaged_alloc_page() is unchanged - it's just called indirectly through ->alloc_hpage().

Ah you're right, sorry for the confusion.

Quoted 12 lines

Quoted 11 lines

Said that, I think it's actually better to keep them, as they do things useful. For example,AFAICT this ->alloc_hpage() change can leak the hpage alloacted for UMA already so that's why I think keeping them makes sense, then iiuc the BUG_ON would trigger with UMA already.

I saw that you've moved khugepaged_find_target_node() into this function which looks definitely good, but if we could keep khugepaged_alloc_page() and then IMHO we could even move khugepaged_find_target_node() into that and drop the "node" parameter in khugepaged_alloc_page().

Quoted 3 lines

I actually had done this, until commit 4272063db38c ("mm/khugepaged:
sched to numa node when collapse huge page") which forced me to keep "node" visible in this function.

Right, I was looking at my local tree and that patch seems to be very lately added into -next.

I'm curious why it wasn't applying to file thps too if it is worthwhile, since if so that's also a suitable candidate to be moved into the same hook. I've asked in that thread instead.

Quoted 2 lines

Before that, feel free to keep your code as is.

Thanks for checking on that. On second thought, it seems like we would actually want to move this sched logic into the khupaged hook impl since we probably don't want to be moving around the calling process if in madvise context.

Quoted 35 lines

Quoted 9 lines

Quoted 5 lines

I'd even consider moving cc->gfp() all into it if possible, since the gfp seems to be always with __GFP_THISNODE anyways.

I would have liked to do this, but the gfp flags are referenced again when calling mem_cgroup_charge(), so I couldn't quite get rid of them from here.

Yeah, maybe we could move mem_cgroup_charge() into the hook too? As below codes are duplicated between file & anon and IMHO they're good candidate to a new helper already anyway:

        /* Only allocate from the target node */
        gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;

        new_page = khugepaged_alloc_page(hpage, gfp, node);
        if (!new_page) {
                result = SCAN_ALLOC_HUGE_PAGE_FAIL;
                goto out;
        }

        if (unlikely(mem_cgroup_charge(page_folio(new_page), mm, gfp))) {
                result = SCAN_CGROUP_CHARGE_FAIL;
                goto out;
        }
        count_memcg_page_event(new_page, THP_COLLAPSE_ALLOC);

If we want to generalize it maybe we want to return the "result" instead of the new page though, so the newpage can be passed in as a pointer.

There's one mmap_read_unlock(mm) for the anonymous code but I think that can simply be moved above the chunk.

Quoted 3 lines

No strong opinion here, please feel free to think about what's the best approach for landing this series.

Thanks for the suggestion. I'll play around with it a little and see what looks good.

Review

Quoted 7 lines · Zach O'Keefe

(quoted diff) +7 -0

+/* Gather information from one khugepaged_scan_[pmd|file]() request */
+struct collapse_result {
+     enum scan_result result;
+
+     /* Was mmap_lock dropped during request? */
+     bool dropped_mmap_lock;
+};

IMHO this new dropped_mmap_lock makes things a bit more complicated..

To me, the old code actually is easy to read on when the lock is dropped:

- For file, we always drop it
- For anon, when khugepaged_scan_pmd() returns 1

That's fairly clear to me.

But I understand what you need, probably the "result" hidden in the old world but you'd need that for MADV_COLLAPSE?

I also noticed major chunks of this patch is converting "result = XXX" into "cr->result = XXX". IMHO that's really not necessary for touching up all the places around.

I'm also wondering whether we could simply pass over the result into your newly introduced collapse_control*.

In summary, this is what's in my mind..

- Add collapse_control.result
- Make sure both khugepaged_scan_file() and khugepaged_scan_pmd() to

    simply remember to update cc->result before it returns

I've attached a pesudo code patch (note! it's pesudo not even compile but to show what I meant..), so far I don't see a problem with it.

--
Peter Xu

Quoted 17 lines · Peter Xu

Review

Quoted 7 lines · Zach O'Keefe

(quoted diff) +7 -0

+/* Gather information from one khugepaged_scan_[pmd|file]() request */
+struct collapse_result {
+     enum scan_result result;
+
+     /* Was mmap_lock dropped during request? */
+     bool dropped_mmap_lock;
+};

IMHO this new dropped_mmap_lock makes things a bit more complicated..

To me, the old code actually is easy to read on when the lock is dropped:

- For file, we always drop it
- For anon, when khugepaged_scan_pmd() returns 1

Quoted 2 lines

That's fairly clear to me.

Agreed - but I know when I first read the code it wasn't obvious to me what the return value of khugepaged_scan_pmd() was saying. I thought giving it a name ("dropped_mmap_lock") would help future readers.

Quoted 18 lines · Zach O'Keefe

Quoted 17 lines · Peter Xu

Review

Quoted 7 lines · Zach O'Keefe

(quoted diff) +7 -0

+/* Gather information from one khugepaged_scan_[pmd|file]() request */
+struct collapse_result {
+     enum scan_result result;
+
+     /* Was mmap_lock dropped during request? */
+     bool dropped_mmap_lock;
+};

IMHO this new dropped_mmap_lock makes things a bit more complicated..

To me, the old code actually is easy to read on when the lock is dropped:

- For file, we always drop it
- For anon, when khugepaged_scan_pmd() returns 1

Quoted 6 lines

Quoted 2 lines

That's fairly clear to me.

Agreed - but I know when I first read the code it wasn't obvious to me what the return value of khugepaged_scan_pmd() was saying. I thought giving it a name ("dropped_mmap_lock") would help future readers.

Yes that's a fair point. It's not that obvious to me too when I first read it, but I am just worried hiding that too deep could make it complicated in another form.

Maybe we can rename "ret" in khugepaged_scan_mm_slot() to "lock_dropped", or we can comment above khugepaged_scan_pmd() explaining the retval. Or.. both?

--
Peter Xu

The following functions/tracepoints are shared between khugepaged and madvise collapse contexts. Remove the khugepaged prefixes.

tracepoint:mm_khugepaged_scan_pmd -> tracepoint:mm_scan_pmd khugepaged_test_exit() -> test_exit() khugepaged_scan_abort() -> scan_abort() khugepaged_scan_pmd() -> scan_pmd() khugepaged_find_target_node() -> find_target_node()

Quoted 8 lines · Zach O'Keefe

The following functions/tracepoints are shared between khugepaged and madvise collapse contexts. Remove the khugepaged prefixes.

tracepoint:mm_khugepaged_scan_pmd -> tracepoint:mm_scan_pmd khugepaged_test_exit() -> test_exit() khugepaged_scan_abort() -> scan_abort() khugepaged_scan_pmd() -> scan_pmd() khugepaged_find_target_node() -> find_target_node()

The khugepaged prefix can indeed be confusing, but are the new names just too short? IMHO it'll be nice to still have some generic prefix.

Perhaps something like "hpage_collapse_"/"hpage_"/"hpc_"/..?

--
Peter Xu

mm/khugepaged.c +1 -0

@@ -2365,6 +2375,7 @@ static int khugepaged(void *none)
 {
 	struct mm_slot *mm_slot;
 	struct collapse_control cc = {
+		.enforce_pte_scan_limits = true,
 		.last_target_node = NUMA_NO_NODE,
 		.gfp = &alloc_hugepage_khugepaged_gfpmask,
 		.alloc_hpage = &khugepaged_alloc_page,

Quoted 12 lines · Zach O'Keefe

mm/khugepaged.c +1 -0

@@ -2365,6 +2375,7 @@ static int khugepaged(void *none)
 {
      struct mm_slot *mm_slot;
      struct collapse_control cc = {
+             .enforce_pte_scan_limits = true,
              .last_target_node = NUMA_NO_NODE,
              .gfp = &alloc_hugepage_khugepaged_gfpmask,
              .alloc_hpage = &khugepaged_alloc_page,

@@ -2512,6 +2523,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
                   unsigned long start, unsigned long end)
 {
      struct collapse_control cc = {

mm/khugepaged.c int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev, · line 2529

Quoted 4 lines

mm/khugepaged.c +1 -0

+             .enforce_pte_scan_limits = false,
              .last_target_node = NUMA_NO_NODE,
              .hpage = NULL,
              .gfp = &alloc_hugepage_madvise_gfpmask,

This changes the semantics of the new madvise(). IMHO it'll be ideal if this patch is moved before the introduction of MADV_COLLAPSE, so the new madvise() will have a consistent behavior.

--
Peter Xu