mm: userspace hugepage collapse

Review

Quoted 11 lines · Zach O'Keefe

(quoted diff) +11 -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);

It seems to be correct on using the atomic fetcher here. Though irrelevant to this patchset.. does it also mean that we miss that on mm_find_pmd()? I meant a separate fix like this one:

---8<---

mm/rmap.c +1 -2

index 69416072b1a6..61309718640f 100644
@@ -785,7 +785,7 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
         * without holding anon_vma lock for write.  So when looking for a
         * genuine pmde (in which to find pte), test present and !THP together.
         */
-       pmde = *pmd;
+       pmde = pmd_read_atomic(pmd);
        barrier();
        if (!pmd_present(pmde) || pmd_trans_huge(pmde))
                pmd = NULL;
---8<---

As otherwise it seems it's also prone to PAE race conditions when reading pmd out, but I could be missing something.

Quoted 6 lines

(quoted diff) +6 -0

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

Review

Quoted 3 lines

(quoted diff) +3 -0

+             return SCAN_PMD_NULL;
+     if (pmd_trans_huge(pmde))
+             return SCAN_PMD_MAPPED;

Would it be safer to check pmd_bad()? I think not all mm pmd paths check that, frankly I don't really know what's the major cause of a bad pmd (either software bugs or corrupted mem), but just to check with you, because potentially a bad pmd can be read as SCAN_SUCCEED and go through.

Review

Quoted 2 lines

(quoted diff) +2 -0

+     return SCAN_SUCCEED;
+}

The rest looks good to me, thanks.

--
Peter Xu

Thanks again for the review, Peter.

Quoted 34 lines · Peter Xu

Review

Quoted 11 lines · Zach O'Keefe

(quoted diff) +11 -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);

It seems to be correct on using the atomic fetcher here. Though irrelevant to this patchset.. does it also mean that we miss that on mm_find_pmd()? I meant a separate fix like this one:

---8<---

mm/rmap.c +1 -2

index 69416072b1a6..61309718640f 100644
@@ -785,7 +785,7 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
         * without holding anon_vma lock for write.  So when looking for a
         * genuine pmde (in which to find pte), test present and !THP together.
         */
-       pmde = *pmd;
+       pmde = pmd_read_atomic(pmd);
        barrier();
        if (!pmd_present(pmde) || pmd_trans_huge(pmde))
                pmd = NULL;
---8<---

Quoted 3 lines

As otherwise it seems it's also prone to PAE race conditions when reading pmd out, but I could be missing something.

This is a good question. I took some time to look into this, but it's very complicated and unfortunately I couldn't reach a conclusion in the time I allotted to myself. My working (unverified) assumption is that mm_find_pmd() is called in places where it doesn't care if the pmd isn't read atomically.

If so, does that also mean MADV_COLLAPSE is
safe? I'm not sure. These i386 PAE + THP racing issues were most recently discussed when considering if READ_ONCE() should be used instead of pmd_read_atomic() [1].

[1] https://lore.kernel.org/linux-mm/[email protected]/

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

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

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+             return SCAN_PMD_NULL;
+     if (pmd_trans_huge(pmde))
+             return SCAN_PMD_MAPPED;

Would it be safer to check pmd_bad()? I think not all mm pmd paths check that, frankly I don't really know what's the major cause of a bad pmd (either software bugs or corrupted mem), but just to check with you, because potentially a bad pmd can be read as SCAN_SUCCEED and go through.

Likewise, I'm not sure what the cause of "bad pmds" is.

Do you mean to check pmd_bad() instead of pmd_trans_huge()? I.e. b/c a pmd-mapped thp counts as "bad" (at least on x86 since PSE set) or do you mean to additionally check pmd_bad() after the pmd_trans_huge() check?

If it's the former, I'd say we can't claim !pmd_bad() == memory already backed by thps / our job here is done.

If it's the latter, I don't see it hurting much (but I can't argue intelligently about why it's needed) and can include the check in v6.

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.

Start by moving global per-node khugepaged statistics into this new structure, and stack allocate one for khugepaged collapse context.

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

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.

Start by moving global per-node khugepaged statistics into this new structure, and stack allocate one for khugepaged collapse context.

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

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

Start by moving global per-node khugepaged statistics into this new structure, and stack allocate one for khugepaged collapse context.

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

Reviewed-by Peter Xu <[email protected]>

There's already some UMA changes that seems to be not needed if you're going with Yang's UMA patch as pre-requisite. And I also see that there's some other changes to settle and rebase (e.g. -mm temporarily removed the khugepaged patch to move the thread around in next patch), so I figured maybe I'll just stop here and read you next version.

Thanks,

--
Peter Xu

mm/khugepaged.c +14 -4

@@ -1069,10 +1067,34 @@ 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)
+{
+#ifdef CONFIG_NUMA
+	const struct cpumask *cpumask;
+#endif
+	gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+	int node = khugepaged_find_target_node(cc);
+
+#ifdef CONFIG_NUMA
+	/* sched to specified node before huge page memory copy */
+	if (task_node(current) != node) {
+		cpumask = cpumask_of_node(node);
+		if (!cpumask_empty(cpumask))

Quoted 27 lines · Zach O'Keefe

mm/khugepaged.c +19 -4

@@ -1069,10 +1067,34 @@ 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)
+{
+#ifdef CONFIG_NUMA
+     const struct cpumask *cpumask;
+#endif
+     gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
+     int node = khugepaged_find_target_node(cc);
+
+#ifdef CONFIG_NUMA
+     /* sched to specified node before huge page memory copy */
+     if (task_node(current) != node) {
+             cpumask = cpumask_of_node(node);
+             if (!cpumask_empty(cpumask))
+                     set_cpus_allowed_ptr(current, cpumask);
+     }
+#endif
+     if (!khugepaged_alloc_page(hpage, gfp, node))
+             return SCAN_ALLOC_HUGE_PAGE_FAIL;

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

Quoted 5 lines

mm/khugepaged.c +5 -0

+     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;
+}

Lots of ifdefs here, I wonder if we can define a helper function that is a no-op when CONFIG_NUMA is disabled that we can call into here instead.

Otherwise this looks like a nice unification. After this is cleaned up feel free to add

Acked-by David Rientjes <[email protected]>

mm/khugepaged.c +0 -0

index c94bc43dff3e..6095fcb3f07c 100644

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

Quoted 41 lines · Zach O'Keefe

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Quoted 8 lines

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

I had a patch that removed UMA allocation, please refer to
https://lore.kernel.org/linux-mm/[email protected]/#t

It was not made upstream due to some requests for further cleanup, but unfortunately I haven't got time to look into it yet.

If this page were merged, would that make your life easier?

Quoted 52 lines · Yang Shi

Quoted 41 lines · Zach O'Keefe

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Quoted 8 lines

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

Quoted 7 lines

I had a patch that removed UMA allocation, please refer to
https://lore.kernel.org/linux-mm/[email protected]/#t

It was not made upstream due to some requests for further cleanup, but unfortunately I haven't got time to look into it yet.

If this page were merged, would that make your life easier?

Hey Yang,

First, sorry for missing that patch in the first place. I actually have some patches queued up that do a similar cleanup of khugepaged_prealloc_page() that was mentioned, but decided to not include them here.

Second, removing the NUMA/UMA story does make this patch easier, I think (esp since the sched change was dropped for now). This is something I wanted while writing this series, but without the larger context referenced in your patch (most users don't build NUMA=n even on single node systems, and the pcp hugepage lists optimization) couldn't justify myself.

Best,
Zach

Quoted 69 lines · Zach O'Keefe

Quoted 52 lines · Yang Shi

Quoted 41 lines · Zach O'Keefe

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Quoted 8 lines

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

Quoted 7 lines

I had a patch that removed UMA allocation, please refer to
https://lore.kernel.org/linux-mm/[email protected]/#t

It was not made upstream due to some requests for further cleanup, but unfortunately I haven't got time to look into it yet.

If this page were merged, would that make your life easier?

Hey Yang,

First, sorry for missing that patch in the first place. I actually have some patches queued up that do a similar cleanup of khugepaged_prealloc_page() that was mentioned, but decided to not include them here.

Quoted 6 lines

Second, removing the NUMA/UMA story does make this patch easier, I think (esp since the sched change was dropped for now). This is something I wanted while writing this series, but without the larger context referenced in your patch (most users don't build NUMA=n even on single node systems, and the pcp hugepage lists optimization) couldn't justify myself.

Thanks, it would be better to add this patch into your series as a prerequisite so that you could make the MADV_COLLAPSE easier.

I don't think I will be able to find time to rework the patch and solve all the review comments at any time soon. If you'd like to take it, please do it.

Quoted 3 lines

Best,
Zach

Quoted 81 lines · Yang Shi

Quoted 69 lines · Zach O'Keefe

Quoted 52 lines · Yang Shi

Quoted 41 lines · Zach O'Keefe

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Quoted 8 lines

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

Quoted 7 lines

I had a patch that removed UMA allocation, please refer to
https://lore.kernel.org/linux-mm/[email protected]/#t

It was not made upstream due to some requests for further cleanup, but unfortunately I haven't got time to look into it yet.

If this page were merged, would that make your life easier?

Hey Yang,

First, sorry for missing that patch in the first place. I actually have some patches queued up that do a similar cleanup of khugepaged_prealloc_page() that was mentioned, but decided to not include them here.

Quoted 6 lines

Second, removing the NUMA/UMA story does make this patch easier, I think (esp since the sched change was dropped for now). This is something I wanted while writing this series, but without the larger context referenced in your patch (most users don't build NUMA=n even on single node systems, and the pcp hugepage lists optimization) couldn't justify myself.

Thanks, it would be better to add this patch into your series as a prerequisite so that you could make the MADV_COLLAPSE easier.

Quoted 4 lines

I don't think I will be able to find time to rework the patch and solve all the review comments at any time soon. If you'd like to take it, please do it.

Sounds good, Yang. I'll take a crack at it for v6.

Thanks for taking the time,
Zach

Quoted 3 lines

Quoted 3 lines

Best,
Zach

Quoted 88 lines · Zach O'Keefe

Quoted 85 lines · Yang Shi

Quoted 69 lines · Zach O'Keefe

Quoted 52 lines · Yang Shi

Quoted 41 lines · Zach O'Keefe

Thanks for the review, David!

Quoted 17 lines · David Rientjes

Quoted 14 lines · Zach O'Keefe

mm/khugepaged.c +4 -0

index c94bc43dff3e..6095fcb3f07c 100644
@@ -92,6 +92,10 @@ struct collapse_control {
 
      /* Last target selected in khugepaged_find_target_node() */
      int last_target_node;
+
+     struct page *hpage;
+     int (*alloc_charge_hpage)(struct mm_struct *mm,
+                               struct collapse_control *cc);
 };
 

Quoted 8 lines

mm/khugepaged.c struct collapse_control { · line 101

Quoted 1 line

mm/khugepaged.c +0 -0

 /**

Embedding this function pointer into collapse_contol seems like it would need some pretty strong rationale. Not to say that it should be a non-starter, but I think the changelog needs to clearly indicate why this is better/cleaner than embedding the needed info for a single allocation and charge function to use. If the callbacks would truly be so different that unifying them would be more complex, I think this makes sense.

Mostly, this boils down to khugepaged having different a allocation pattern for NUMA/UMA ; the former scans the pages first to determine the right node, the latter preallocates before scanning. khugepaged has the luxury on UMA systems of just holding onto a hugepage indefinitely for the next collapse target.

For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ... else" casing, defining this as a context-defined operation seemed appropriate.

Quoted 8 lines

Collapsing both alloc and charging together was mostly a code cleanliness decision resulting from not wanting to embed a ->gfp() hook (gfp flags are used both by allocation and memcg charging). Alternatively, a .gfp member could exist - it would just need to be refreshed periodically in the khugepaged codepath.

That all said - let me take another crack at seeing if I can make this work without the need for a function pointer here.

Quoted 7 lines

I had a patch that removed UMA allocation, please refer to
https://lore.kernel.org/linux-mm/[email protected]/#t

It was not made upstream due to some requests for further cleanup, but unfortunately I haven't got time to look into it yet.

If this page were merged, would that make your life easier?

Hey Yang,

First, sorry for missing that patch in the first place. I actually have some patches queued up that do a similar cleanup of khugepaged_prealloc_page() that was mentioned, but decided to not include them here.

Quoted 6 lines

Second, removing the NUMA/UMA story does make this patch easier, I think (esp since the sched change was dropped for now). This is something I wanted while writing this series, but without the larger context referenced in your patch (most users don't build NUMA=n even on single node systems, and the pcp hugepage lists optimization) couldn't justify myself.

Thanks, it would be better to add this patch into your series as a prerequisite so that you could make the MADV_COLLAPSE easier.

I don't think I will be able to find time to rework the patch and solve all the review comments at any time soon. If you'd like to take it, please do it.

Quoted 1 line

Sounds good, Yang. I'll take a crack at it for v6.

Hi Zach,

Fortunately I got some time, if you haven't spent too much effort in this, I could rework the patch then resubmit. Just let me know.

Quoted 7 lines

Thanks for taking the time,
Zach

Quoted 3 lines

Quoted 3 lines

Best,
Zach

Pipe enum scan_result codes back through return values of functions downstream of khugepaged_scan_file() and khugepaged_scan_pmd() to inform callers if the operation was successful, and if not, why.

Since khugepaged_scan_pmd()'s return value already has a specific meaning (whether mmap_lock was unlocked or not), add a bool* argument to khugepaged_scan_pmd() to retrieve this information.

Change khugepaged to take action based on the return values of khugepaged_scan_file() and khugepaged_scan_pmd() instead of acting deep within the collapsing functions themselves.

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

Add enforce_pte_scan_limits flag to struct collapse_control that allows context to ignore the sysfs-controlled knobs khugepaged_max_ptes_[none|swap|shared] and set this flag in khugepaged collapse context to preserve existing khugepaged behavior.

This flag will be used (unset) when introducing madvise collapse context since here, the user presumably has reason to believe the collapse will be beneficial and khugepaged heuristics shouldn't tell the user they are wrong.

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

Add enforce_young flag to struct collapse_control that allows context to ignore requirement that some pages in region being collapsed be young or referenced. Set this flag in khugepaged collapse context to preserve existing khugepaged behavior.

This flag will be used (unset) when introducing madvise collapse context since here, the user presumably has reason to believe the collapse will be beneficial and khugepaged heuristics shouldn't tell the user they are wrong.

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

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

mm/khugepaged.c +0 -0

index ca730aec0e3e..b14807b7002e 100644

Quoted 23 lines · Zach O'Keefe

Add enforce_young flag to struct collapse_control that allows context to ignore requirement that some pages in region being collapsed be young or referenced. Set this flag in khugepaged collapse context to preserve existing khugepaged behavior.

This flag will be used (unset) when introducing madvise collapse context since here, the user presumably has reason to believe the collapse will be beneficial and khugepaged heuristics shouldn't tell the user they are wrong.

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

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

mm/khugepaged.c +0 -0

index ca730aec0e3e..b14807b7002e 100644
@@ -90,6 +90,9 @@ struct collapse_control {
      /* Respect khugepaged_max_ptes_[none|swap|shared] */
      bool enforce_pte_scan_limits;
 

mm/khugepaged.c struct collapse_control { · line 98

Quoted 6 lines

mm/khugepaged.c +3 -0

+     /* Require memory to be young */
+     bool enforce_young;
+
      /* Num pages scanned per node */
      int node_load[MAX_NUMNODES];
 

Small nit: is it possible to unify these into a single bool khugepaged flag?

Either way:

Acked-by David Rientjes <[email protected]>

This idea was introduced by David Rientjes[1].

Introduce a new madvise mode, MADV_COLLAPSE, that allows users to request a synchronous collapse of memory at their own expense.

The benefits of this approach are:

* CPU is charged to the process that wants to spend the cycles for the THP
* Avoid unpredictable timing of khugepaged collapse

An immediate user of this new functionality are malloc() implementations that manage memory in hugepage-sized chunks, but sometimes subrelease memory back to the system in native-sized chunks via MADV_DONTNEED; zapping the pmd. Later, when the memory is hot, the implementation could madvise(MADV_COLLAPSE) to re-back the memory by THPs to regain hugepage coverage and dTLB performance. TCMalloc is such an implementation that could benefit from this[2].

Only privately-mapped anon memory is supported for now, but it is expected that file and shmem support will be added later to support the use-case of backing executable text by THPs. Current support provided by CONFIG_READ_ONLY_THP_FOR_FS may take a long time on a large system which might impair services from serving at their full rated load after (re)starting. Tricks like mremap(2)'ing text onto anonymous memory to immediately realize iTLB performance prevents page sharing and demand paging, both of which increase steady state memory footprint. With MADV_COLLAPSE, we get the best of both worlds: Peak upfront performance and lower RAM footprints.

This call respects THP eligibility as determined by the system-wide /sys/kernel/mm/transparent_hugepage/enabled sysfs settings and the VMA flags for the memory range being collapsed.

THP allocation may enter direct reclaim and/or compaction.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://github.com/google/tcmalloc/tree/master/tcmalloc

Suggested-by David Rientjes <[email protected]>

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

---
 arch/alpha/include/uapi/asm/mman.h     |   2 +
 arch/mips/include/uapi/asm/mman.h      |   2 +
 arch/parisc/include/uapi/asm/mman.h    |   2 +
 arch/xtensa/include/uapi/asm/mman.h    |   2 +
 include/linux/huge_mm.h                |  12 ++
 include/uapi/asm-generic/mman-common.h |   2 +
 mm/khugepaged.c                        | 167 +++++++++++++++++++++++--
 mm/madvise.c                           |   5 +
 8 files changed, 182 insertions(+), 12 deletions(-)

arch/alpha/include/uapi/asm/mman.h +2 -0

index 4aa996423b0d..763929e814e9 100644
@@ -76,6 +76,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

arch/mips/include/uapi/asm/mman.h +2 -0

index 1be428663c10..c6e1fc77c996 100644
@@ -103,6 +103,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

arch/parisc/include/uapi/asm/mman.h +2 -0

index a7ea3204a5fa..22133a6a506e 100644
@@ -70,6 +70,8 @@
 #define MADV_WIPEONFORK 71             /* Zero memory on fork, child only */
 #define MADV_KEEPONFORK 72             /* Undo MADV_WIPEONFORK */
 
+#define MADV_COLLAPSE  73              /* Synchronous hugepage collapse */
+
 #define MADV_HWPOISON     100          /* poison a page for testing */
 #define MADV_SOFT_OFFLINE 101          /* soft offline page for testing */
 

arch/xtensa/include/uapi/asm/mman.h +2 -0

index 7966a58af472..1ff0c858544f 100644
@@ -111,6 +111,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

include/linux/huge_mm.h +12 -0

index 9a26bd10e083..4a2ea1b5437c 100644
@@ -222,6 +222,9 @@ void __split_huge_pud(struct vm_area_struct *vma, pud_t *pud,
 
 int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags,
                     int advice);
+int madvise_collapse(struct vm_area_struct *vma,
+                    struct vm_area_struct **prev,
+                    unsigned long start, unsigned long end);
 void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start,
                           unsigned long end, long adjust_next);
 spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma);

@@ -378,6 +381,15 @@ static inline int hugepage_madvise(struct vm_area_struct *vma,
        BUG();
        return 0;
 }
+
+static inline int madvise_collapse(struct vm_area_struct *vma,
+                                  struct vm_area_struct **prev,
+                                  unsigned long start, unsigned long end)
+{
+       BUG();
+       return 0;
+}
+
 static inline void vma_adjust_trans_huge(struct vm_area_struct *vma,
                                         unsigned long start,
                                         unsigned long end,

include/uapi/asm-generic/mman-common.h +2 -0

index 6c1aa92a92e4..6ce1f1ceb432 100644
@@ -77,6 +77,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

mm/khugepaged.c +87 -12

index b14807b7002e..165c646ddb8f 100644
@@ -837,6 +837,22 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
        return khugepaged_defrag() ? GFP_TRANSHUGE : GFP_TRANSHUGE_LIGHT;
 }
 
+static bool alloc_hpage(gfp_t gfp, int node, struct collapse_control *cc)
+{
+       VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
+
+       cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
+       if (unlikely(!cc->hpage)) {
+               count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
+               cc->hpage = ERR_PTR(-ENOMEM);
+               return false;
+       }
+
+       prep_transhuge_page(cc->hpage);
+       count_vm_event(THP_COLLAPSE_ALLOC);
+       return true;
+}
+
 #ifdef CONFIG_NUMA
 static int khugepaged_find_target_node(struct collapse_control *cc)
 {

@@ -882,18 +898,7 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
 static bool khugepaged_alloc_page(gfp_t gfp, int node,
                                  struct collapse_control *cc)
 {
-       VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
-
-       cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
-       if (unlikely(!cc->hpage)) {
-               count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
-               cc->hpage = ERR_PTR(-ENOMEM);
-               return false;
-       }
-
-       prep_transhuge_page(cc->hpage);
-       count_vm_event(THP_COLLAPSE_ALLOC);
-       return true;
+       return alloc_hpage(gfp, node, cc);
 }
 #else
 static int khugepaged_find_target_node(struct collapse_control *cc)

@@ -2457,3 +2462,141 @@ void khugepaged_min_free_kbytes_update(void)
                set_recommended_min_free_kbytes();
        mutex_unlock(&khugepaged_mutex);
 }
+
+static void madvise_collapse_cleanup_page(struct page **hpage)
+{
+       if (!IS_ERR(*hpage) && *hpage)
+               put_page(*hpage);
+       *hpage = NULL;
+}
+
+static int madvise_collapse_errno(enum scan_result r)
+{
+       switch (r) {
+       case SCAN_PMD_NULL:
+       case SCAN_ADDRESS_RANGE:
+       case SCAN_VMA_NULL:
+       case SCAN_PTE_NON_PRESENT:
+       case SCAN_PAGE_NULL:
+               /*
+                * Addresses in the specified range are not currently mapped,
+                * or are outside the AS of the process.
+                */
+               return -ENOMEM;
+       case SCAN_ALLOC_HUGE_PAGE_FAIL:
+       case SCAN_CGROUP_CHARGE_FAIL:
+               /* A kernel resource was temporarily unavailable. */
+               return -EAGAIN;
+       default:
+               return -EINVAL;
+       }
+}
+
+static int madvise_alloc_charge_hpage(struct mm_struct *mm,
+                                     struct collapse_control *cc)
+{
+       if (!alloc_hpage(GFP_TRANSHUGE, khugepaged_find_target_node(cc), cc))
+               return SCAN_ALLOC_HUGE_PAGE_FAIL;
+       if (unlikely(mem_cgroup_charge(page_folio(cc->hpage), mm,
+                                      GFP_TRANSHUGE)))
+               return SCAN_CGROUP_CHARGE_FAIL;
+       count_memcg_page_event(cc->hpage, THP_COLLAPSE_ALLOC);
+       return SCAN_SUCCEED;
+}
+
+int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
+                    unsigned long start, unsigned long end)
+{
+       struct collapse_control cc = {
+               .enforce_pte_scan_limits = false,
+               .enforce_young = false,
+               .last_target_node = NUMA_NO_NODE,
+               .hpage = NULL,
+               .alloc_charge_hpage = &madvise_alloc_charge_hpage,
+       };
+       struct mm_struct *mm = vma->vm_mm;
+       unsigned long hstart, hend, addr;
+       int thps = 0, nr_hpages = 0, result = SCAN_FAIL;
+       bool mmap_locked = true;
+
+       BUG_ON(vma->vm_start > start);
+       BUG_ON(vma->vm_end < end);
+
+       *prev = vma;
+
+       if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file)
+               return -EINVAL;
+
+       hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
+       hend = end & HPAGE_PMD_MASK;
+       nr_hpages = (hend - hstart) >> HPAGE_PMD_SHIFT;
+
+       if (hstart >= hend || !transparent_hugepage_active(vma))

mm/khugepaged.c +58 -0

+               if (unlikely(khugepaged_test_exit(mm))) {
+                       result = SCAN_ANY_PROCESS;
+                       break;
+               }
+
+               memset(cc.node_load, 0, sizeof(cc.node_load));
+               result = khugepaged_scan_pmd(mm, vma, addr, &mmap_locked, &cc);
+               if (!mmap_locked)
+                       *prev = NULL;  /* tell madvise we dropped mmap_lock */
+
+               switch (result) {
+               /* Whitelisted set of results where continuing OK */
+               case SCAN_SUCCEED:
+               case SCAN_PMD_MAPPED:
+                       ++thps;
+                       fallthrough;
+               case SCAN_PMD_NULL:
+               case SCAN_PTE_NON_PRESENT:
+               case SCAN_PTE_UFFD_WP:
+               case SCAN_PAGE_RO:
+               case SCAN_LACK_REFERENCED_PAGE:
+               case SCAN_PAGE_NULL:
+               case SCAN_PAGE_COUNT:
+               case SCAN_PAGE_LOCK:
+               case SCAN_PAGE_COMPOUND:
+                       break;
+               case SCAN_PAGE_LRU:
+                       lru_add_drain_all();
+                       goto retry;
+               default:
+                       /* Other error, exit */
+                       goto break_loop;
+               }
+               addr += HPAGE_PMD_SIZE;
+               if (addr >= hend)
+                       break;
+retry:
+               if (!mmap_locked) {
+                       mmap_read_lock(mm);
+                       mmap_locked = true;
+                       result = hugepage_vma_revalidate(mm, addr, &vma);
+                       if (result)
+                               goto out;
+               }
+               madvise_collapse_cleanup_page(&cc.hpage);
+       }
+
+break_loop:
+       /* madvise_walk_vmas() expects us to hold mmap_lock on return */
+       if (!mmap_locked)
+               mmap_read_lock(mm);
+out:
+       mmap_assert_locked(mm);
+       madvise_collapse_cleanup_page(&cc.hpage);
+       mmdrop(mm);
+
+       return thps == nr_hpages ? 0 : madvise_collapse_errno(result);
+}

mm/madvise.c +5 -1

index 5f4537511532..638517952bd2 100644
@@ -59,6 +59,7 @@ static int madvise_need_mmap_write(int behavior)
        case MADV_FREE:
        case MADV_POPULATE_READ:
        case MADV_POPULATE_WRITE:
+       case MADV_COLLAPSE:
                return 0;
        default:
                /* be safe, default to 1. list exceptions explicitly */

@@ -1054,6 +1055,8 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
                if (error)
                        goto out;
                break;
+       case MADV_COLLAPSE:
+               return madvise_collapse(vma, prev, start, end);
        }
 
        anon_name = anon_vma_name(vma);

@@ -1147,6 +1150,7 @@ madvise_behavior_valid(int behavior)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
        case MADV_HUGEPAGE:
        case MADV_NOHUGEPAGE:
+       case MADV_COLLAPSE:
 #endif
        case MADV_DONTDUMP:
        case MADV_DODUMP:

@@ -1336,6 +1340,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
  *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
  *             transparent huge pages so the existing pages will not be
  *             coalesced into THP and new pages will not be allocated as THP.
+ *  MADV_COLLAPSE - synchronously coalesce pages into new THP.
  *  MADV_DONTDUMP - the application wants to prevent pages in the given range
  *             from being included in its core dump.
  *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
--

2.36.0.464.gb9c8b46e94-goog

Quoted 275 lines · Zach O'Keefe

This idea was introduced by David Rientjes[1].

Introduce a new madvise mode, MADV_COLLAPSE, that allows users to request a synchronous collapse of memory at their own expense.

The benefits of this approach are:

* CPU is charged to the process that wants to spend the cycles for the THP
* Avoid unpredictable timing of khugepaged collapse

An immediate user of this new functionality are malloc() implementations that manage memory in hugepage-sized chunks, but sometimes subrelease memory back to the system in native-sized chunks via MADV_DONTNEED; zapping the pmd. Later, when the memory is hot, the implementation could madvise(MADV_COLLAPSE) to re-back the memory by THPs to regain hugepage coverage and dTLB performance. TCMalloc is such an implementation that could benefit from this[2].

Only privately-mapped anon memory is supported for now, but it is expected that file and shmem support will be added later to support the use-case of backing executable text by THPs. Current support provided by CONFIG_READ_ONLY_THP_FOR_FS may take a long time on a large system which might impair services from serving at their full rated load after (re)starting. Tricks like mremap(2)'ing text onto anonymous memory to immediately realize iTLB performance prevents page sharing and demand paging, both of which increase steady state memory footprint. With MADV_COLLAPSE, we get the best of both worlds: Peak upfront performance and lower RAM footprints.

This call respects THP eligibility as determined by the system-wide /sys/kernel/mm/transparent_hugepage/enabled sysfs settings and the VMA flags for the memory range being collapsed.

THP allocation may enter direct reclaim and/or compaction.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://github.com/google/tcmalloc/tree/master/tcmalloc

Suggested-by David Rientjes <[email protected]>

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

---
 arch/alpha/include/uapi/asm/mman.h     |   2 +
 arch/mips/include/uapi/asm/mman.h      |   2 +
 arch/parisc/include/uapi/asm/mman.h    |   2 +
 arch/xtensa/include/uapi/asm/mman.h    |   2 +
 include/linux/huge_mm.h                |  12 ++
 include/uapi/asm-generic/mman-common.h |   2 +
 mm/khugepaged.c                        | 167 +++++++++++++++++++++++--
 mm/madvise.c                           |   5 +
 8 files changed, 182 insertions(+), 12 deletions(-)

arch/alpha/include/uapi/asm/mman.h +2 -0

index 4aa996423b0d..763929e814e9 100644
@@ -76,6 +76,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

arch/mips/include/uapi/asm/mman.h +2 -0

index 1be428663c10..c6e1fc77c996 100644
@@ -103,6 +103,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

arch/parisc/include/uapi/asm/mman.h +2 -0

index a7ea3204a5fa..22133a6a506e 100644
@@ -70,6 +70,8 @@
 #define MADV_WIPEONFORK 71             /* Zero memory on fork, child only */
 #define MADV_KEEPONFORK 72             /* Undo MADV_WIPEONFORK */
 
+#define MADV_COLLAPSE  73              /* Synchronous hugepage collapse */
+
 #define MADV_HWPOISON     100          /* poison a page for testing */
 #define MADV_SOFT_OFFLINE 101          /* soft offline page for testing */
 

arch/xtensa/include/uapi/asm/mman.h +2 -0

index 7966a58af472..1ff0c858544f 100644
@@ -111,6 +111,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

include/linux/huge_mm.h +12 -0

index 9a26bd10e083..4a2ea1b5437c 100644
@@ -222,6 +222,9 @@ void __split_huge_pud(struct vm_area_struct *vma, pud_t *pud,
 
 int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags,
                     int advice);
+int madvise_collapse(struct vm_area_struct *vma,
+                    struct vm_area_struct **prev,
+                    unsigned long start, unsigned long end);
 void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start,
                           unsigned long end, long adjust_next);
 spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma);

@@ -378,6 +381,15 @@ static inline int hugepage_madvise(struct vm_area_struct *vma,
        BUG();
        return 0;
 }
+
+static inline int madvise_collapse(struct vm_area_struct *vma,
+                                  struct vm_area_struct **prev,
+                                  unsigned long start, unsigned long end)
+{
+       BUG();
+       return 0;
+}
+
 static inline void vma_adjust_trans_huge(struct vm_area_struct *vma,
                                         unsigned long start,
                                         unsigned long end,

include/uapi/asm-generic/mman-common.h +2 -0

index 6c1aa92a92e4..6ce1f1ceb432 100644
@@ -77,6 +77,8 @@
 
 #define MADV_DONTNEED_LOCKED   24      /* like DONTNEED, but drop locked pages too */
 
+#define MADV_COLLAPSE  25              /* Synchronous hugepage collapse */
+
 /* compatibility flags */
 #define MAP_FILE       0
 

mm/khugepaged.c +93 -12

index b14807b7002e..165c646ddb8f 100644
@@ -837,6 +837,22 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
        return khugepaged_defrag() ? GFP_TRANSHUGE : GFP_TRANSHUGE_LIGHT;
 }
 
+static bool alloc_hpage(gfp_t gfp, int node, struct collapse_control *cc)
+{
+       VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
+
+       cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
+       if (unlikely(!cc->hpage)) {
+               count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
+               cc->hpage = ERR_PTR(-ENOMEM);
+               return false;
+       }
+
+       prep_transhuge_page(cc->hpage);
+       count_vm_event(THP_COLLAPSE_ALLOC);
+       return true;
+}
+
 #ifdef CONFIG_NUMA
 static int khugepaged_find_target_node(struct collapse_control *cc)
 {

@@ -882,18 +898,7 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
 static bool khugepaged_alloc_page(gfp_t gfp, int node,
                                  struct collapse_control *cc)
 {
-       VM_BUG_ON_PAGE(cc->hpage, cc->hpage);
-
-       cc->hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
-       if (unlikely(!cc->hpage)) {
-               count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
-               cc->hpage = ERR_PTR(-ENOMEM);
-               return false;
-       }
-
-       prep_transhuge_page(cc->hpage);
-       count_vm_event(THP_COLLAPSE_ALLOC);
-       return true;
+       return alloc_hpage(gfp, node, cc);
 }
 #else
 static int khugepaged_find_target_node(struct collapse_control *cc)

@@ -2457,3 +2462,141 @@ void khugepaged_min_free_kbytes_update(void)
                set_recommended_min_free_kbytes();
        mutex_unlock(&khugepaged_mutex);
 }
+
+static void madvise_collapse_cleanup_page(struct page **hpage)
+{
+       if (!IS_ERR(*hpage) && *hpage)
+               put_page(*hpage);
+       *hpage = NULL;
+}
+
+static int madvise_collapse_errno(enum scan_result r)
+{
+       switch (r) {
+       case SCAN_PMD_NULL:
+       case SCAN_ADDRESS_RANGE:
+       case SCAN_VMA_NULL:
+       case SCAN_PTE_NON_PRESENT:
+       case SCAN_PAGE_NULL:
+               /*
+                * Addresses in the specified range are not currently mapped,
+                * or are outside the AS of the process.
+                */
+               return -ENOMEM;
+       case SCAN_ALLOC_HUGE_PAGE_FAIL:
+       case SCAN_CGROUP_CHARGE_FAIL:
+               /* A kernel resource was temporarily unavailable. */
+               return -EAGAIN;
+       default:
+               return -EINVAL;
+       }
+}
+
+static int madvise_alloc_charge_hpage(struct mm_struct *mm,
+                                     struct collapse_control *cc)
+{
+       if (!alloc_hpage(GFP_TRANSHUGE, khugepaged_find_target_node(cc), cc))
+               return SCAN_ALLOC_HUGE_PAGE_FAIL;
+       if (unlikely(mem_cgroup_charge(page_folio(cc->hpage), mm,
+                                      GFP_TRANSHUGE)))
+               return SCAN_CGROUP_CHARGE_FAIL;
+       count_memcg_page_event(cc->hpage, THP_COLLAPSE_ALLOC);
+       return SCAN_SUCCEED;
+}
+
+int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
+                    unsigned long start, unsigned long end)
+{
+       struct collapse_control cc = {
+               .enforce_pte_scan_limits = false,
+               .enforce_young = false,
+               .last_target_node = NUMA_NO_NODE,
+               .hpage = NULL,
+               .alloc_charge_hpage = &madvise_alloc_charge_hpage,
+       };
+       struct mm_struct *mm = vma->vm_mm;
+       unsigned long hstart, hend, addr;
+       int thps = 0, nr_hpages = 0, result = SCAN_FAIL;
+       bool mmap_locked = true;
+
+       BUG_ON(vma->vm_start > start);
+       BUG_ON(vma->vm_end < end);
+
+       *prev = vma;
+
+       if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file)
+               return -EINVAL;
+
+       hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
+       hend = end & HPAGE_PMD_MASK;
+       nr_hpages = (hend - hstart) >> HPAGE_PMD_SHIFT;
+
+       if (hstart >= hend || !transparent_hugepage_active(vma))
+               return -EINVAL;
+
+       mmgrab(mm);
+       lru_add_drain();
+
+       for (addr = hstart; ; ) {

mm/khugepaged.c void khugepaged_min_free_kbytes_update(void) · line 2544

Quoted 4 lines

mm/khugepaged.c +4 -0

+               mmap_assert_locked(mm);
+               cond_resched();
+               result = SCAN_FAIL;
+

Made a last minute change to khugepaged_scan_pmd() which no longer cleared mmap_locked, so it needs to be reset here. Sorry about that - testing was only collapsing 1 huge page so it wasn't caught.

Quoted 98 lines

mm/khugepaged.c +58 -0

+               if (unlikely(khugepaged_test_exit(mm))) {
+                       result = SCAN_ANY_PROCESS;
+                       break;
+               }
+
+               memset(cc.node_load, 0, sizeof(cc.node_load));
+               result = khugepaged_scan_pmd(mm, vma, addr, &mmap_locked, &cc);
+               if (!mmap_locked)
+                       *prev = NULL;  /* tell madvise we dropped mmap_lock */
+
+               switch (result) {
+               /* Whitelisted set of results where continuing OK */
+               case SCAN_SUCCEED:
+               case SCAN_PMD_MAPPED:
+                       ++thps;
+                       fallthrough;
+               case SCAN_PMD_NULL:
+               case SCAN_PTE_NON_PRESENT:
+               case SCAN_PTE_UFFD_WP:
+               case SCAN_PAGE_RO:
+               case SCAN_LACK_REFERENCED_PAGE:
+               case SCAN_PAGE_NULL:
+               case SCAN_PAGE_COUNT:
+               case SCAN_PAGE_LOCK:
+               case SCAN_PAGE_COMPOUND:
+                       break;
+               case SCAN_PAGE_LRU:
+                       lru_add_drain_all();
+                       goto retry;
+               default:
+                       /* Other error, exit */
+                       goto break_loop;
+               }
+               addr += HPAGE_PMD_SIZE;
+               if (addr >= hend)
+                       break;
+retry:
+               if (!mmap_locked) {
+                       mmap_read_lock(mm);
+                       mmap_locked = true;
+                       result = hugepage_vma_revalidate(mm, addr, &vma);
+                       if (result)
+                               goto out;
+               }
+               madvise_collapse_cleanup_page(&cc.hpage);
+       }
+
+break_loop:
+       /* madvise_walk_vmas() expects us to hold mmap_lock on return */
+       if (!mmap_locked)
+               mmap_read_lock(mm);
+out:
+       mmap_assert_locked(mm);
+       madvise_collapse_cleanup_page(&cc.hpage);
+       mmdrop(mm);
+
+       return thps == nr_hpages ? 0 : madvise_collapse_errno(result);
+}

mm/madvise.c +5 -1

index 5f4537511532..638517952bd2 100644
@@ -59,6 +59,7 @@ static int madvise_need_mmap_write(int behavior)
        case MADV_FREE:
        case MADV_POPULATE_READ:
        case MADV_POPULATE_WRITE:
+       case MADV_COLLAPSE:
                return 0;
        default:
                /* be safe, default to 1. list exceptions explicitly */

@@ -1054,6 +1055,8 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
                if (error)
                        goto out;
                break;
+       case MADV_COLLAPSE:
+               return madvise_collapse(vma, prev, start, end);
        }
 
        anon_name = anon_vma_name(vma);

@@ -1147,6 +1150,7 @@ madvise_behavior_valid(int behavior)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
        case MADV_HUGEPAGE:
        case MADV_NOHUGEPAGE:
+       case MADV_COLLAPSE:
 #endif
        case MADV_DONTDUMP:
        case MADV_DODUMP:

@@ -1336,6 +1340,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
  *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
  *             transparent huge pages so the existing pages will not be
  *             coalesced into THP and new pages will not be allocated as THP.
+ *  MADV_COLLAPSE - synchronously coalesce pages into new THP.
  *  MADV_DONTDUMP - the application wants to prevent pages in the given range
  *             from being included in its core dump.
  *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
--

2.36.0.464.gb9c8b46e94-goog

The following functions/tracepoints are shared between khugepaged and madvise collapse contexts. Replace the "khugepaged_" prefixe with generic "hpage_collapse_" prefix in such cases:

huge_memory:mm_khugepaged_scan_pmd -> huge_memory:mm_hpage_collapse_scan_pmd khugepaged_test_exit() -> hpage_collapse_test_exit() khugepaged_scan_abort() -> hpage_collapse_scan_abort() khugepaged_scan_pmd() -> hpage_collapse_scan_pmd() khugepaged_find_target_node() -> hpage_collapse_find_target_node()

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

Allow MADV_COLLAPSE behavior for process_madvise(2) if caller has CAP_SYS_ADMIN or is requesting collapse of it's own memory.

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

---
  mm/madvise.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

mm/madvise.c +4 -2

index 638517952bd2..08c11217025a 100644
@@ -1168,13 +1168,15 @@ madvise_behavior_valid(int behavior)
  }
 
  static bool
-process_madvise_behavior_valid(int behavior)
+process_madvise_behavior_valid(int behavior, struct task_struct *task)
  {
  	switch (behavior) {
  	case MADV_COLD:
  	case MADV_PAGEOUT:
  	case MADV_WILLNEED:
  		return true;
+	case MADV_COLLAPSE:
+		return task == current || capable(CAP_SYS_ADMIN);
  	default:
  		return false;
  	}

@@ -1452,7 +1454,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
  		goto free_iov;
  	}
 
-	if (!process_madvise_behavior_valid(behavior)) {
+	if (!process_madvise_behavior_valid(behavior, task)) {
  		ret = -EINVAL;
  		goto release_task;
  	}

Hi, Zach

Thanks for your great patchset!
Recently, We also try to collapse THP in this way, likes performance degradation due to using too much hugepages in our scenes.

And there is a doubt about process_madvise(MADV_COLLAPSE) when we test this patchset:. It seems that process_madvise(MADV_COLLAPSE) rely on madvise(MADV_HUGEPAGE)? If the vma wasn't marked with 'hg', process_madvise(MADV_COLLAPSE) will fail to collapse. And if I miss something, please let me know.

If so, how about introducing process_madvise(MADV_HUGEPAGE) or process_madvise(MADV_NOHUGEPAGE)? The former helps to mark the target vma with 'hg', and the collapse process can be finished completely with the help of other processes. the latter could let some special vma avoid collapsing when setting 'THP=always'.

Best regards,
-wrw

Quoted 38 lines · Zach O'Keefe

Allow MADV_COLLAPSE behavior for process_madvise(2) if caller has CAP_SYS_ADMIN or is requesting collapse of it's own memory.

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

---
  mm/madvise.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

mm/madvise.c +4 -2

index 638517952bd2..08c11217025a 100644
@@ -1168,13 +1168,15 @@ madvise_behavior_valid(int behavior)
  }
 
  static bool
-process_madvise_behavior_valid(int behavior)
+process_madvise_behavior_valid(int behavior, struct task_struct *task)
  {
      switch (behavior) {
      case MADV_COLD:
      case MADV_PAGEOUT:
      case MADV_WILLNEED:
              return true;
+     case MADV_COLLAPSE:
+             return task == current || capable(CAP_SYS_ADMIN);
      default:
              return false;
      }

@@ -1452,7 +1454,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
              goto free_iov;
      }
 
-     if (!process_madvise_behavior_valid(behavior)) {
+     if (!process_madvise_behavior_valid(behavior, task)) {
              ret = -EINVAL;
              goto release_task;
      }

Hey Rongwei,

Thanks for taking the time to review!

Quoted 7 lines · Rongwei Wang

Hi, Zach

Thanks for your great patchset!
Recently, We also try to collapse THP in this way, likes performance degradation due to using too much hugepages in our scenes.

Quoted 6 lines

And there is a doubt about process_madvise(MADV_COLLAPSE) when we test this patchset:. It seems that process_madvise(MADV_COLLAPSE) rely on madvise(MADV_HUGEPAGE)? If the vma wasn't marked with 'hg', process_madvise(MADV_COLLAPSE) will fail to collapse. And if I miss something, please let me know.

I tried to have MADV_COLLAPSE follow the same THP eligibility semantics as khugepaged and at-fault: either THP=always, or THP=madvise and the vma is marked with MADV_HUGEPAGE, as you point out.

If I understand you correctly, the usefulness of process_madvise(MADV_COLLAPSE) is limited in the case where THP=madvise and a CAP_SYS_ADMIN user is requesting a collapse of behalf of another process since they don't have a way to mark the target memory as eligible (which requires VM_HUGEPAGE).

If so, I think that's a valid point, and your suggestion below of a supporting MADV_[NO]HUGEPAGE for process_madvise(2) makes sense. For the sake of exploring all options, I'll mention that there was also a previous idea suggested by Yang Shi where MADV_COLLAPSE could also set VM_HUGEPAGE[1].

Since it's possible supporting MADV_[NO]HUGEPAGE for process_madivse(2) has applications outside a subsequent MADV_COLLAPSE, and since I don't see process_madvise(MADV_COLLAPSE) to be in a hot path, I'd vote in favor of your suggestion and include process_madvise(MADV_[NO]HUGEPAGE) support in v6 unless others object.

Thanks again for your review and your suggestion!
Zach

[1] https://lore.kernel.org/linux-mm/CAHbLzkqLRBd6u3qn=KqpOhRcPZtpGXbTXLUjK1z=4d_dQ06Pvw@mail.gmail.com/

Quoted 48 lines

If so, how about introducing process_madvise(MADV_HUGEPAGE) or process_madvise(MADV_NOHUGEPAGE)? The former helps to mark the target vma with 'hg', and the collapse process can be finished completely with the help of other processes. the latter could let some special vma avoid collapsing when setting 'THP=always'.

Best regards,
-wrw

Quoted 38 lines · Zach O'Keefe

Allow MADV_COLLAPSE behavior for process_madvise(2) if caller has CAP_SYS_ADMIN or is requesting collapse of it's own memory.

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

---
   mm/madvise.c | 6 ++++--
   1 file changed, 4 insertions(+), 2 deletions(-)

mm/madvise.c +4 -2

index 638517952bd2..08c11217025a 100644
@@ -1168,13 +1168,15 @@ madvise_behavior_valid(int behavior)
   }
 
   static bool
-process_madvise_behavior_valid(int behavior)
+process_madvise_behavior_valid(int behavior, struct task_struct *task)
   {
       switch (behavior) {
       case MADV_COLD:
       case MADV_PAGEOUT:
       case MADV_WILLNEED:
               return true;
+     case MADV_COLLAPSE:
+             return task == current || capable(CAP_SYS_ADMIN);
       default:
               return false;
       }

@@ -1452,7 +1454,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
               goto free_iov;
       }
 
-     if (!process_madvise_behavior_valid(behavior)) {
+     if (!process_madvise_behavior_valid(behavior, task)) {
               ret = -EINVAL;
               goto release_task;
       }

Hey Rongwei,

Thanks for taking the time to review!

Quoted 7 lines · Rongwei Wang

Hi, Zach

Thanks for your great patchset!
Recently, We also try to collapse THP in this way, likes performance degradation due to using too much hugepages in our scenes.

Quoted 6 lines

And there is a doubt about process_madvise(MADV_COLLAPSE) when we test this patchset:. It seems that process_madvise(MADV_COLLAPSE) rely on madvise(MADV_HUGEPAGE)? If the vma wasn't marked with 'hg', process_madvise(MADV_COLLAPSE) will fail to collapse. And if I miss something, please let me know.

I tried to have MADV_COLLAPSE follow the same THP eligibility semantics as khugepaged and at-fault: either THP=always, or THP=madvise and the vma is marked with MADV_HUGEPAGE, as you point out.

If I understand you correctly, the usefulness of process_madvise(MADV_COLLAPSE) is limited in the case where THP=madvise and a CAP_SYS_ADMIN user is requesting a collapse of behalf of another process since they don't have a way to mark the target memory as eligible (which requires VM_HUGEPAGE).

If so, I think that's a valid point, and your suggestion below of a supporting MADV_[NO]HUGEPAGE for process_madvise(2) makes sense. For the sake of exploring all options, I'll mention that there was also a previous idea suggested by Yang Shi where MADV_COLLAPSE could also set VM_HUGEPAGE[1].

Since it's possible supporting MADV_[NO]HUGEPAGE for process_madivse(2) has applications outside a subsequent MADV_COLLAPSE, and since I don't see process_madvise(MADV_COLLAPSE) to be in a hot path, I'd vote in favor of your suggestion and include process_madvise(MADV_[NO]HUGEPAGE) support in v6 unless others object.

Thanks again for your review and your suggestion!
Zach

[1] https://lore.kernel.org/linux-mm/CAHbLzkqLRBd6u3qn=KqpOhRcPZtpGXbTXLUjK1z=4d_dQ06Pvw@mail.gmail.com/

Quoted 48 lines

If so, how about introducing process_madvise(MADV_HUGEPAGE) or process_madvise(MADV_NOHUGEPAGE)? The former helps to mark the target vma with 'hg', and the collapse process can be finished completely with the help of other processes. the latter could let some special vma avoid collapsing when setting 'THP=always'.

Best regards,
-wrw

Quoted 38 lines · Zach O'Keefe

Allow MADV_COLLAPSE behavior for process_madvise(2) if caller has CAP_SYS_ADMIN or is requesting collapse of it's own memory.

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

---
  mm/madvise.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

mm/madvise.c +4 -2

index 638517952bd2..08c11217025a 100644
@@ -1168,13 +1168,15 @@ madvise_behavior_valid(int behavior)
  }
 
  static bool
-process_madvise_behavior_valid(int behavior)
+process_madvise_behavior_valid(int behavior, struct task_struct *task)
  {
      switch (behavior) {
      case MADV_COLD:
      case MADV_PAGEOUT:
      case MADV_WILLNEED:
              return true;
+     case MADV_COLLAPSE:
+             return task == current || capable(CAP_SYS_ADMIN);
      default:
              return false;
      }

@@ -1452,7 +1454,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
              goto free_iov;
      }
 
-     if (!process_madvise_behavior_valid(behavior)) {
+     if (!process_madvise_behavior_valid(behavior, task)) {
              ret = -EINVAL;
              goto release_task;
      }