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