| Message ID | bdca97e6c59f1b58f09e18defac0c47d1e38f44b.1709041586.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | make the hugetlb migration strategy consistent | expand |
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)) {
On 2024/2/28 16:41, Oscar Salvador wrote: > 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; IMHO, users also should not be aware of these hugetlb logics. > > 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); 'allow_fallback' can be confusing, that means it is 'allow_fallback' for a new temporary hugetlb allocation, but not 'allow_fallback' for an available hugetlb allocation in alloc_hugetlb_folio_nodemask().
On Wed, Mar 06, 2024 at 04:35:26PM +0800, Baolin Wang wrote: > On 2024/2/28 16:41, Oscar Salvador wrote: > > if (folio_test_hugetlb(src)) { > > struct hstate *h = folio_hstate(src); > > + bool allow_fallback = false; > > + > > + if ((1UL << reason) & HTLB_ALLOW_FALLBACK) > > + allow_fallback = true; > > IMHO, users also should not be aware of these hugetlb logics. Note that what I wrote there was ugly, because it was just a PoC. It could be a helper e.g: if (hugetlb_reason_allow_alloc_fallback(reason)) (or whatever) allow_fallback_alloc = 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); > > 'allow_fallback' can be confusing, that means it is 'allow_fallback' for a > new temporary hugetlb allocation, but not 'allow_fallback' for an available > hugetlb allocation in alloc_hugetlb_folio_nodemask(). Well, you can pick "alloc_fallback_on_alloc" which is more descriptive I guess. Bottomline line is that I do not think that choosing to allow fallbacking or not here is spreading more logic than having the htlb_modify_alloc_mask() here and not directly in alloc_hugetlb_folio_nodemask(). As I said, code-wise looks fine, it is just that having to pass the 'reason' all the way down and making the decision there makes me go "meh..".
On 2024/3/6 16:46, Oscar Salvador wrote: > On Wed, Mar 06, 2024 at 04:35:26PM +0800, Baolin Wang wrote: >> On 2024/2/28 16:41, Oscar Salvador wrote: > >>> if (folio_test_hugetlb(src)) { >>> struct hstate *h = folio_hstate(src); >>> + bool allow_fallback = false; >>> + >>> + if ((1UL << reason) & HTLB_ALLOW_FALLBACK) >>> + allow_fallback = true; >> >> IMHO, users also should not be aware of these hugetlb logics. > > Note that what I wrote there was ugly, because it was just a PoC. > > It could be a helper e.g: > > if (hugetlb_reason_allow_alloc_fallback(reason)) (or whatever) > allow_fallback_alloc = 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); >> >> 'allow_fallback' can be confusing, that means it is 'allow_fallback' for a >> new temporary hugetlb allocation, but not 'allow_fallback' for an available >> hugetlb allocation in alloc_hugetlb_folio_nodemask(). > > Well, you can pick "alloc_fallback_on_alloc" which is more descriptive I > guess. Seems better. > > Bottomline line is that I do not think that choosing to allow > fallbacking or not here is spreading more logic than having the > htlb_modify_alloc_mask() here and not directly in > alloc_hugetlb_folio_nodemask(). > > As I said, code-wise looks fine, it is just that having to pass > the 'reason' all the way down and making the decision there makes > me go "meh..". Well, fair enough:) let me respin it. Thanks.
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 77b30a8c6076..fa122dc509cf 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -747,7 +747,7 @@ int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list); struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, unsigned long addr, int avoid_reserve); 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, int reason); int hugetlb_add_to_page_cache(struct folio *folio, struct address_space *mapping, pgoff_t idx); void restore_reserve_on_error(struct hstate *h, struct vm_area_struct *vma, @@ -1065,7 +1065,7 @@ static inline struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, static inline 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, int reason) { return NULL; } diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 418d66953224..e8eb08bbc688 100644 --- 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) { struct folio *folio; + bool allowed_fallback = false; if (hstate_is_gigantic(h)) return NULL; + if (gfp_mask & __GFP_THISNODE) + goto alloc_new; + + /* + * Note: the memory offline, memory failure and migration syscalls will + * be allowed to fallback to other nodes due to lack of a better chioce, + * that might break the per-node hugetlb pool. While other cases will + * set the __GFP_THISNODE to avoid breaking the per-node hugetlb pool. + */ + switch (reason) { + case MR_MEMORY_HOTPLUG: + case MR_MEMORY_FAILURE: + case MR_SYSCALL: + case MR_MEMPOLICY_MBIND: + allowed_fallback = true; + break; + default: + break; + } + + if (!allowed_fallback) + gfp_mask |= __GFP_THISNODE; + +alloc_new: folio = alloc_fresh_hugetlb_folio(h, gfp_mask, nid, nmask, NULL); if (!folio) return NULL; @@ -2621,7 +2646,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, int reason) { spin_lock_irq(&hugetlb_lock); if (available_huge_pages(h)) { @@ -2636,7 +2661,7 @@ struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid, } spin_unlock_irq(&hugetlb_lock); - return alloc_migrate_hugetlb_folio(h, gfp_mask, preferred_nid, nmask); + return alloc_migrate_hugetlb_folio(h, gfp_mask, preferred_nid, nmask, reason); } /* @@ -6653,7 +6678,13 @@ static struct folio *alloc_hugetlb_folio_vma(struct hstate *h, gfp_mask = htlb_alloc_mask(h); node = huge_node(vma, address, gfp_mask, &mpol, &nodemask); - folio = alloc_hugetlb_folio_nodemask(h, node, nodemask, gfp_mask); + /* + * This is used to allocate a temporary hugetlb to hold the copied + * content, which will then be copied again to the final hugetlb + * consuming a reservation. Set the migrate reason to -1 to indicate + * that breaking the per-node hugetlb pool is not allowed in this case. + */ + folio = alloc_hugetlb_folio_nodemask(h, node, nodemask, gfp_mask, -1); mpol_cond_put(mpol); return folio; diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 98ceb12e0e17..436e817eeaeb 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1228,7 +1228,7 @@ static struct folio *alloc_migration_target_by_mpol(struct folio *src, h = folio_hstate(src); gfp = htlb_alloc_mask(h); nodemask = policy_nodemask(gfp, pol, ilx, &nid); - return alloc_hugetlb_folio_nodemask(h, nid, nodemask, gfp); + return alloc_hugetlb_folio_nodemask(h, nid, nodemask, gfp, MR_MEMPOLICY_MBIND); } if (folio_test_large(src)) diff --git a/mm/migrate.c b/mm/migrate.c index bde63010a3cf..0c2b70800da3 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2022,7 +2022,7 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private) gfp_mask = htlb_modify_alloc_mask(h, gfp_mask); return alloc_hugetlb_folio_nodemask(h, nid, - mtc->nmask, gfp_mask); + mtc->nmask, gfp_mask, mtc->reason); } if (folio_test_large(src)) {
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(-)