Respect mempolicy when calculating surplus huge pages.

Presently, when calculating how many huge pages are needed when reserving surplus huge pages, the global count of free huge pages are used. When reserving with a mempolicy, the global count of free huge pages is used even if some/all of those free huge pages are on numa nodes outside of the mempolicy. Fix it so free huge pages only on nodes within the mempolicy are considered.

---
 mm/hugetlb.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

index f24bf49be0..02752ce735 100644
@@ -2255,6 +2255,23 @@ static nodemask_t *policy_mbind_nodemask(gfp_t gfp)
 	return NULL;
 }
 
+static unsigned int allowed_mems_nr(struct hstate *h)
+{
+	int node;
+	unsigned int nr = 0;
+	nodemask_t *mbind_nodemask;
+	unsigned int *array = h->free_huge_pages_node;
+	gfp_t gfp_mask = htlb_alloc_mask(h);
+
+	mbind_nodemask = policy_mbind_nodemask(gfp_mask);
+	for_each_node_mask(node, cpuset_current_mems_allowed) {
+		if (!mbind_nodemask || node_isset(node, *mbind_nodemask))
+			nr += array[node];
+	}
+
+	return nr;
+}
+
 /*
  * Increase the hugetlb pool such that it can accommodate a reservation
  * of size 'delta'.

@@ -2277,7 +2294,7 @@ static int gather_surplus_pages(struct hstate *h, long delta)
 		alloc_nodemask = cpuset_current_mems_allowed;
 
 	lockdep_assert_held(&hugetlb_lock);
-	needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
+	needed = (h->resv_huge_pages + delta) - allowed_mems_nr(h);
 	if (needed <= 0) {
 		h->resv_huge_pages += delta;
 		return 0;

@@ -2312,7 +2329,7 @@ static int gather_surplus_pages(struct hstate *h, long delta)
 	 */
 	spin_lock_irq(&hugetlb_lock);
 	needed = (h->resv_huge_pages + delta) -
-			(h->free_huge_pages + allocated);
+			(allowed_mems_nr(h) + allocated);
 	if (needed > 0) {
 		if (alloc_ok)
 			goto retry;

@@ -4513,23 +4530,6 @@ static int __init hugepage_alloc_threads_setup(char *s)
 }
 __setup("hugepage_alloc_threads=", hugepage_alloc_threads_setup);
 
-static unsigned int allowed_mems_nr(struct hstate *h)
-{
-	int node;
-	unsigned int nr = 0;
-	nodemask_t *mbind_nodemask;
-	unsigned int *array = h->free_huge_pages_node;
-	gfp_t gfp_mask = htlb_alloc_mask(h);
-
-	mbind_nodemask = policy_mbind_nodemask(gfp_mask);
-	for_each_node_mask(node, cpuset_current_mems_allowed) {
-		if (!mbind_nodemask || node_isset(node, *mbind_nodemask))
-			nr += array[node];
-	}
-
-	return nr;
-}
-
 void hugetlb_report_meminfo(struct seq_file *m)
 {
 	struct hstate *h;

2.54.0

Thanks. AI review asked a thing:

	https://sashiko.dev/#/patchset/[email protected]

https://sashiko.dev/#/patchset/[email protected]

Thank you! In double checking, I can confirm overallocation of surplus huge pages occur under specific circumstances as described. I am working on a second version to address this.

Hello Charles, thank you for the patch!

I just wanted to add that it seems like this is a known issue. From the comment in hugetlb_acct_memory (the only caller of gather_surplus_pages) we have the following comment block:

/*
* When cpuset is configured, it breaks the strict hugetlb page
* reservation as the accounting is done on a global variable. Such
* reservation is completely rubbish in the presence of cpuset because
* the reservation is not checked against page availability for the
* current cpuset. Application can still potentially OOM'ed by kernel
* with lack of free htlb page in cpuset that the task is in.
* Attempt to enforce strict accounting with cpuset is almost
* impossible (or too ugly) because cpuset is too fluid that
* task or memory node can be dynamically moved between cpusets. *
* The change of semantics for shared hugetlb mapping with cpuset is
* undesirable. However, in order to preserve some of the semantics,
* we fall back to check against current free page availability as
* a best attempt and hopefully to minimize the impact of changing
* semantics that cpuset has.
*
* Apart from cpuset, we also have memory policy mechanism that
* also determines from which node the kernel will allocate memory
* in a NUMA system. So similar to cpuset, we also should consider
* the memory policy of the current task. Similar to the description
* above.

So it would appear that getting an exact number of pages to allocate, and ensure that there are no changes with the reservation or which nodes those reservations actually go to is a lot more difficult. But I think we can do a bit better.

FWIW, I think over-allocating is actually not fatal (although overallocating by a lot is obviously not desirable) since we free all the unused hugetlb pages at the end of gather_surplus_pages. I wonder if an approach like this could work:

@@ -2260,7 +2277,8 @@ static int gather_surplus_pages(struct hstate *h, long delta)

                alloc_nodemask = cpuset_current_mems_allowed;

        lockdep_assert_held(&hugetlb_lock);
-       needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
+       needed = max(delta - allowed_mems_nr(h),
+                    (h->resv_huge_pages + delta) - h->free_huge_pages);
        if (needed <= 0) {
                h->resv_huge_pages += delta;
                return 0;
@@ -2294,8 +2312,8 @@ static int gather_surplus_pages(struct hstate *h, long delta)
         * because either resv_huge_pages or free_huge_pages may have changed.
         */
        spin_lock_irq(&hugetlb_lock);
-       needed = (h->resv_huge_pages + delta) -
-                       (h->free_huge_pages + allocated);
+       needed = max((h->resv_huge_pages + delta) - h->free_huge_pages,
+                    delta - allowed_mems_nr(h)) - allocated;
        if (needed > 0) {
                if (alloc_ok)
                        goto retry;

So we compare the mempolicy-perspective "needed" and compare it to the global "needed" and take whatever. Since we are taking a max it should only ever make it more likely to actually succeed with the mempolicy-bound hugetlb page usage, even though we still can't make guarantees since a free page on our node may be taken by a different reservation later.

Let me know what you think. Thanks again!
Joshua

You are so welcome, and thank you! I'm newer to contributing to upstream so I welcome any and all feedback!

Agreed; actually, after submitting the initial version, I went back and have been working on adding per-node reserve hugepage counts to the hstates and adding some selftests for them. It's going well so far, but I'm hitting an issue in accounting where, after reserving a static hugetlb page on node0, reserving and then freeing a surplus hugetlb page on node1, and then freeing the static hugetlb page on node0, followup reservations on node0 additionally count towards surplus pages even when there are free static hugetlb pages on that node. Trace-cmd revealed allowed_mems_nr_free is returning more than what was requested resulting in the over-allocated surplus pages to stick around rather than be freed back. I plan to dig into what's causing that, however, if doing so leads down a nasty path of adding per-node accounting, I may need to fall back to the first version of this patch (but with selftests) and be ok with simply doing "a bit better" like you mention.

overallocating

Agreed on this point as well; afaik, surplus hugetlb pages are viewed quite differently from static hugetlb pages where surplus hugetlb pages are "best effort." Obviously we should try to reserve the requested amount, true users of large quantities of hugetlb pages should tend towards setting them on boot rather than dynamically.

This is considerably simpler than a per-node reserved hugetlb page count! If I end up falling back to my initial patch, I will absolutely be adding this and attributing you. Thank you!

Hello Charles, welcome!! : -)

I see. So from the perspective of the system, even though you free the static hugetlb page, the system views that as unavailable to other reservations and so future allocations go towards surplus pages? How exactly are you freeing the page in this case? And how long do you wait between freeing the static page and allocating the next page?

I wonder if you are seeing this issue without your per-node hugetlb page situation as well. I have seen quite a few hugetlb folio reservation restoration accounting bugs recently, including some that I think are still in mm-new. If you're developing against anything other than mm-new, I'd suggest trying to build on top of that to see if your bug still persists.

Sounds good! I wonder if it would be a good exercise to measure how much we typically over-allocate by when using the simplified calculation I noted in my previously reply.

Yup, totally agree here.

Sounds good with me : -) Good luck and have a great day! Joshua