mm: userspace hugepage collapse

From: Yang Shi <[email protected]>

The khugepaged has optimization to reduce huge page allocation calls for !CONFIG_NUMA by carrying the allocated but failed to collapse huge page to the next loop. CONFIG_NUMA doesn't do so since the next loop may try to collapse huge page from a different node, so it doesn't make too much sense to carry it.

But when NUMA=n, the huge page is allocated by khugepaged_prealloc_page() before scanning the address space, so it means huge page may be allocated even though there is no suitable range for collapsing. Then the page would be just freed if khugepaged already made enough progress. This could make NUMA=n run have 5 times as much thp_collapse_alloc as NUMA=y run. This problem actually makes things worse due to the way more pointless THP allocations and makes the optimization pointless.

This could be fixed by carrying the huge page across scans, but it will complicate the code further and the huge page may be carried indefinitely. But if we take one step back, the optimization itself seems not worth keeping nowadays since:
* Not too many users build NUMA=n kernel nowadays even though the kernel is actually running on a non-NUMA machine. Some small devices may run NUMA=n kernel, but I don't think they actually use THP.
* Since commit 44042b449872 ("mm/page_alloc: allow high-order pages to be stored on the per-cpu lists"), THP could be cached by pcp. This actually somehow does the job done by the optimization.

Cc Hugh Dickins <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

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

---
 mm/khugepaged.c | 100 ++++++++----------------------------------------
 1 file changed, 17 insertions(+), 83 deletions(-)

mm/khugepaged.c +17 -84

index 476d79360101..cc3d6fb446d5 100644
@@ -833,29 +833,30 @@ static int khugepaged_find_target_node(void)
        last_khugepaged_target_node = target_node;
        return target_node;
 }
+#else
+static int khugepaged_find_target_node(void)
+{
+       return 0;
+}
+#endif
 
-static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
+/* Sleep for the first alloc fail, break the loop for the second fail */
+static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
 {
        if (IS_ERR(*hpage)) {
                if (!*wait)
-                       return false;
+                       return true;
 
                *wait = false;
                *hpage = NULL;
                khugepaged_alloc_sleep();
-       } else if (*hpage) {
-               put_page(*hpage);
-               *hpage = NULL;
        }
-
-       return true;
+       return false;
 }
 
 static struct page *
 khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
 {
-       VM_BUG_ON_PAGE(*hpage, *hpage);
-
        *hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
        if (unlikely(!*hpage)) {
                count_vm_event(THP_COLLAPSE_ALLOC_FAILED);

@@ -867,74 +868,6 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
        count_vm_event(THP_COLLAPSE_ALLOC);
        return *hpage;
 }
-#else
-static int khugepaged_find_target_node(void)
-{
-       return 0;
-}
-
-static inline struct page *alloc_khugepaged_hugepage(void)
-{
-       struct page *page;
-
-       page = alloc_pages(alloc_hugepage_khugepaged_gfpmask(),
-                          HPAGE_PMD_ORDER);
-       if (page)
-               prep_transhuge_page(page);
-       return page;
-}
-
-static struct page *khugepaged_alloc_hugepage(bool *wait)
-{
-       struct page *hpage;
-
-       do {
-               hpage = alloc_khugepaged_hugepage();
-               if (!hpage) {
-                       count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
-                       if (!*wait)
-                               return NULL;
-
-                       *wait = false;
-                       khugepaged_alloc_sleep();
-               } else
-                       count_vm_event(THP_COLLAPSE_ALLOC);
-       } while (unlikely(!hpage) && likely(khugepaged_enabled()));
-
-       return hpage;
-}
-
-static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
-{
-       /*
-        * If the hpage allocated earlier was briefly exposed in page cache
-        * before collapse_file() failed, it is possible that racing lookups
-        * have not yet completed, and would then be unpleasantly surprised by
-        * finding the hpage reused for the same mapping at a different offset.
-        * Just release the previous allocation if there is any danger of that.
-        */
-       if (*hpage && page_count(*hpage) > 1) {
-               put_page(*hpage);
-               *hpage = NULL;
-       }
-
-       if (!*hpage)
-               *hpage = khugepaged_alloc_hugepage(wait);
-
-       if (unlikely(!*hpage))
-               return false;
-
-       return true;
-}
-
-static struct page *
-khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
-{
-       VM_BUG_ON(!*hpage);
-
-       return  *hpage;
-}
-#endif
 
 /*
  * If mmap_lock temporarily dropped, revalidate vma

@@ -1188,8 +1121,10 @@ static void collapse_huge_page(struct mm_struct *mm,
 out_up_write:
        mmap_write_unlock(mm);
 out_nolock:
-       if (!IS_ERR_OR_NULL(*hpage))
+       if (!IS_ERR_OR_NULL(*hpage)) {
                mem_cgroup_uncharge(page_folio(*hpage));
+               put_page(*hpage);
+       }
        trace_mm_collapse_huge_page(mm, isolated, result);
        return;
 }

@@ -1992,8 +1927,10 @@ 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))
+       if (!IS_ERR_OR_NULL(*hpage)) {
                mem_cgroup_uncharge(page_folio(*hpage));
+               put_page(*hpage);
+       }
        /* TODO: tracepoints */
 }
 

@@ -2243,7 +2180,7 @@ static void khugepaged_do_scan(void)
        lru_add_drain_all();
 
        while (progress < pages) {
-               if (!khugepaged_prealloc_page(&hpage, &wait))
+               if (alloc_fail_should_sleep(&hpage, &wait))
                        break;
 
                cond_resched();

@@ -2262,9 +2199,6 @@ static void khugepaged_do_scan(void)
                        progress = pages;
                spin_unlock(&khugepaged_mm_lock);
        }
-
-       if (!IS_ERR_OR_NULL(hpage))
-               put_page(hpage);
 }
 
 static bool khugepaged_should_wakeup(void)
--

2.36.1.255.ge46751e96f-goog

(quoted diff) +3 -2

-static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
+/* Sleep for the first alloc fail, break the loop for the second fail */
+static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
 {
 	if (IS_ERR(*hpage)) {
 		if (!*wait)
-			return false;
+			return true;
 
 		*wait = false;

Quoted 10 lines · Zach O'Keefe

(quoted diff) +3 -2

-static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
+/* Sleep for the first alloc fail, break the loop for the second fail */
+static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
 {
 	if (IS_ERR(*hpage)) {
 		if (!*wait)
-			return false;
+			return true;
 
 		*wait = false;

Review

Quoted 10 lines

(quoted diff) +1 -5

 		*hpage = NULL;
 		khugepaged_alloc_sleep();
-	} else if (*hpage) {
-		put_page(*hpage);
-		*hpage = NULL;
 	}
-
-	return true;
+	return false;
 }

One nitpick here:

It's weird to me to sleep in a function called XXX_should_sleep(), we'd normally expect to sleep only if it returns true.

Meanwhile, would this be a very good chance to unwrap this function already to remove the "bool*" reference, which looks not pretty? Something like:

---8<---

mm/khugepaged.c +12 -4

index 16be62d493cd..807c10cd0816 100644
@@ -2235,9 +2235,6 @@ static void khugepaged_do_scan(void)
        lru_add_drain_all();
 
        while (progress < pages) {
-               if (!khugepaged_prealloc_page(&hpage, &wait))
-                       break;
-
                cond_resched();
 
                if (unlikely(kthread_should_stop() || try_to_freeze()))

@@ -2253,6 +2250,18 @@ static void khugepaged_do_scan(void)
                else
                        progress = pages;
                spin_unlock(&khugepaged_mm_lock);
+
+               if (IS_ERR(*hpage)) {
+                       /*
+                        * If fail to allocate the first time, try to sleep
+                        * for a while.  When hit again, cancel the scan.
+                        */
+                       if (!wait)
+                               break;
+                       wait = false;
+                       *hpage = NULL;
+                       khugepaged_alloc_sleep();
+               }
        }
---8<---
 

Would this look slightly better?

Thanks,

Signature · 1 line

Peter Xu

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]>

---
 include/trace/events/huge_memory.h |  1 +
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 32 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 ++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

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

index d651f3437367..55392bf30a03 100644
@@ -11,6 +11,7 @@
        EM( SCAN_FAIL,                  "failed")                       \
        EM( SCAN_SUCCEED,               "succeeded")                    \
        EM( SCAN_PMD_NULL,              "pmd_null")                     \
+       EM( SCAN_PMD_MAPPED,            "page_pmd_mapped")              \
        EM( SCAN_EXCEED_NONE_PTE,       "exceed_none_pte")              \
        EM( SCAN_EXCEED_SWAP_PTE,       "exceed_swap_pte")              \
        EM( SCAN_EXCEED_SHARED_PTE,     "exceed_shared_pte")            \

mm/internal.h +1 -0

index 6e14749ad1e5..f768c7fae668 100644
@@ -188,6 +188,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 cc3d6fb446d5..7a914ca19e96 100644
@@ -28,6 +28,7 @@ enum scan_result {
        SCAN_FAIL,
        SCAN_SUCCEED,
        SCAN_PMD_NULL,
+       SCAN_PMD_MAPPED,
        SCAN_EXCEED_NONE_PTE,
        SCAN_EXCEED_SWAP_PTE,
        SCAN_EXCEED_SHARED_PTE,

@@ -901,6 +902,31 @@ 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 +3 -0

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

mm/khugepaged.c +0 -1

                goto out;
-       }
 
        memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
        pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +0 -0

index 04fac1af870b..c9979c6ad7a1 100644

mm/rmap.c +12 -2

 {
        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))

@@ -788,6 +787,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
--

2.36.1.255.ge46751e96f-goog

Quoted 74 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.

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

---
 include/trace/events/huge_memory.h |  1 +
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 32 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 ++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

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

index d651f3437367..55392bf30a03 100644
@@ -11,6 +11,7 @@
        EM( SCAN_FAIL,                  "failed")                       \
        EM( SCAN_SUCCEED,               "succeeded")                    \
        EM( SCAN_PMD_NULL,              "pmd_null")                     \
+       EM( SCAN_PMD_MAPPED,            "page_pmd_mapped")              \
        EM( SCAN_EXCEED_NONE_PTE,       "exceed_none_pte")              \
        EM( SCAN_EXCEED_SWAP_PTE,       "exceed_swap_pte")              \
        EM( SCAN_EXCEED_SHARED_PTE,     "exceed_shared_pte")            \

mm/internal.h +1 -0

index 6e14749ad1e5..f768c7fae668 100644
@@ -188,6 +188,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 +19 -0

index cc3d6fb446d5..7a914ca19e96 100644
@@ -28,6 +28,7 @@ enum scan_result {
        SCAN_FAIL,
        SCAN_SUCCEED,
        SCAN_PMD_NULL,
+       SCAN_PMD_MAPPED,
        SCAN_EXCEED_NONE_PTE,
        SCAN_EXCEED_SWAP_PTE,
        SCAN_EXCEED_SHARED_PTE,

@@ -901,6 +902,31 @@ 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);
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
+       barrier();
+#endif
+       if (!pmd_present(pmde))
+               return SCAN_PMD_NULL;

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

Quoted 4 lines

mm/khugepaged.c +4 -0

+       if (pmd_trans_huge(pmde))
+               return SCAN_PMD_MAPPED;
+       if (pmd_bad(pmde))
+               return SCAN_FAIL;

khugepaged doesn't handle pmd_bad before, IIRC it may just return SCAN_SUCCEED if everything else is good? It is fine to add it, but it may be better to return SCAN_PMD_NULL?

Quoted 11 lines

mm/khugepaged.c +3 -0

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

@@ -1146,11 +1172,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
        VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 

mm/khugepaged.c static int khugepaged_scan_pmd(struct mm_struct *mm, · line 1176

Quoted 5 lines

mm/khugepaged.c +2 -3

-       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)

There are a couple of other callsites for mm_find_pmd(), you may need to change all of them to find_pmd_or_thp_or_none() for MADV_COLLAPSE since khugepaged may collapse the area before MADV_COLLAPSE reacquiring mmap_lock IIUC and MADV_COLLAPSE does care this case. It is fine w/o MADV_COLLAPSE since khupaged doesn't care if it is PMD mapped or not.

So it may be better to move this patch right before MADV_COLLAPSE is introduced?

Quoted 10 lines

mm/khugepaged.c +0 -1

                goto out;
-       }
 
        memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
        pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +0 -0

index 04fac1af870b..c9979c6ad7a1 100644

mm/rmap.c unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma) · line 770

Quoted 5 lines

mm/rmap.c +1 -1

@@ -767,13 +767,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)

May be better to have some notes for mm_find_pmd_raw() and mm_find_pmd().

Quoted 31 lines

mm/rmap.c +12 -2

 {
        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))

@@ -788,6 +787,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
--

2.36.1.255.ge46751e96f-goog

Quoted 76 lines · Yang Shi

Quoted 74 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.

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

---
 include/trace/events/huge_memory.h |  1 +
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 32 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 ++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

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

index d651f3437367..55392bf30a03 100644
@@ -11,6 +11,7 @@
        EM( SCAN_FAIL,                  "failed")                       \
        EM( SCAN_SUCCEED,               "succeeded")                    \
        EM( SCAN_PMD_NULL,              "pmd_null")                     \
+       EM( SCAN_PMD_MAPPED,            "page_pmd_mapped")              \
        EM( SCAN_EXCEED_NONE_PTE,       "exceed_none_pte")              \
        EM( SCAN_EXCEED_SWAP_PTE,       "exceed_swap_pte")              \
        EM( SCAN_EXCEED_SHARED_PTE,     "exceed_shared_pte")            \

mm/internal.h +1 -0

index 6e14749ad1e5..f768c7fae668 100644
@@ -188,6 +188,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 +19 -0

index cc3d6fb446d5..7a914ca19e96 100644
@@ -28,6 +28,7 @@ enum scan_result {
        SCAN_FAIL,
        SCAN_SUCCEED,
        SCAN_PMD_NULL,
+       SCAN_PMD_MAPPED,
        SCAN_EXCEED_NONE_PTE,
        SCAN_EXCEED_SWAP_PTE,
        SCAN_EXCEED_SHARED_PTE,

@@ -901,6 +902,31 @@ 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);
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
+       barrier();
+#endif
+       if (!pmd_present(pmde))
+               return SCAN_PMD_NULL;

Quoted 8 lines

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

Quoted 4 lines

mm/khugepaged.c +4 -0

+       if (pmd_trans_huge(pmde))
+               return SCAN_PMD_MAPPED;
+       if (pmd_bad(pmde))
+               return SCAN_FAIL;

khugepaged doesn't handle pmd_bad before, IIRC it may just return SCAN_SUCCEED if everything else is good? It is fine to add it, but it may be better to return SCAN_PMD_NULL?

Correct, pmd_bad() wasn't handled before. I actually don't know how a bad pmd might arise in the wild (would love to actually know this), but I don't see the check hurting (might be overly convervative though). Conversely, I'm not sure where things go astray currently if the pmd is bad. Guess it depends in what way the flags are mutated. Returning SCAN_PMD_NULL SGTM.

Quoted 11 lines

Quoted 11 lines

mm/khugepaged.c +3 -0

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

@@ -1146,11 +1172,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
        VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 

Quoted 12 lines

mm/khugepaged.c static int khugepaged_scan_pmd(struct mm_struct *mm, · line 1176

Quoted 5 lines

mm/khugepaged.c +2 -3

-       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)

There are a couple of other callsites for mm_find_pmd(), you may need to change all of them to find_pmd_or_thp_or_none() for MADV_COLLAPSE since khugepaged may collapse the area before MADV_COLLAPSE reacquiring mmap_lock IIUC and MADV_COLLAPSE does care this case. It is fine w/o MADV_COLLAPSE since khupaged doesn't care if it is PMD mapped or not.

Ya, I was just questioning the same thing after responding above - at least w.r.t whether the pmd_bad() also needs to be in these callsites (check for pmd mapping, as you mention, I think is definitely necessary). Thanks for catching this!

Quoted 1 line

So it may be better to move this patch right before MADV_COLLAPSE is introduced?

I think this should be ok - I'll give it a try at least.

Again, thank you for taking the time to thoroughly review this.

Best,
Zach

Quoted 18 lines

Quoted 10 lines

mm/khugepaged.c +0 -1

                goto out;
-       }
 
        memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
        pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +0 -0

index 04fac1af870b..c9979c6ad7a1 100644

mm/rmap.c unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma) · line 770

Quoted 5 lines

mm/rmap.c +1 -1

@@ -767,13 +767,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)

May be better to have some notes for mm_find_pmd_raw() and mm_find_pmd().

Quoted 31 lines

Quoted 31 lines

mm/rmap.c +12 -2

 {
        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))

@@ -788,6 +787,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
--

2.36.1.255.ge46751e96f-goog

Quoted 123 lines · Zach O'Keefe

Quoted 76 lines · Yang Shi

Quoted 74 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.

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

---
 include/trace/events/huge_memory.h |  1 +
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 32 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 ++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

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

index d651f3437367..55392bf30a03 100644
@@ -11,6 +11,7 @@
        EM( SCAN_FAIL,                  "failed")                       \
        EM( SCAN_SUCCEED,               "succeeded")                    \
        EM( SCAN_PMD_NULL,              "pmd_null")                     \
+       EM( SCAN_PMD_MAPPED,            "page_pmd_mapped")              \
        EM( SCAN_EXCEED_NONE_PTE,       "exceed_none_pte")              \
        EM( SCAN_EXCEED_SWAP_PTE,       "exceed_swap_pte")              \
        EM( SCAN_EXCEED_SHARED_PTE,     "exceed_shared_pte")            \

mm/internal.h +1 -0

index 6e14749ad1e5..f768c7fae668 100644
@@ -188,6 +188,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 +19 -0

index cc3d6fb446d5..7a914ca19e96 100644
@@ -28,6 +28,7 @@ enum scan_result {
        SCAN_FAIL,
        SCAN_SUCCEED,
        SCAN_PMD_NULL,
+       SCAN_PMD_MAPPED,
        SCAN_EXCEED_NONE_PTE,
        SCAN_EXCEED_SWAP_PTE,
        SCAN_EXCEED_SHARED_PTE,

@@ -901,6 +902,31 @@ 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);
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
+       barrier();
+#endif
+       if (!pmd_present(pmde))
+               return SCAN_PMD_NULL;

Quoted 8 lines

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

Quoted 4 lines

mm/khugepaged.c +4 -0

+       if (pmd_trans_huge(pmde))
+               return SCAN_PMD_MAPPED;
+       if (pmd_bad(pmde))
+               return SCAN_FAIL;

khugepaged doesn't handle pmd_bad before, IIRC it may just return SCAN_SUCCEED if everything else is good? It is fine to add it, but it may be better to return SCAN_PMD_NULL?

Correct, pmd_bad() wasn't handled before. I actually don't know how a bad pmd might arise in the wild (would love to actually know this), but I don't see the check hurting (might be overly convervative though). Conversely, I'm not sure where things go astray currently if the pmd is bad. Guess it depends in what way the flags are mutated. Returning SCAN_PMD_NULL SGTM.

Quoted 11 lines

Quoted 11 lines

mm/khugepaged.c +3 -0

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

@@ -1146,11 +1172,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
        VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 

Quoted 12 lines

mm/khugepaged.c static int khugepaged_scan_pmd(struct mm_struct *mm, · line 1176

Quoted 5 lines

mm/khugepaged.c +2 -3

-       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)

There are a couple of other callsites for mm_find_pmd(), you may need to change all of them to find_pmd_or_thp_or_none() for MADV_COLLAPSE since khugepaged may collapse the area before MADV_COLLAPSE reacquiring mmap_lock IIUC and MADV_COLLAPSE does care this case. It is fine w/o MADV_COLLAPSE since khupaged doesn't care if it is PMD mapped or not.

Ya, I was just questioning the same thing after responding above - at least w.r.t whether the pmd_bad() also needs to be in these callsites (check for pmd mapping, as you mention, I think is definitely necessary). Thanks for catching this!

Quoted 27 lines

Quoted 1 line

So it may be better to move this patch right before MADV_COLLAPSE is introduced?

I think this should be ok - I'll give it a try at least.

Again, thank you for taking the time to thoroughly review this.

Best,
Zach

Quoted 18 lines

Quoted 10 lines

mm/khugepaged.c +0 -1

                goto out;
-       }
 
        memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
        pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +0 -0

index 04fac1af870b..c9979c6ad7a1 100644

mm/rmap.c unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma) · line 770

Quoted 5 lines

mm/rmap.c +1 -1

@@ -767,13 +767,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)

May be better to have some notes for mm_find_pmd_raw() and mm_find_pmd().

Agreed. Looking over this code again, there are only 3 users of mm_find_pmd():

1) khugepaged
2) ksm (replace_page())
3) split_huge_pmd_address()

Once khugepaged codepaths care about THP-pmds, ksm is the only remaining user that really wants a pte-mapping pmd.

I've gone and consolidated the open-coded code in split_huge_pmd_address() to use the mm_find_pmd_raw().

I've also done a name switch:

mm_find_pmd() -> mm_find_pte_pmd()
mm_find_pmd_raw() -> mm_find_pmd()

This basically reverts mm_find_pmd() to its pre commit f72e7dcdd252 ("mm: let mm_find_pmd fix buggy race with THP fault") behavior, and special cases (what will be, after MADV_COLLAPSE file support) the only remaining callsite which *doesn't* care about THP-pmds (ksm). The naming here is a little more meaningful than "*raw", and IMHO more readable.

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

mm/rmap.c +12 -2

 {
        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))

@@ -788,6 +787,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
--

2.36.1.255.ge46751e96f-goog

Quoted 165 lines · Zach O'Keefe

Quoted 123 lines · Zach O'Keefe

Quoted 76 lines · Yang Shi

Quoted 74 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.

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

---
 include/trace/events/huge_memory.h |  1 +
 mm/internal.h                      |  1 +
 mm/khugepaged.c                    | 32 ++++++++++++++++++++++++++----
 mm/rmap.c                          | 15 ++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

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

index d651f3437367..55392bf30a03 100644
@@ -11,6 +11,7 @@
        EM( SCAN_FAIL,                  "failed")                       \
        EM( SCAN_SUCCEED,               "succeeded")                    \
        EM( SCAN_PMD_NULL,              "pmd_null")                     \
+       EM( SCAN_PMD_MAPPED,            "page_pmd_mapped")              \
        EM( SCAN_EXCEED_NONE_PTE,       "exceed_none_pte")              \
        EM( SCAN_EXCEED_SWAP_PTE,       "exceed_swap_pte")              \
        EM( SCAN_EXCEED_SHARED_PTE,     "exceed_shared_pte")            \

mm/internal.h +1 -0

index 6e14749ad1e5..f768c7fae668 100644
@@ -188,6 +188,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 +19 -0

index cc3d6fb446d5..7a914ca19e96 100644
@@ -28,6 +28,7 @@ enum scan_result {
        SCAN_FAIL,
        SCAN_SUCCEED,
        SCAN_PMD_NULL,
+       SCAN_PMD_MAPPED,
        SCAN_EXCEED_NONE_PTE,
        SCAN_EXCEED_SWAP_PTE,
        SCAN_EXCEED_SHARED_PTE,

@@ -901,6 +902,31 @@ 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);
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
+       barrier();
+#endif
+       if (!pmd_present(pmde))
+               return SCAN_PMD_NULL;

Quoted 8 lines

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

Quoted 4 lines

mm/khugepaged.c +4 -0

+       if (pmd_trans_huge(pmde))
+               return SCAN_PMD_MAPPED;
+       if (pmd_bad(pmde))
+               return SCAN_FAIL;

khugepaged doesn't handle pmd_bad before, IIRC it may just return SCAN_SUCCEED if everything else is good? It is fine to add it, but it may be better to return SCAN_PMD_NULL?

Correct, pmd_bad() wasn't handled before. I actually don't know how a bad pmd might arise in the wild (would love to actually know this), but I don't see the check hurting (might be overly convervative though). Conversely, I'm not sure where things go astray currently if the pmd is bad. Guess it depends in what way the flags are mutated. Returning SCAN_PMD_NULL SGTM.

Quoted 11 lines

Quoted 11 lines

mm/khugepaged.c +3 -0

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

@@ -1146,11 +1172,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
        VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 

Quoted 12 lines

mm/khugepaged.c static int khugepaged_scan_pmd(struct mm_struct *mm, · line 1176

Quoted 5 lines

mm/khugepaged.c +2 -3

-       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)

There are a couple of other callsites for mm_find_pmd(), you may need to change all of them to find_pmd_or_thp_or_none() for MADV_COLLAPSE since khugepaged may collapse the area before MADV_COLLAPSE reacquiring mmap_lock IIUC and MADV_COLLAPSE does care this case. It is fine w/o MADV_COLLAPSE since khupaged doesn't care if it is PMD mapped or not.

Ya, I was just questioning the same thing after responding above - at least w.r.t whether the pmd_bad() also needs to be in these callsites (check for pmd mapping, as you mention, I think is definitely necessary). Thanks for catching this!

Quoted 27 lines

Quoted 1 line

So it may be better to move this patch right before MADV_COLLAPSE is introduced?

I think this should be ok - I'll give it a try at least.

Again, thank you for taking the time to thoroughly review this.

Best,
Zach

Quoted 18 lines

Quoted 10 lines

mm/khugepaged.c +0 -1

                goto out;
-       }
 
        memset(khugepaged_node_load, 0, sizeof(khugepaged_node_load));
        pte = pte_offset_map_lock(mm, pmd, address, &ptl);

mm/rmap.c +0 -0

index 04fac1af870b..c9979c6ad7a1 100644

mm/rmap.c unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma) · line 770

Quoted 5 lines

mm/rmap.c +1 -1

@@ -767,13 +767,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)

May be better to have some notes for mm_find_pmd_raw() and mm_find_pmd().

Agreed. Looking over this code again, there are only 3 users of mm_find_pmd():

1) khugepaged
2) ksm (replace_page())
3) split_huge_pmd_address()

Once khugepaged codepaths care about THP-pmds, ksm is the only remaining user that really wants a pte-mapping pmd.

I've gone and consolidated the open-coded code in split_huge_pmd_address() to use the mm_find_pmd_raw().

Quoted 4 lines

I've also done a name switch:

mm_find_pmd() -> mm_find_pte_pmd()
mm_find_pmd_raw() -> mm_find_pmd()

If ksm is the only user of *current* mm_find_pmd(), I think you should be able to open code it w/o introducing mm_find_pte_pmd() and revert mm_find_pmd() to its *old* behavior.

Quoted 40 lines

This basically reverts mm_find_pmd() to its pre commit f72e7dcdd252 ("mm: let mm_find_pmd fix buggy race with THP fault") behavior, and special cases (what will be, after MADV_COLLAPSE file support) the only remaining callsite which *doesn't* care about THP-pmds (ksm). The naming here is a little more meaningful than "*raw", and IMHO more readable.

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

mm/rmap.c +12 -2

 {
        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))

@@ -788,6 +787,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
--

2.36.1.255.ge46751e96f-goog

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

vim +2284 mm/khugepaged.c

b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2261
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2262  static int khugepaged(void *none)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2263  {
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2264      struct mm_slot *mm_slot;
d87b6065d6050b Zach O'Keefe       2022-06-03  2265      struct collapse_control cc = {
d87b6065d6050b Zach O'Keefe       2022-06-03  2266              .last_target_node = NUMA_NO_NODE,
d87b6065d6050b Zach O'Keefe       2022-06-03  2267      };
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2268
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2269      set_freezable();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2270      set_user_nice(current, MAX_NICE);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2271
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2272      while (!kthread_should_stop()) {
d87b6065d6050b Zach O'Keefe       2022-06-03  2273              khugepaged_do_scan(&cc);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2274              khugepaged_wait_work();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2275      }
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2276
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2277      spin_lock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2278      mm_slot = khugepaged_scan.mm_slot;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2279      khugepaged_scan.mm_slot = NULL;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2280      if (mm_slot)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2281              collect_mm_slot(mm_slot);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2282      spin_unlock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2283      return 0;

b46e756f5e4703 Kirill A. Shutemov 2016-07-26 @2284 } b46e756f5e4703 Kirill A. Shutemov 2016-07-26 2285

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Thanks,
Zach

Quoted 33 lines

vim +2284 mm/khugepaged.c

b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2261
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2262  static int khugepaged(void *none)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2263  {
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2264      struct mm_slot *mm_slot;
d87b6065d6050b Zach O'Keefe       2022-06-03  2265      struct collapse_control cc = {
d87b6065d6050b Zach O'Keefe       2022-06-03  2266              .last_target_node = NUMA_NO_NODE,
d87b6065d6050b Zach O'Keefe       2022-06-03  2267      };
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2268
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2269      set_freezable();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2270      set_user_nice(current, MAX_NICE);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2271
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2272      while (!kthread_should_stop()) {
d87b6065d6050b Zach O'Keefe       2022-06-03  2273              khugepaged_do_scan(&cc);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2274              khugepaged_wait_work();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2275      }
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2276
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2277      spin_lock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2278      mm_slot = khugepaged_scan.mm_slot;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2279      khugepaged_scan.mm_slot = NULL;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2280      if (mm_slot)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2281              collect_mm_slot(mm_slot);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2282      spin_unlock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2283      return 0;

b46e756f5e4703 Kirill A. Shutemov 2016-07-26 @2284 } b46e756f5e4703 Kirill A. Shutemov 2016-07-26 2285

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Quoted 39 lines · Zach O'Keefe

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Quoted 6 lines

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

I don't have a better idea other than moving it out of the collapse_control struct. You may consider changing node_load to two dimensions, for example:

node_load[2][MAX_NUMNODES], then define:

enum {
    /* khugepaged */
    COLLAPSE_ASYNC,
    /* MADV_COLLAPSE */
    COLLAPSE_SYNC
}

Then khugepaged and MADV_COLLAPSE get their dedicated node_load respectively.

The more aggressive approach may be just killing node_load, but I'm not sure what impact it may incur.

Quoted 37 lines

Thanks,
Zach

Quoted 33 lines

vim +2284 mm/khugepaged.c

b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2261
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2262  static int khugepaged(void *none)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2263  {
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2264      struct mm_slot *mm_slot;
d87b6065d6050b Zach O'Keefe       2022-06-03  2265      struct collapse_control cc = {
d87b6065d6050b Zach O'Keefe       2022-06-03  2266              .last_target_node = NUMA_NO_NODE,
d87b6065d6050b Zach O'Keefe       2022-06-03  2267      };
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2268
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2269      set_freezable();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2270      set_user_nice(current, MAX_NICE);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2271
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2272      while (!kthread_should_stop()) {
d87b6065d6050b Zach O'Keefe       2022-06-03  2273              khugepaged_do_scan(&cc);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2274              khugepaged_wait_work();
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2275      }
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2276
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2277      spin_lock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2278      mm_slot = khugepaged_scan.mm_slot;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2279      khugepaged_scan.mm_slot = NULL;
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2280      if (mm_slot)
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2281              collect_mm_slot(mm_slot);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2282      spin_unlock(&khugepaged_mm_lock);
b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2283      return 0;

b46e756f5e4703 Kirill A. Shutemov 2016-07-26 @2284 } b46e756f5e4703 Kirill A. Shutemov 2016-07-26 2285

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Quoted 41 lines · On Mon, 6 Jun 2022 09:40:20 -0700 "Zach O'Keefe" <[email protected]> wrote:

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Quoted 3 lines

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Adding 4k to the stack isn't a good thing to do. It's trivial to kmalloc the thing, so why not do that?

I'll await some reviewer input (hopefully positive ;)) before merging this series.

Quoted 44 lines · Andrew Morton

Quoted 41 lines · On Mon, 6 Jun 2022 09:40:20 -0700 "Zach O'Keefe" <[email protected]> wrote:

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Quoted 6 lines

Quoted 3 lines

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Adding 4k to the stack isn't a good thing to do. It's trivial to kmalloc the thing, so why not do that?

Thanks, Andrew. Yeah, I just suggested that too.

Quoted 3 lines

I'll await some reviewer input (hopefully positive ;)) before merging this series.

Quoted 46 lines · Yang Shi

Quoted 44 lines · Andrew Morton

Quoted 41 lines · On Mon, 6 Jun 2022 09:40:20 -0700 "Zach O'Keefe" <[email protected]> wrote:

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Adding 4k to the stack isn't a good thing to do. It's trivial to kmalloc the thing, so why not do that?

Thanks, Andrew. Yeah, I just suggested that too.

Thanks Yang / Andrew for taking the time to voice your suggestions.

I'll go ahead and just kmalloc() the thing and fail if we can't.

Yang, is there a reason to kmalloc() the entire struct collapse_control with trailing flex array vs stack allocating the struct collapse_control + kmalloc()'ing the node_load array?

Quoted 5 lines

Quoted 5 lines

I'll await some reviewer input (hopefully positive ;)) before merging this series.

Quoted 57 lines · Zach O'Keefe

Quoted 46 lines · Yang Shi

Quoted 44 lines · Andrew Morton

Quoted 41 lines · On Mon, 6 Jun 2022 09:40:20 -0700 "Zach O'Keefe" <[email protected]> wrote:

Quoted 21 lines · kernel test robot

Hi Zach,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
        git checkout d87b6065d6050b89930cca0814921aca7c269286
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable

Quoted 8 lines

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

All warnings (new ones prefixed by >>):

mm/khugepaged.c: In function 'khugepaged':

Quoted 1 line

Quoted 1 line

mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]

    2284 | }
         | ^

Thanks lkp(a)intel.com.

This is due to config with:

CONFIG_FRAME_WARN=2048
CONFIG_NODES_SHIFT=10

Where struct collapse_control has a member int node_load[MAX_NUMNODES], and we stack allocate one.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Is this a configuration that needs to be supported? 1024 nodes seems like a lot and I'm not sure if these configs are randomly generated or are reminiscent of real systems.

Adding 4k to the stack isn't a good thing to do. It's trivial to kmalloc the thing, so why not do that?

Thanks, Andrew. Yeah, I just suggested that too.

Quoted 7 lines

Thanks Yang / Andrew for taking the time to voice your suggestions.

I'll go ahead and just kmalloc() the thing and fail if we can't.

Yang, is there a reason to kmalloc() the entire struct collapse_control with trailing flex array vs stack allocating the struct collapse_control + kmalloc()'ing the node_load array?

I don't think those two have too much difference. I don't have a strong preference personally. However you could choose:

Define collapse_control as:

struct collapse_control {
    xxx;
    ...
    int node_load[MAX_NUMANODES];
}

Then you could kmalloc the whole struct.

Or it could be defined as:

struct collapse_control {
    xxx;
    ...
    int *node_load[];
}

In this way you could allocate collapse_control on stack or by kmalloc, then kmalloc node_load for all possible nodes instead of MAX_NUMANODES. This may have a better success rate since you do kmalloc much less memory (typically the number of possible nodes is much less than MAX_NUMANODES), but it may be not worth it since the error handling path is more complicated and it may not make too much difference.

The first choice is definitely much simpler, you may want to try that first.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I'll await some reviewer input (hopefully positive ;)) before merging this series.

The following code is duplicated in collapse_huge_page() and collapse_file():

        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);

Also, "node" is passed as an argument to both collapse_huge_page() and collapse_file() and obtained the same way, via khugepaged_find_target_node().

Move all this into a new helper, alloc_charge_hpage(), and remove the duplicate code from collapse_huge_page() and collapse_file(). Also, simplify khugepaged_alloc_page() by returning a bool indicating allocation success instead of a copy of the allocated struct page.

Suggested-by Peter Xu <[email protected]>

---

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

---
 mm/khugepaged.c | 77 ++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 43 deletions(-)

mm/khugepaged.c +34 -44

index 907d0b2bd4bd..38488d114073 100644
@@ -860,19 +860,18 @@ static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
        return false;
 }
 
-static struct page *
-khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
+static bool khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
 {
        *hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
        if (unlikely(!*hpage)) {
                count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
                *hpage = ERR_PTR(-ENOMEM);
-               return NULL;
+               return false;
        }
 
        prep_transhuge_page(*hpage);
        count_vm_event(THP_COLLAPSE_ALLOC);
-       return *hpage;
+       return true;
 }
 
 /*

@@ -995,10 +994,23 @@ 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 int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
+                             struct collapse_control *cc)
+{
+       gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+       int node = khugepaged_find_target_node(cc);
+
+       if (!khugepaged_alloc_page(hpage, gfp, node))
+               return SCAN_ALLOC_HUGE_PAGE_FAIL;
+       if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, gfp)))
+               return SCAN_CGROUP_CHARGE_FAIL;
+       count_memcg_page_event(*hpage, THP_COLLAPSE_ALLOC);
+       return SCAN_SUCCEED;
+}
+
+static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
+                              struct page **hpage, int referenced,
+                              int unmapped, struct collapse_control *cc)
 {
        LIST_HEAD(compound_pagelist);
        pmd_t *pmd, _pmd;

@@ -1009,13 +1021,9 @@ static void collapse_huge_page(struct mm_struct *mm,
        int isolated = 0, result = 0;
        struct vm_area_struct *vma;
        struct mmu_notifier_range range;
-       gfp_t gfp;
 
        VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-       /* Only allocate from the target node */
-       gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
-
        /*
         * Before allocating the hugepage, release the mmap_lock read lock.
         * The allocation can take potentially a long time if it involves

@@ -1023,17 +1031,12 @@ static void collapse_huge_page(struct mm_struct *mm,
         * that. We will recheck the vma after taking it again in write mode.
         */
        mmap_read_unlock(mm);
-       new_page = khugepaged_alloc_page(hpage, gfp, node);
-       if (!new_page) {
-               result = SCAN_ALLOC_HUGE_PAGE_FAIL;
-               goto out_nolock;
-       }
 
-       if (unlikely(mem_cgroup_charge(page_folio(new_page), mm, gfp))) {
-               result = SCAN_CGROUP_CHARGE_FAIL;
+       result = alloc_charge_hpage(hpage, mm, cc);
+       if (result != SCAN_SUCCEED)
                goto out_nolock;
-       }
-       count_memcg_page_event(new_page, THP_COLLAPSE_ALLOC);
+
+       new_page = *hpage;
 
        mmap_read_lock(mm);
        result = hugepage_vma_revalidate(mm, address, &vma);

@@ -1306,10 +1309,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm, struct vm_area_struct *vma,
 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, hpage, referenced, unmapped,
+                                  cc);
        }
 out:
        trace_mm_khugepaged_scan_pmd(mm, page, writable, referenced,

@@ -1578,7 +1580,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  * @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;

@@ -1595,12 +1597,11 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  *    + restore gaps in the page cache;
  *    + unlock and free huge page;
  */
-static void collapse_file(struct mm_struct *mm,
-               struct file *file, pgoff_t start,
-               struct page **hpage, int node)
+static void collapse_file(struct mm_struct *mm, struct file *file,
+                         pgoff_t start, struct page **hpage,
+                         struct collapse_control *cc)
 {
        struct address_space *mapping = file->f_mapping;
-       gfp_t gfp;
        struct page *new_page;
        pgoff_t index, end = start + HPAGE_PMD_NR;
        LIST_HEAD(pagelist);

@@ -1612,20 +1613,11 @@ static void collapse_file(struct mm_struct *mm,
        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;
-
-       new_page = khugepaged_alloc_page(hpage, gfp, node);
-       if (!new_page) {
-               result = SCAN_ALLOC_HUGE_PAGE_FAIL;
+       result = alloc_charge_hpage(hpage, mm, cc);
+       if (result != SCAN_SUCCEED)
                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);
+       new_page = *hpage;
 
        /*
         * Ensure we have slots for all the pages in the range.  This is

@@ -2037,8 +2029,7 @@ static void khugepaged_scan_file(struct mm_struct *mm, struct file *file,
                        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, hpage, cc);
                }
        }
 
--

2.36.1.255.ge46751e96f-goog

The following code is duplicated in collapse_huge_page() and collapse_file():

        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);

Also, "node" is passed as an argument to both collapse_huge_page() and collapse_file() and obtained the same way, via khugepaged_find_target_node().

Move all this into a new helper, alloc_charge_hpage(), and remove the duplicate code from collapse_huge_page() and collapse_file(). Also, simplify khugepaged_alloc_page() by returning a bool indicating allocation success instead of a copy of the allocated struct page.

Suggested-by Peter Xu <[email protected]>

---

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

Quoted 26 lines · Zach O'Keefe

The following code is duplicated in collapse_huge_page() and collapse_file():

        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);

Also, "node" is passed as an argument to both collapse_huge_page() and collapse_file() and obtained the same way, via khugepaged_find_target_node().

Move all this into a new helper, alloc_charge_hpage(), and remove the duplicate code from collapse_huge_page() and collapse_file(). Also, simplify khugepaged_alloc_page() by returning a bool indicating allocation success instead of a copy of the allocated struct page.

Quoted 3 lines

Suggested-by Peter Xu <[email protected]>

---

[note: please remember to drop this "---" when repost since I think it
could drop your sign-off when apply]

Quoted 2 lines

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

Reviewed-by Peter Xu <[email protected]>

Thanks,

Signature · 1 line

Peter Xu

Add a gfp_t flags member to struct collapse_control that allows contexts to specify their own allocation semantics. This decouples the allocation semantics from /sys/kernel/mm/transparent_hugepage/khugepaged/defrag.

khugepaged updates this member for every hugepage processed, since the sysfs setting might change at any time.

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

---
 mm/khugepaged.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

mm/khugepaged.c +3 -0

index 38488d114073..ba722347bebd 100644
@@ -92,6 +92,9 @@ struct collapse_control {
 
        /* Last target selected in khugepaged_find_target_node() */
        int last_target_node;
+
+       /* gfp used for allocation and memcg charging */
+       gfp_t gfp;
 };
 
 /**

mm/khugepaged.c +4 -5

                              struct collapse_control *cc)
 {
-       gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
        int node = khugepaged_find_target_node(cc);
 
-       if (!khugepaged_alloc_page(hpage, gfp, node))
+       if (!khugepaged_alloc_page(hpage, cc->gfp, node))
                return SCAN_ALLOC_HUGE_PAGE_FAIL;
-       if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, gfp)))
+       if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, cc->gfp)))
                return SCAN_CGROUP_CHARGE_FAIL;
        count_memcg_page_event(*hpage, THP_COLLAPSE_ALLOC);
        return SCAN_SUCCEED;

@@ -1032,7 +1034,7 @@ static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
         */
        mmap_read_unlock(mm);
 
-       result = alloc_charge_hpage(hpage, mm, cc);
+       result = alloc_charge_hpage(mm, hpage, cc);
        if (result != SCAN_SUCCEED)
                goto out_nolock;
 

@@ -1613,7 +1615,7 @@ static void collapse_file(struct mm_struct *mm, struct file *file,
        VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
        VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
 
-       result = alloc_charge_hpage(hpage, mm, cc);
+       result = alloc_charge_hpage(mm, hpage, cc);
        if (result != SCAN_SUCCEED)
                goto out;
 

mm/khugepaged.c +3 -0

 {
        BUILD_BUG();
 }

@@ -2121,6 +2122,9 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
                        if (unlikely(khugepaged_test_exit(mm)))
                                goto breakouterloop;
 
+                       /* reset gfp flags since sysfs settings might change */
+                       cc->gfp = alloc_hugepage_khugepaged_gfpmask() |
+                                       __GFP_THISNODE;
                        VM_BUG_ON(khugepaged_scan.address < hstart ||
                                  khugepaged_scan.address + HPAGE_PMD_SIZE >
                                  hend);

mm/khugepaged.c +0 -1

        };
 
        set_freezable();
--

2.36.1.255.ge46751e96f-goog

Quoted 32 lines · Zach O'Keefe

Add a gfp_t flags member to struct collapse_control that allows contexts to specify their own allocation semantics. This decouples the allocation semantics from /sys/kernel/mm/transparent_hugepage/khugepaged/defrag.

khugepaged updates this member for every hugepage processed, since the sysfs setting might change at any time.

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

---
 mm/khugepaged.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

mm/khugepaged.c +3 -0

index 38488d114073..ba722347bebd 100644
@@ -92,6 +92,9 @@ struct collapse_control {
 
        /* Last target selected in khugepaged_find_target_node() */
        int last_target_node;
+
+       /* gfp used for allocation and memcg charging */
+       gfp_t gfp;
 };
 
 /**

@@ -994,15 +997,14 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
        return true;
 }
 

mm/khugepaged.c static bool __collapse_huge_page_swapin(struct mm_struct *mm, · line 1000

Quoted 2 lines

mm/khugepaged.c +1 -1

-static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
+static int alloc_charge_hpage(struct mm_struct *mm, struct page **hpage,

Why did you have to reverse the order of mm and hpage? It seems pointless and you could save a couple of changed lines.

Quoted 31 lines

mm/khugepaged.c +4 -5

                              struct collapse_control *cc)
 {
-       gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
        int node = khugepaged_find_target_node(cc);
 
-       if (!khugepaged_alloc_page(hpage, gfp, node))
+       if (!khugepaged_alloc_page(hpage, cc->gfp, node))
                return SCAN_ALLOC_HUGE_PAGE_FAIL;
-       if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, gfp)))
+       if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, cc->gfp)))
                return SCAN_CGROUP_CHARGE_FAIL;
        count_memcg_page_event(*hpage, THP_COLLAPSE_ALLOC);
        return SCAN_SUCCEED;

@@ -1032,7 +1034,7 @@ static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
         */
        mmap_read_unlock(mm);
 
-       result = alloc_charge_hpage(hpage, mm, cc);
+       result = alloc_charge_hpage(mm, hpage, cc);
        if (result != SCAN_SUCCEED)
                goto out_nolock;
 

@@ -1613,7 +1615,7 @@ static void collapse_file(struct mm_struct *mm, struct file *file,
        VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
        VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
 
-       result = alloc_charge_hpage(hpage, mm, cc);
+       result = alloc_charge_hpage(mm, hpage, cc);
        if (result != SCAN_SUCCEED)
                goto out;
 

mm/khugepaged.c static void khugepaged_scan_file(struct mm_struct *mm, struct file *file, · line 2042

Quoted 7 lines

mm/khugepaged.c +1 -2

@@ -2037,8 +2039,7 @@ static void khugepaged_scan_file(struct mm_struct *mm, struct file *file,
 }
 #else
 static void khugepaged_scan_file(struct mm_struct *mm, struct file *file,
-                                pgoff_t start, struct page **hpage,
-                                struct collapse_control *cc)
+                                pgoff_t start, struct collapse_control *cc)

Why was the !CONFIG_SHMEM version definition changed, but CONFIG_SHMEM version was not?

Quoted 7 lines

mm/khugepaged.c +0 -0

 {
        BUILD_BUG();
 }

@@ -2121,6 +2122,9 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
                        if (unlikely(khugepaged_test_exit(mm)))
                                goto breakouterloop;
 

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

Quoted 11 lines

mm/khugepaged.c +4 -0

+                       /* reset gfp flags since sysfs settings might change */
+                       cc->gfp = alloc_hugepage_khugepaged_gfpmask() |
+                                       __GFP_THISNODE;
                        VM_BUG_ON(khugepaged_scan.address < hstart ||
                                  khugepaged_scan.address + HPAGE_PMD_SIZE >
                                  hend);

@@ -2255,6 +2259,7 @@ static int khugepaged(void *none)
        struct mm_slot *mm_slot;
        struct collapse_control cc = {
                .last_target_node = NUMA_NO_NODE,
+               /* .gfp set later  */

Seems pointless to me.

Quoted 6 lines

mm/khugepaged.c +0 -1

        };
 
        set_freezable();
--

2.36.1.255.ge46751e96f-goog