[2/3] mm: hugetlb: make the hugetlb migration strategy consistent

  As discussed in previous thread [1], there is an inconsistency when handing
hugetlb migration. When handling the migration of freed hugetlb, it prevents
fallback to other NUMA nodes in alloc_and_dissolve_hugetlb_folio(). However,
when dealing with in-use hugetlb, it allows fallback to other NUMA nodes in
alloc_hugetlb_folio_nodemask(), which can break the per-node hugetlb pool
and might result in unexpected failures when node bound workloads doesn't get
what is asssumed available.

To make hugetlb migration strategy more clear, we should list all the scenarios
of hugetlb migration and analyze whether allocation fallback is permitted:
1) Memory offline: will call dissolve_free_huge_pages() to free the freed hugetlb,
and call do_migrate_range() to migrate the in-use hugetlb. Both can break the
per-node hugetlb pool, but as this is an explicit offlining operation, no better
choice. So should allow the hugetlb allocation fallback.
2) Memory failure: same as memory offline. Should allow fallback to a different node
might be the only option to handle it, otherwise the impact of poisoned memory can
be amplified.
3) Longterm pinning: will call migrate_longterm_unpinnable_pages() to migrate in-use
and not-longterm-pinnable hugetlb, which can break the per-node pool. But we should
fail to longterm pinning if can not allocate on current node to avoid breaking the
per-node pool.
4) Syscalls (mbind, migrate_pages, move_pages): these are explicit users operation
to move pages to other nodes, so fallback to other nodes should not be prohibited.
5) alloc_contig_range: used by CMA allocation and virtio-mem fake-offline to allocate
given range of pages. Now the freed hugetlb migration is not allowed to fallback, to
keep consistency, the in-use hugetlb migration should be also not allowed to fallback.
6) alloc_contig_pages: used by kfence, pgtable_debug etc. The strategy should be
consistent with that of alloc_contig_range().

Based on the analysis of the various scenarios above, determine whether fallback is
permitted according to the migration reason in alloc_migrate_hugetlb_folio().

[1] https://lore.kernel.org/all/6f26ce22d2fcd523418a085f2c588fe0776d46e7.1706794035.git.baolin.wang@linux.alibaba.com/
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 include/linux/hugetlb.h |  4 ++--
 mm/hugetlb.c            | 39 +++++++++++++++++++++++++++++++++++----
 mm/mempolicy.c          |  2 +-
 mm/migrate.c            |  2 +-
 4 files changed, 39 insertions(+), 8 deletions(-)
  

On Tue, Feb 27, 2024 at 09:52:26PM +0800, Baolin Wang wrote:

> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2567,13 +2567,38 @@ static struct folio *alloc_surplus_hugetlb_folio(struct hstate *h,
>  }
>  
>  static struct folio *alloc_migrate_hugetlb_folio(struct hstate *h, gfp_t gfp_mask,
> -				     int nid, nodemask_t *nmask)
> +				     int nid, nodemask_t *nmask, int reason)

I still dislike taking the reason argument this far, and I'd rather have
this as a boolean specifing whether we allow fallback on other nodes.
That would mean parsing the reason in alloc_migration_target().
If we don't add a new helper e.g: gfp_allow_fallback(), we can just do
it right there an opencode it with a e.g: macro etc.

Although doing it in an inline helper might help hiding these details.

That's my take on this, but let's see what others have to say.
  

On 2024/2/27 23:17, Oscar Salvador wrote:
> On Tue, Feb 27, 2024 at 09:52:26PM +0800, Baolin Wang wrote:
> 
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -2567,13 +2567,38 @@ static struct folio *alloc_surplus_hugetlb_folio(struct hstate *h,
>>   }
>>   
>>   static struct folio *alloc_migrate_hugetlb_folio(struct hstate *h, gfp_t gfp_mask,
>> -				     int nid, nodemask_t *nmask)
>> +				     int nid, nodemask_t *nmask, int reason)
> 
> I still dislike taking the reason argument this far, and I'd rather have
> this as a boolean specifing whether we allow fallback on other nodes.
> That would mean parsing the reason in alloc_migration_target().
> If we don't add a new helper e.g: gfp_allow_fallback(), we can just do
> it right there an opencode it with a e.g: macro etc.
> 
> Although doing it in an inline helper might help hiding these details.
> 
> That's my take on this, but let's see what others have to say.

Sure. I also expressed my preference for hiding these details within the 
hugetlb core as much as possible.

Muchun, what do you think? Thanks.
  

On Wed, Feb 28, 2024 at 03:40:08PM +0800, Baolin Wang wrote:
> 
> 
> On 2024/2/27 23:17, Oscar Salvador wrote:
> > On Tue, Feb 27, 2024 at 09:52:26PM +0800, Baolin Wang wrote:
> > 
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -2567,13 +2567,38 @@ static struct folio *alloc_surplus_hugetlb_folio(struct hstate *h,
> > >   }
> > >   static struct folio *alloc_migrate_hugetlb_folio(struct hstate *h, gfp_t gfp_mask,
> > > -				     int nid, nodemask_t *nmask)
> > > +				     int nid, nodemask_t *nmask, int reason)
> > 
> > I still dislike taking the reason argument this far, and I'd rather have
> > this as a boolean specifing whether we allow fallback on other nodes.
> > That would mean parsing the reason in alloc_migration_target().
> > If we don't add a new helper e.g: gfp_allow_fallback(), we can just do
> > it right there an opencode it with a e.g: macro etc.
> > 
> > Although doing it in an inline helper might help hiding these details.
> > 
> > That's my take on this, but let's see what others have to say.
> 
> Sure. I also expressed my preference for hiding these details within the
> hugetlb core as much as possible.
> 
> Muchun, what do you think? Thanks.

JFTR: I'm talking about https://lore.kernel.org/linux-mm/ZdxXLTDZn8fD3pEn@localhost.localdomain/
or maybe something cleaner which doesn't need a new helper (we could if
we want though):

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index c1ee640d87b1..ddd794e861e6 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -73,6 +73,16 @@ struct resv_map {
 #endif
 };

+#define MIGRATE_MEMORY_HOTPLUG	1UL << MR_MEMORY_HOTPLUG
+#define MIGRATE_MEMORY_FAILURE	1UL << MR_MEMORY_FAILURE
+#define MIGRATE_SYSCALL		1UL << MR_SYSCALL
+#define MIGRATE_MBIND		1UL << MR_MEMPOLICY_MBIND
+#define HTLB_ALLOW_FALLBACK	(MIGRATE_MEMORY_HOTPLUG| \
+				 MIGRATE_MEMORY_FAILURE| \
+				 MIGRATE_SYSCALL| \
+				 MIGRATE_MBIND)
+
+
 /*
  * Region tracking -- allows tracking of reservations and instantiated pages
  *                    across the pages in a mapping.
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index ed1581b670d4..7e8d6b5885d6 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2619,7 +2619,7 @@ struct folio *alloc_buddy_hugetlb_folio_with_mpol(struct hstate *h,

 /* folio migration callback function */
 struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid,
-		nodemask_t *nmask, gfp_t gfp_mask)
+		nodemask_t *nmask, gfp_t gfp_mask, bool allow_fallback)
 {
 	spin_lock_irq(&hugetlb_lock);
 	if (available_huge_pages(h)) {
@@ -2634,6 +2634,12 @@ struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid,
 	}
 	spin_unlock_irq(&hugetlb_lock);

+	/*
+	 * We cannot fallback to other nodes, as we could break the per-node pool
+	 */
+	if (!allow_fallback)
+		gfp_mask |= GFP_THISNODE;
+
 	return alloc_migrate_hugetlb_folio(h, gfp_mask, preferred_nid, nmask);
 }

diff --git a/mm/migrate.c b/mm/migrate.c
index cc9f2bcd73b4..c1f1d011629d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2016,10 +2016,15 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)

 	if (folio_test_hugetlb(src)) {
 		struct hstate *h = folio_hstate(src);
+		bool allow_fallback = false;
+
+		if ((1UL << reason) & HTLB_ALLOW_FALLBACK)
+			allow_fallback = true;

 		gfp_mask = htlb_modify_alloc_mask(h, gfp_mask);
 		return alloc_hugetlb_folio_nodemask(h, nid,
-						mtc->nmask, gfp_mask);
+						mtc->nmask, gfp_mask,
+						allow_fallback);
 	}

 	if (folio_test_large(src)) {