Skip to contentSkip to conversation

mm/hugetlb: fix subpool accounting after cgroup charge failure

PATCH Superseded Catherine 8 participants 17 messages Apr 27 – Oct 28

Subsystems (3)
Stable branch Memory management HugeTLB
lore
Share & tools
lore

Catherine

author
#0 lore raw

PATCH 1/1 mm/hugetlb: fix subpool accounting after cgroup charge failure

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For subpools with max_hpages, that increments used_hpages even when the returned gbl_chg is positive.

If a later hugetlb cgroup charge fails, the cleanup currently calls hugepage_subpool_put_pages() only for !gbl_chg. The gbl_chg > 0 path therefore leaks one used_hpages charge per failure.

Always undo the subpool charge after a successful subpool get. Keep the global reservation accounting under !gbl_chg, because only that path consumed a reservation from the subpool.

Signed-off-by Catherine <[email protected]>
---
 mm/hugetlb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
mm/hugetlb.c +6 -4
index f24bf49be..b3ad024a0 100644
@@ -3026,12 +3026,14 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 						    h_cg);
 out_subpool_put:
 	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
+	 * map_chg means hugepage_subpool_get_pages() succeeded above.
+	 * Always undo the subpool quota charge; only drop global reservation
+	 * accounting if the subpool consumed a reservation.
 	 */
-	if (map_chg && !gbl_chg) {
+	if (map_chg) {
 		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+		if (!gbl_chg)
+			hugetlb_acct_memory(h, -gbl_reserve);
 	}
Signature · 1 line
2.50.1 (Apple Git-155)
maintainer
#1 lore raw
Quoted 11 lines · On Mon, 27 Apr 2026 22:52:48 +0800 Catherine <[email protected]> wrote:

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For subpools with max_hpages, that increments used_hpages even when the returned gbl_chg is positive.

If a later hugetlb cgroup charge fails, the cleanup currently calls hugepage_subpool_put_pages() only for !gbl_chg. The gbl_chg > 0 path therefore leaks one used_hpages charge per failure.

Always undo the subpool charge after a successful subpool get. Keep the global reservation accounting under !gbl_chg, because only that path consumed a reservation from the subpool.

Thanks.

We do prefer full, real names for kernel alterations. Can you please provide that?

author
#4 lore raw

PATCH v2 1/1 mm/hugetlb: fix subpool accounting after cgroup charge failure

In reply to Catherine

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For subpools with max_hpages, that increments used_hpages. If the later hugetlb cgroup charge fails, the unwind must undo that charge even when gbl_chg > 0.

hugepage_subpool_put_pages() can also restore rsv_hpages if concurrent frees move used_hpages below min_hpages between the get and put. When that happens on the gbl_chg > 0 path, restore the matching global reservation as well.

Skip the gbl_chg > 0 put when max_hpages is unset. For a min_size-only subpool, get_pages() did not change subpool state and put_pages() would create a false reservation.

Signed-off-by Zhao Li <[email protected]>

---
Changes in v2:
- Handle rsv_hpages restoration when racing frees cross min_hpages.
- Skip gbl_chg > 0 put_pages() when max_hpages is unset.

 mm/hugetlb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
mm/hugetlb.c +16 -5
index f24bf49be047e..4065d66fdcb5c 100644
@@ -3026,12 +3026,23 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 						    h_cg);
 out_subpool_put:
 	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
+	 * map_chg means hugepage_subpool_get_pages() succeeded above.
+	 * If max_hpages accounting was touched, undo it.  If racing frees
+	 * moved the subpool below min_hpages, the put path may restore a
+	 * subpool reservation.  Restore the matching global reservation too.
 	 */
-	if (map_chg && !gbl_chg) {
-		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+	if (map_chg) {
+		if (!gbl_chg) {
+			gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+			hugetlb_acct_memory(h, -gbl_reserve);
+		} else if (spool && spool->max_hpages != -1) {
+			gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+			if (!gbl_reserve) {
+				spin_lock_irq(&hugetlb_lock);
+				h->resv_huge_pages++;
+				spin_unlock_irq(&hugetlb_lock);
+			}
+		}
 	}

--
2.50.1 (Apple Git-155)

maintainer
#5 lore raw
Quoted 4 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For subpools with max_hpages, that increments used_hpages. If the later hugetlb cgroup charge fails, the unwind must undo that charge even when gbl_chg > 0.

I found that last sentence misleading, because we do not really care about hugetlb cgroup charge/uncharge (besides that being of the reasons we end up on error path) but rather the fact that we fiddle with subpool->used_hpages and we need to undo that when we rollback.

Quoted 4 lines

hugepage_subpool_put_pages() can also restore rsv_hpages if concurrent frees move used_hpages below min_hpages between the get and put. When that happens on the gbl_chg > 0 path, restore the matching global reservation as well.

Well, that does not quite explain the problem I think, at least not clear enough? So the problem at hand (IIUC) is that

1) if we took a global reservation
2) and concurrent hugepage_subpool_put_pages operations made used_hpages be below min_hpages, and so subpool's reservation was incremented, but we need to increment the global one as well otherwise the next time we pull a page from the spool, global resv_huge_pages will be inbalanced

Quoted 18 lines

Skip the gbl_chg > 0 put when max_hpages is unset. For a min_size-only subpool, get_pages() did not change subpool state and put_pages() would create a false reservation.

Signed-off-by Zhao Li <[email protected]>

---
Changes in v2:
- Handle rsv_hpages restoration when racing frees cross min_hpages.
- Skip gbl_chg > 0 put_pages() when max_hpages is unset.

 mm/hugetlb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
mm/hugetlb.c +0 -0
index f24bf49be047e..4065d66fdcb5c 100644

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3032

Quoted 9 lines
mm/hugetlb.c +4 -2
@@ -3026,12 +3026,23 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 						    h_cg);
 out_subpool_put:
 	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
+	 * map_chg means hugepage_subpool_get_pages() succeeded above.
+	 * If max_hpages accounting was touched, undo it.  If racing frees
+	 * moved the subpool below min_hpages, the put path may restore a
+	 * subpool reservation.  Restore the matching global reservation too.

I would split the comment in two parts and place them within the block they belong, otherwise it sounds confusing.
And maybe elaborate a little bit more.

Subpools, reservations and hugetlb make a very head-spinning situation, so let us make our life easier.

Signature · 2 lines
Oscar Salvador
SUSE Labs
#6 lore raw
Quoted 59 lines · Oscar Salvador
Quoted 4 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For subpools with max_hpages, that increments used_hpages. If the later hugetlb cgroup charge fails, the unwind must undo that charge even when gbl_chg > 0.

I found that last sentence misleading, because we do not really care about hugetlb cgroup charge/uncharge (besides that being of the reasons we end up on error path) but rather the fact that we fiddle with subpool->used_hpages and we need to undo that when we rollback.

Quoted 4 lines

hugepage_subpool_put_pages() can also restore rsv_hpages if concurrent frees move used_hpages below min_hpages between the get and put. When that happens on the gbl_chg > 0 path, restore the matching global reservation as well.

Well, that does not quite explain the problem I think, at least not clear enough? So the problem at hand (IIUC) is that

1) if we took a global reservation
2) and concurrent hugepage_subpool_put_pages operations made used_hpages be below min_hpages, and so subpool's reservation was incremented, but we need to increment the global one as well otherwise the next time we pull a page from the spool, global resv_huge_pages will be inbalanced

Quoted 18 lines

Skip the gbl_chg > 0 put when max_hpages is unset. For a min_size-only subpool, get_pages() did not change subpool state and put_pages() would create a false reservation.

Signed-off-by Zhao Li <[email protected]>

---
Changes in v2:
- Handle rsv_hpages restoration when racing frees cross min_hpages.
- Skip gbl_chg > 0 put_pages() when max_hpages is unset.

 mm/hugetlb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
mm/hugetlb.c +0 -0
index f24bf49be047e..4065d66fdcb5c 100644

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3032

Quoted 9 lines
mm/hugetlb.c +4 -2
@@ -3026,12 +3026,23 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 						    h_cg);
 out_subpool_put:
 	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
+	 * map_chg means hugepage_subpool_get_pages() succeeded above.
+	 * If max_hpages accounting was touched, undo it.  If racing frees
+	 * moved the subpool below min_hpages, the put path may restore a
+	 * subpool reservation.  Restore the matching global reservation too.

I would split the comment in two parts and place them within the block they belong, otherwise it sounds confusing.
And maybe elaborate a little bit more.

Quoted 2 lines

Subpools, reservations and hugetlb make a very head-spinning situation, so let us make our life easier.

Yep, the comment is confusing to me as well ...

+ if (map_chg) {
+ if (!gbl_chg) {
+ gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+ hugetlb_acct_memory(h, -gbl_reserve);
+ } else if (spool && spool->max_hpages != -1) {
+ gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+ if (!gbl_reserve) {
+ spin_lock_irq(&hugetlb_lock);
+ h->resv_huge_pages++;
+ spin_unlock_irq(&hugetlb_lock);
+ }
+ }
}

IIUC, there are three cases:

1) !gbl_chg

hugepage_subpool_get_pages() consumed one reservation from spool->rsv_hpages. So the error path needs to call hugepage_subpool_put_pages() and then drop the matching global reservation with hugetlb_acct_memory().

2) gbl_chg > 0 && spool->max_hpages != -1

hugepage_subpool_get_pages() did not consume spool->rsv_hpages, but it did increment spool->used_hpages after the max_hpages check passed. So the error path still needs to call hugepage_subpool_put_pages() to undo that.

If hugepage_subpool_put_pages() returns 0 here, it restored one reservation in spool->rsv_hpages, so we also need to increment h->resv_huge_pages.

3) gbl_chg > 0 && spool->max_hpages == -1

hugepage_subpool_get_pages() did not change spool->rsv_hpages or spool->used_hpages, so the error path should not call hugepage_subpool_put_pages(), otherwise it could create a false reservation in spool->rsv_hpages.

Hopefully I didn't miss anything :)
Lance

author
#7 lore raw
Quoted 7 lines · Lance Yang

IIUC, there are three cases:
[...]
2) gbl_chg > 0 && spool->max_hpages != -1
[...]
If hugepage_subpool_put_pages() returns 0 here, it restored one reservation in spool->rsv_hpages, so we also need to increment h->resv_huge_pages.

Thanks for working through the three cases - that's a clean breakdown and matches what v2 was trying to do. We ran into trouble on case 2 specifically: the h->resv_huge_pages++ on the put-returned-0 path looked right in isolation but turns out to be unsafe once you put it next to concurrent hugetlb_unreserve_pages() or free_huge_folio(). Two reachable orderings break it:

* free_huge_folio() with HPageRestoreReserve set already does h->resv_huge_pages++ on its own. v2's bump on the gbl_chg > 0 cleanup double-counts against that.

* hugetlb_unreserve_pages() does hugetlb_acct_memory(h, -X) which subtracts from h->resv_huge_pages and may also return surplus backing. v2's bump then leaves rsv_hpages backed by no h->resv_huge_pages - a phantom reservation that the next subpool_get_pages() consumes without real backing.

v3 ended up going a different way: the gbl_chg > 0 cleanup is now restricted to (max_hpages != -1, min_hpages == -1). In that configuration hugepage_subpool_put_pages()'s min-restoration branch is dead, so a direct used_hpages-- under spool->lock is the exact inverse of the speculative bump - no put_pages(), no h->resv_huge_pages++, no concurrent-races to reason about.

Your case 3 (max_hpages == -1) is unchanged: cleanup is a no-op, because get_pages() didn't touch any subpool field.

Mounts with min_hpages != -1 are left at v1 behaviour for now. That quadrant has an inherited race that also exists at hugetlb_reserve_pages()'s out_put_pages cleanup; a coordinated fix belongs in a separate RFC rather than this stable backport.

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

Thanks for the review.

--
Zhao Li

author
#8 lore raw
In reply to Oscar Salvador
Quoted 5 lines · Oscar Salvador

I found that last sentence misleading, because we do not really care about hugetlb cgroup charge/uncharge (besides that being of the reasons we end up on error path) but rather the fact that we fiddle with subpool->used_hpages and we need to undo that when we rollback.

Agreed - reframed in v3. The commit body now states the bug as the unwind missing the used_hpages rollback, without pinning it to the cgroup-charge case, and the subject is narrowed to "fix max-only subpool accounting on alloc_hugetlb_folio failure".

Quoted 2 lines

Well, that does not quite explain the problem I think, at least not clear enough? [...]

Fair - that explanation got tangled because v2's design itself was trying to compensate for racing min crossings. v3 sidesteps it
entirely: the gbl_chg > 0 cleanup is now restricted to (max_hpages != -1, min_hpages == -1). In that configuration hugepage_subpool_put_pages()'s min-restoration branch is dead, so a direct used_hpages-- under spool->lock is the exact inverse of the speculative bump - no h->resv_huge_pages++ needed, no rsv_hpages publication, no racing-put reasoning.

Mounts with min_hpages != -1 are left at v1 behaviour for now. That quadrant has an inherited race that also exists at hugetlb_reserve_pages()'s out_put_pages cleanup, so a coordinated fix belongs in a separate RFC rather than this stable backport.

Quoted 5 lines

I would split the comment in two parts and place them within the block they belong, otherwise it sounds confusing.

Subpools, reservations and hugetlb make a very head-spinning situation, so let us make our life easier.

Done - one short comment per branch placed inside the relevant code block in v3. Hopefully easier to follow now.

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

Thanks for the review.

--
Zhao Li

author
#9 lore raw

PATCH v3 1/1 mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

In reply to Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Fixes a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
Cc [email protected] # v6.15+
Signed-off-by Zhao Li <[email protected]>

---
Changes in v3:
- Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
- Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where the direct decrement is the exact inverse of the speculative bump.

Changes in v2:
- Skip the gbl_chg > 0 cleanup when max_hpages is unset.
- Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch.

 mm/hugetlb.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
mm/hugetlb.c +18 -7
index f24bf49be047e..cfdeaf6394c5b 100644
@@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),
 						    h_cg);
 out_subpool_put:
-	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
-	 */
-	if (map_chg && !gbl_chg) {
-		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+	if (map_chg) {
+		if (!gbl_chg) {
+			/* Full inverse when subpool_get_pages() consumed rsv_hpages. */
+			gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+			hugetlb_acct_memory(h, -gbl_reserve);
+		} else if (gbl_chg > 0 && spool && spool->min_hpages == -1 &&
+			   spool->max_hpages != -1) {
+			unsigned long flags;
+
+			/*
+			 * For max-only subpools, subpool_get_pages() took only a
+			 * speculative used_hpages slot. Drop that slot directly.
+			 */
+			spin_lock_irqsave(&spool->lock, flags);
+			if (spool->used_hpages > 0)
+				spool->used_hpages--;
+			unlock_or_release_subpool(spool, flags);
+		}
 	}

--
2.50.1 (Apple Git-155)

maintainer
#10 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

Quoted 25 lines · On Tue, 28 Apr 2026 19:30:38 +0800 Zhao Li <[email protected]> wrote:

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Quoted 3 lines

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Thanks.

I do like to see a clear statement of the user-visible effects of bugs when we fix them. "subpool quota leak" sounds bad, but how does this visibly manifest?

I asked you-know-what and came up with

Failed hugetlbfs page allocations can permanently consume the mount's size= quota without allocating a huge page. Repeated failures can make the filesystem appear full and cause later huge-page faults or allocations to fail with SIGBUS/allocation failure despite available huge pages and unused real filesystem capacity.

Which I'll paste in there. Because I do like to tell downstream people why we want a backport, and to help further downstream people to understand whether this might fix a problem they're having. Please lmk if it's inaccurate/incomplete.

I'll queue this as a backportable hotfix and shall await further reviewer input.

author
#11 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

Quoted 3 lines · Andrew Morton

I do like to see a clear statement of the user-visible effects of bugs when we fix them. "subpool quota leak" sounds bad, but how does this visibly manifest?

Thanks for picking this up. The added description accurately captures the max-only case.

The leftover used_hpages charge reduces the free space reported by statfs(), so the mount can appear full despite having fewer allocated huge pages than its configured size= limit. Later allocations can fail with ENOSPC, or page faults can result in SIGBUS, even when huge pages are available.

The patch leaves the min_size= reservation-accounting case unchanged. I'll make the user-visible impact clearer in future patch descriptions.

Thanks,
Zhao

#12 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

In reply to Zhao Li
Quoted 1 line · Zhao Li

Mounts with min_hpages != -1 are left unchanged for now.

In case it helps with that follow-up, here is a small reproducer for the min_size case. It needs no hugetlb cgroup.

The pool has two huge pages and no overcommit, and the mount is size=8M,min_size=4M. A test maps a 4-page file MAP_SHARED | MAP_NORESERVE, touches the first N pages, then unmaps and removes the file. Five rounds on one mount:

  N   faults ok / SIGBUS   statfs free / blocks   HugePages_Rsvd
  -                                               2 (after mount)
  2   2 / 0                4 / 4                  2
  2   2 / 0                4 / 4                  2
  4   2 / 2                2 / 4                  0
  2   2 / 0                2 / 4                  0
  2   2 / 0                2 / 4                  0

The two SIGBUS faults keep their used_hpages charge. Half of the quota stays gone with no file on the mount. used_hpages then no longer drops below min_hpages, so hugepage_subpool_put_pages() never restores rsv_hpages, and the mount has lost its minimum reservation for good.

The out_subpool_put: path skips this case on purpose, as the commit message says.

Same result on v7.3-rc4 (f0100363d8c3), on v7.3-rc4 with this patch, and on next-20260922. That tree also has 6f8b22e85eac ("mm/hugetlb: fix subpool minimum reservation rollback"), which fixes the hugetlb_reserve_pages() error path. This is the fault path. Tested under QEMU x86_64, one CPU.

Thanks,
Karl

#include <fcntl.h>
#include <setjmp.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/statfs.h>
#include <unistd.h>

#define HPAGE (2UL << 20)

static sigjmp_buf env;

static void on_sigbus(int sig)

{
	siglongjmp(env, 1);
}

/* usage: t <hugetlbfs dir> <pages to touch, at most 4> */ int main(int argc, char **argv)

{
	int n = atoi(argv[2]), fd, i;
	volatile int ok = 0, bus = 0;
	struct statfs st;
	char path[256];
	char *p;

	signal(SIGBUS, on_sigbus);
	snprintf(path, sizeof(path), "%s/f", argv[1]);
	fd = open(path, O_CREAT | O_RDWR, 0600);
	if (fd < 0 || ftruncate(fd, 4 * HPAGE))
		return 1;
	p = mmap(NULL, 4 * HPAGE, PROT_READ | PROT_WRITE,
		 MAP_SHARED | MAP_NORESERVE, fd, 0);
	if (p == MAP_FAILED)
		return 1;
	for (i = 0; i < n; i++) {
		if (!sigsetjmp(env, 1)) {
			p[i * HPAGE] = 1;
			ok++;
		} else {
			bus++;
		}
	}
	munmap(p, 4 * HPAGE);
	close(fd);
	unlink(path);
	statfs(argv[1], &st);
	printf("ok=%d sigbus=%d free=%ld/%ld\n", ok, bus,
	       (long)st.f_bfree, (long)st.f_blocks);
	return 0;
}

echo 0 > /proc/sys/vm/nr_overcommit_hugepages
echo 2 > /proc/sys/vm/nr_hugepages
mount -t hugetlbfs -o size=8M,min_size=4M none /mnt/h
for n in 2 2 4 2 2; do ./t /mnt/h $n; grep HugePages_Rsvd /proc/meminfo; done

Tested-by on v3 1/1
#13 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

In reply to Zhao Li
Quoted 84 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Fixes a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
Cc [email protected] # v6.15+
Signed-off-by Zhao Li <[email protected]>

---
Changes in v3:
- Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
- Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where the direct decrement is the exact inverse of the speculative bump.

Changes in v2:
- Skip the gbl_chg > 0 cleanup when max_hpages is unset.
- Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch.

 mm/hugetlb.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
mm/hugetlb.c +18 -8
index f24bf49be047e..cfdeaf6394c5b 100644
@@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),
 						    h_cg);
 out_subpool_put:
-	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
-	 */
-	if (map_chg && !gbl_chg) {
-		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+	if (map_chg) {
+		if (!gbl_chg) {
+			/* Full inverse when subpool_get_pages() consumed rsv_hpages. */
+			gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+			hugetlb_acct_memory(h, -gbl_reserve);
+		} else if (gbl_chg > 0 && spool && spool->min_hpages == -1 &&
+			   spool->max_hpages != -1) {
+			unsigned long flags;
+
+			/*
+			 * For max-only subpools, subpool_get_pages() took only a
+			 * speculative used_hpages slot. Drop that slot directly.
+			 */
+			spin_lock_irqsave(&spool->lock, flags);
+			if (spool->used_hpages > 0)
+				spool->used_hpages--;
+			unlock_or_release_subpool(spool, flags);
+		}
 	}
 
 
--
Quoted 1 line

2.50.1 (Apple Git-155)

This fixes out_subpool_put in alloc_hugetlb_folio() on a mount that specifies max_size only, and does not introduce any further bugs AFAICT.

Tested-by Ackerley Tng <[email protected]>

Further details at [1].

[1] https://lore.kernel.org/all/CAEvNRgHQLifmcD1NO6cd7EVcCiQb6gJgNUo4HkFhZAvzuwMq0w@mail.gmail.com/

maintainer
#14 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

In reply to Zhao Li
Quoted 62 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Fixes a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
Cc [email protected] # v6.15+
Signed-off-by Zhao Li <[email protected]>

---
Changes in v3:
- Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
- Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where the direct decrement is the exact inverse of the speculative bump.

Changes in v2:
- Skip the gbl_chg > 0 cleanup when max_hpages is unset.
- Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch.

mm/hugetlb.c | 25 ++++++++++++++++++-------

1 file changed, 18 insertions(+), 7 deletions(-)
mm/hugetlb.c +0 -0
index f24bf49be047e..cfdeaf6394c5b 100644
@@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),
   h_cg);

out_subpool_put:

mm/hugetlb.c +0 -3
- 	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3033

Quoted 10 lines
mm/hugetlb.c +6 -4
-	 */
- 	if (map_chg && !gbl_chg) {
- 		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
- 		hugetlb_acct_memory(h, -gbl_reserve);
+ 	if (map_chg) {
+ 		if (!gbl_chg) {
+ 			/* Full inverse when subpool_get_pages() consumed rsv_hpages. */
+ 			gbl_reserve = hugepage_subpool_put_pages(spool, 1);
+			hugetlb_acct_memory(h, -gbl_reserve);
+ 		} else if (gbl_chg > 0 && spool && spool->min_hpages == -1 &&

The gbl_chg > 0 check can be dropped: negative values jump directly to out_end_reservation, while zero is handled by the preceding if (!gbl_chg).

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3044

Quoted 11 lines
mm/hugetlb.c +11 -0
+   			spool->max_hpages != -1) {
+ 			unsigned long flags;
+
+ 			/*
+			 * For max-only subpools, subpool_get_pages() took only a
+			 * speculative used_hpages slot. Drop that slot directly.
+			 */
+ 			spin_lock_irqsave(&spool->lock, flags);
+ 			if (spool->used_hpages > 0)
+ 				spool->used_hpages--;
+ 			unlock_or_release_subpool(spool, flags);

Could we use hugepage_subpool_put_pages(spool, 1) here?

With the max-only check in place, the helper cannot restore rsv_hpages, so it already performs the required used_hpages decrement and subpool lifetime handling under the appropriate lock.

Thanks,
Muchun

Quoted 6 lines
mm/hugetlb.c +1 -0
+ 		}

}

--
2.50.1 (Apple Git-155)

reviewer
#15 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

In reply to Zhao Li
Quoted 53 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Fixes a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
Cc [email protected] # v6.15+
Signed-off-by Zhao Li <[email protected]>

---
Changes in v3:
- Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
- Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where the direct decrement is the exact inverse of the speculative bump.

Changes in v2:
- Skip the gbl_chg > 0 cleanup when max_hpages is unset.
- Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch.

 mm/hugetlb.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
mm/hugetlb.c +0 -0
index f24bf49be047e..cfdeaf6394c5b 100644
@@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3028

Quoted 10 lines
mm/hugetlb.c +1 -7
 						    h_cg);
 out_subpool_put:
-	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
-	 */
-	if (map_chg && !gbl_chg) {
-		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+	if (map_chg) {

I agree with Muchuns comment about removing the "gbl_chg > 0" check.

You could consider the following structure, which reduces indentation and avoids touching the existing handling.

if (map_chg && !gbl_chg) {
	...
} else if (map_chg && spool && spool->min_hpages == -1 &&
	   spool->max_hpages != -1) {
	...
}

But the code would read a lot nicer as

if (map_chg && !gbl_chg) {
	...
} else if (map_chg) {

hugepage_subpool_XXX_pages()
}

Whereby the spool checks etc are handled in there.

Muchun things that we might just be able to use hugepage_subpool_put_pages(). It that's possible, great. Otherwise create a new helper that wraps all the logic below.

Signature · 3 lines
Cheers,

David
reviewer
#16 lore raw

Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

Quoted 83 lines · David Hildenbrand (Arm)
Quoted 53 lines · Zhao Li

alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg is set. For a subpool with max_hpages != -1, that bumps used_hpages regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or gbl_chg > 0 (used_hpages slot only). If the allocation later fails before a folio is returned, the unwind must undo the used_hpages bump. The old cleanup only ran for !gbl_chg, leaking used_hpages on the gbl_chg > 0 path.

For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages == -1), hugepage_subpool_get_pages() took only a speculative used_hpages slot. Drop that slot directly under spool->lock. In that configuration hugepage_subpool_put_pages() cannot restore rsv_hpages, so the direct decrement is the exact inverse and is race-free against concurrent puts. This matches the used_hpages-only part of hugetlb_reserve_pages()'s out_put_pages cleanup, but restricts it to the max-only case where no rsv_hpages restoration is possible.

Mounts with min_hpages != -1 are left unchanged for now. v2's approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to back a restored rsv_hpages slot) double-counts global backing under concurrent free_huge_folio() and creates phantom reservations under concurrent hugetlb_unreserve_pages(). Safe cleanup of that quadrant needs a coordinated fix across multiple call sites.

Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb cgroup whose limit is exceeded. Vanilla leaks 6/8 hugepages of subpool quota; this patch leaks 0/8. Verified under QEMU.

Fixes a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
Cc [email protected] # v6.15+
Signed-off-by Zhao Li <[email protected]>

---
Changes in v3:
- Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
- Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where the direct decrement is the exact inverse of the speculative bump.

Changes in v2:
- Skip the gbl_chg > 0 cleanup when max_hpages is unset.
- Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the gbl_chg > 0 branch.

 mm/hugetlb.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
mm/hugetlb.c +0 -0
index f24bf49be047e..cfdeaf6394c5b 100644
@@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),

mm/hugetlb.c struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, · line 3028

Quoted 10 lines
mm/hugetlb.c +1 -7
 						    h_cg);
 out_subpool_put:
-	/*
-	 * put page to subpool iff the quota of subpool's rsv_hpages is used
-	 * during hugepage_subpool_get_pages.
-	 */
-	if (map_chg && !gbl_chg) {
-		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
-		hugetlb_acct_memory(h, -gbl_reserve);
+	if (map_chg) {

I agree with Muchuns comment about removing the "gbl_chg > 0" check.

You could consider the following structure, which reduces indentation and avoids touching the existing handling.

if (map_chg && !gbl_chg) {
	...
} else if (map_chg && spool && spool->min_hpages == -1 &&
	   spool->max_hpages != -1) {
	...
}

But the code would read a lot nicer as

if (map_chg && !gbl_chg) {
	...
} else if (map_chg) {
Quoted 9 lines

hugepage_subpool_XXX_pages()
}

Whereby the spool checks etc are handled in there.

Muchun things that we might just be able to use hugepage_subpool_put_pages(). It that's possible, great. Otherwise create a new helper that wraps all the logic below.

Any update on this?

Signature · 3 lines
Cheers,

David