[PATCH RFC v2 0/3] tweaks for __alloc_pages_slowpath()

[PATCH RFC v2 0/3] tweaks for __alloc_pages_slowpath()

[Vlastimil Babka]

RFC, feel free to ignore until next year... Sending just in case
some feedback will materialize.

Since v1 the main motivating fix (patch 1 of v1) was sent separately:
https://lore.kernel.org/all/20251219-costly-noretry-thisnode-fix-v1-1-e1085a4a0c34@suse.cz/

In patch 2 of v1 I have proposed not to attempt reclaim for costly
__GFP_NORETRY allocations at all. Johannes suggested that we should not
change the semantics that much, so now instead we allow them all (except
with __GFP_THISNODE). The main idea that remains is that we don't decide
based on the exact compaction result anymore.

Patch 3 is a cleanup also based on a suggestion from Johannes, and Patch
4 became the obvious next step in that direction. These should be making
no functional changes.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
Changes in v2:
- actual THP reclaim fix sent separately
- allow one reclaim attempt for __GFP_NORETRY allocations
- further __alloc_pages_slowpath() cleanups
- Link to v1: https://patch.msgid.link/20251216-thp-thisnode-tweak-v1-0-0e499d13d2eb@suse.cz

---
Vlastimil Babka (3):
      mm/page_alloc: ignore the exact initial compaction result
      mm/page_alloc: refactor the initial compaction handling
      mm/page_alloc: simplify __alloc_pages_slowpath() flow

 mm/page_alloc.c | 169 ++++++++++++++++++++++++++------------------------------
 1 file changed, 77 insertions(+), 92 deletions(-)
---
base-commit: 81a5997c0d5ac4cb3ce65f1128197cb751ccd15b
change-id: 20251216-thp-thisnode-tweak-c9c2acb3a627

Best regards,
-- 
Vlastimil Babka <vbabka@suse.cz>

[PATCH RFC v2 1/3] mm/page_alloc: ignore the exact initial compaction result

[Vlastimil Babka]

For allocations that are of costly order and __GFP_NORETRY (and can
perform compaction) we attempt direct compaction first. If that fails,
we continue with a single round of direct reclaim+compaction (as for
other __GFP_NORETRY allocations, except the compaction is of lower
priority), with two exceptions that fail immediately:

- __GFP_THISNODE is specified, to prevent zone_reclaim_mode-like
  behavior for e.g. THP page faults

- compaction failed because it was deferred (i.e. has been failing
  recently so further attempts are not done for a while) or skipped,
  which means there are insufficient free base pages to defragment to
  begin with

Upon closer inspection, the second condition has a somewhat flawed
reasoning. If there are not enough base pages and reclaim could create
them, we instead fail. When there are enough base pages and compaction
has already ran and failed, we proceed and hope that reclaim and the
subsequent compaction attempt will succeed. But it's unclear why they
should and whether it will be as inexpensive as intended.

It might make therefore more sense to just fail unconditionally after
the initial compaction attempt. However that would change the semantics
of __GFP_NORETRY to attempt reclaim at least once.

Alternatively we can remove the compaction result checks and proceed
with the single reclaim and (lower priority) compaction attempt, leaving
only the __GFP_THISNODE exception for failing immediately.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6f5e1b902999..9e7b0967f1b5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4767,44 +4767,22 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 * includes some THP page fault allocations
 		 */
 		if (costly_order && (gfp_mask & __GFP_NORETRY)) {
-			/*
-			 * If allocating entire pageblock(s) and compaction
-			 * failed because all zones are below low watermarks
-			 * or is prohibited because it recently failed at this
-			 * order, fail immediately unless the allocator has
-			 * requested compaction and reclaim retry.
-			 *
-			 * Reclaim is
-			 *  - potentially very expensive because zones are far
-			 *    below their low watermarks or this is part of very
-			 *    bursty high order allocations,
-			 *  - not guaranteed to help because isolate_freepages()
-			 *    may not iterate over freed pages as part of its
-			 *    linear scan, and
-			 *  - unlikely to make entire pageblocks free on its
-			 *    own.
-			 */
-			if (compact_result == COMPACT_SKIPPED ||
-			    compact_result == COMPACT_DEFERRED)
-				goto nopage;
-
 			/*
 			 * THP page faults may attempt local node only first,
 			 * but are then allowed to only compact, not reclaim,
 			 * see alloc_pages_mpol().
 			 *
-			 * Compaction can fail for other reasons than those
-			 * checked above and we don't want such THP allocations
-			 * to put reclaim pressure on a single node in a
-			 * situation where other nodes might have plenty of
-			 * available memory.
+			 * Compaction has failed above and we don't want such
+			 * THP allocations to put reclaim pressure on a single
+			 * node in a situation where other nodes might have
+			 * plenty of available memory.
 			 */
 			if (gfp_mask & __GFP_THISNODE)
 				goto nopage;
 
 			/*
-			 * Looks like reclaim/compaction is worth trying, but
-			 * sync compaction could be very expensive, so keep
+			 * Proceed with single round of reclaim/compaction, but
+			 * since sync compaction could be very expensive, keep
 			 * using async compaction.
 			 */
 			compact_priority = INIT_COMPACT_PRIORITY;

-- 
2.52.0

[PATCH RFC v2 2/3] mm/page_alloc: refactor the initial compaction handling

[Vlastimil Babka]

The initial direct compaction done in some cases in
__alloc_pages_slowpath() stands out from the main retry loop of
reclaim + compaction.

We can simplify this by instead skipping the initial reclaim attempt via
a new local variable compact_first, and handle the compact_prority to
match the original behavior.

Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 106 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9e7b0967f1b5..cb8965fd5e20 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4512,6 +4512,11 @@ static bool oom_reserves_allowed(struct task_struct *tsk)
 	return true;
 }
 
+static inline bool gfp_thisnode_noretry(gfp_t gfp_mask)
+{
+	return (gfp_mask & __GFP_NORETRY) && (gfp_mask & __GFP_THISNODE);
+}
+
 /*
  * Distinguish requests which really need access to full memory
  * reserves from oom victims which can live with a portion of it
@@ -4664,7 +4669,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 						struct alloc_context *ac)
 {
 	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
-	bool can_compact = gfp_compaction_allowed(gfp_mask);
+	bool can_compact = can_direct_reclaim && gfp_compaction_allowed(gfp_mask);
 	bool nofail = gfp_mask & __GFP_NOFAIL;
 	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
 	struct page *page = NULL;
@@ -4677,6 +4682,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	unsigned int cpuset_mems_cookie;
 	unsigned int zonelist_iter_cookie;
 	int reserve_flags;
+	bool compact_first = false;
 
 	if (unlikely(nofail)) {
 		/*
@@ -4700,6 +4706,19 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	cpuset_mems_cookie = read_mems_allowed_begin();
 	zonelist_iter_cookie = zonelist_iter_begin();
 
+	/*
+	 * For costly allocations, try direct compaction first, as it's likely
+	 * that we have enough base pages and don't need to reclaim. For non-
+	 * movable high-order allocations, do that as well, as compaction will
+	 * try prevent permanent fragmentation by migrating from blocks of the
+	 * same migratetype.
+	 */
+	if (can_compact && (costly_order || (order > 0 &&
+					ac->migratetype != MIGRATE_MOVABLE))) {
+		compact_first = true;
+		compact_priority = INIT_COMPACT_PRIORITY;
+	}
+
 	/*
 	 * The fast path uses conservative alloc_flags to succeed only until
 	 * kswapd needs to be woken up, and to avoid the cost of setting up
@@ -4742,53 +4761,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
-	/*
-	 * For costly allocations, try direct compaction first, as it's likely
-	 * that we have enough base pages and don't need to reclaim. For non-
-	 * movable high-order allocations, do that as well, as compaction will
-	 * try prevent permanent fragmentation by migrating from blocks of the
-	 * same migratetype.
-	 * Don't try this for allocations that are allowed to ignore
-	 * watermarks, as the ALLOC_NO_WATERMARKS attempt didn't yet happen.
-	 */
-	if (can_direct_reclaim && can_compact &&
-			(costly_order ||
-			   (order > 0 && ac->migratetype != MIGRATE_MOVABLE))
-			&& !gfp_pfmemalloc_allowed(gfp_mask)) {
-		page = __alloc_pages_direct_compact(gfp_mask, order,
-						alloc_flags, ac,
-						INIT_COMPACT_PRIORITY,
-						&compact_result);
-		if (page)
-			goto got_pg;
-
-		/*
-		 * Checks for costly allocations with __GFP_NORETRY, which
-		 * includes some THP page fault allocations
-		 */
-		if (costly_order && (gfp_mask & __GFP_NORETRY)) {
-			/*
-			 * THP page faults may attempt local node only first,
-			 * but are then allowed to only compact, not reclaim,
-			 * see alloc_pages_mpol().
-			 *
-			 * Compaction has failed above and we don't want such
-			 * THP allocations to put reclaim pressure on a single
-			 * node in a situation where other nodes might have
-			 * plenty of available memory.
-			 */
-			if (gfp_mask & __GFP_THISNODE)
-				goto nopage;
-
-			/*
-			 * Proceed with single round of reclaim/compaction, but
-			 * since sync compaction could be very expensive, keep
-			 * using async compaction.
-			 */
-			compact_priority = INIT_COMPACT_PRIORITY;
-		}
-	}
-
 retry:
 	/*
 	 * Deal with possible cpuset update races or zonelist updates to avoid
@@ -4832,10 +4804,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		goto nopage;
 
 	/* Try direct reclaim and then allocating */
-	page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
-							&did_some_progress);
-	if (page)
-		goto got_pg;
+	if (!compact_first) {
+		page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags,
+							ac, &did_some_progress);
+		if (page)
+			goto got_pg;
+	}
 
 	/* Try direct compaction and then allocating */
 	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
@@ -4843,6 +4817,34 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (compact_first) {
+		/*
+		 * THP page faults may attempt local node only first, but are
+		 * then allowed to only compact, not reclaim, see
+		 * alloc_pages_mpol().
+		 *
+		 * Compaction has failed above and we don't want such THP
+		 * allocations to put reclaim pressure on a single node in a
+		 * situation where other nodes might have plenty of available
+		 * memory.
+		 */
+		if (gfp_thisnode_noretry(gfp_mask))
+			goto nopage;
+
+		/*
+		 * For the initial compaction attempt we have lowered its
+		 * priority. Restore it for further retries. With __GFP_NORETRY
+		 * there will be a single round of reclaim+compaction with the
+		 * lowered priority.
+		 */
+		if (!(gfp_mask & __GFP_NORETRY)) {
+			compact_priority = DEF_COMPACT_PRIORITY;
+		}
+
+		compact_first = false;
+		goto retry;
+	}
+
 	/* Do not loop if specifically requested */
 	if (gfp_mask & __GFP_NORETRY)
 		goto nopage;

-- 
2.52.0

Re: [PATCH RFC v2 2/3] mm/page_alloc: refactor the initial compaction handling

[Joshua Hahn]

On Fri, 19 Dec 2025 18:38:52 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:

Hi Vlastimil,

I hope you are doing well, sorry for the late reply. The patch overall looks
good to me, but I have a few very small nits.

> The initial direct compaction done in some cases in
> __alloc_pages_slowpath() stands out from the main retry loop of
> reclaim + compaction.
> 
> We can simplify this by instead skipping the initial reclaim attempt via
> a new local variable compact_first, and handle the compact_prority to
> match the original behavior.
> 
> Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/page_alloc.c | 106 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 54 insertions(+), 52 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9e7b0967f1b5..cb8965fd5e20 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4512,6 +4512,11 @@ static bool oom_reserves_allowed(struct task_struct *tsk)
>  	return true;
>  }
>  
> +static inline bool gfp_thisnode_noretry(gfp_t gfp_mask)
> +{
> +	return (gfp_mask & __GFP_NORETRY) && (gfp_mask & __GFP_THISNODE);
> +}

NIT: Is there a reason why this was turned into its own function? The checks
seem short enough to open-code and it seems like there's only one caller as
far as I can tell. Actually I think there are some better candidates of
turning boolean checks into functions, like the one below:

[...snip...]

> +	/*
> +	 * For costly allocations, try direct compaction first, as it's likely
> +	 * that we have enough base pages and don't need to reclaim. For non-
> +	 * movable high-order allocations, do that as well, as compaction will
> +	 * try prevent permanent fragmentation by migrating from blocks of the
> +	 * same migratetype.
> +	 */
> +	if (can_compact && (costly_order || (order > 0 &&
> +					ac->migratetype != MIGRATE_MOVABLE))) {
> +		compact_first = true;
> +		compact_priority = INIT_COMPACT_PRIORITY;
> +	}
> +

It has indeed become shorter thanks to this patch, but I think if we want to
make the code more "readable" we can stay consistent and move these into thier
own boolean checks, or just leave them open-coded. No strong preference here,
just wanted to offer my 2c and hear what you think.

[...snip...]

> +		/*
> +		 * For the initial compaction attempt we have lowered its
> +		 * priority. Restore it for further retries. With __GFP_NORETRY
> +		 * there will be a single round of reclaim+compaction with the
> +		 * lowered priority.
> +		 */
> +		if (!(gfp_mask & __GFP_NORETRY)) {
> +			compact_priority = DEF_COMPACT_PRIORITY;
> +		}

NIT: I think these braces are unecessary : -)

Otherwise, LGTM! Thank you for your work, it looks a lot cleaner.

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

[PATCH RFC v2 3/3] mm/page_alloc: simplify __alloc_pages_slowpath() flow

[Vlastimil Babka]

The actions done before entering the main retry loop include waking up
kswapds and an allocation attempt with the precise alloc_flags.
Then in the loop we keep waking up kswapds, and we retry the allocation
with flags potentially further adjusted by being allowed to use reserves
(due to e.g. becoming an oom victim).

We can adjust the retry loop to keep only one instance of waking up
kswapds and allocation attempt. Introduce a can_retry_reserves variable
for retrying once when we become eligible for reserves. It is still
useful not to evaluate reserve_flags immediately for the first
allocation attempt, because it's better to first try succeed in a
non-preferred zone above the min watermark before allocating immediately
from the preferred zone below min watermark.

Additionaly move the cpuset update checks introduced by e05741fb10c3
("mm/page_alloc.c: avoid infinite retries caused by cpuset race")
further in the retry loop. It's enough to check those only before
reaching any potentially infinite 'goto retry;' loop.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cb8965fd5e20..4a68adb383b2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4683,6 +4683,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	unsigned int zonelist_iter_cookie;
 	int reserve_flags;
 	bool compact_first = false;
+	bool can_retry_reserves = true;
 
 	if (unlikely(nofail)) {
 		/*
@@ -4750,6 +4751,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 			goto nopage;
 	}
 
+retry:
+	/* Ensure kswapd doesn't accidentally go to sleep as long as we loop */
 	if (alloc_flags & ALLOC_KSWAPD)
 		wake_all_kswapds(order, gfp_mask, ac);
 
@@ -4761,19 +4764,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
-retry:
-	/*
-	 * Deal with possible cpuset update races or zonelist updates to avoid
-	 * infinite retries.
-	 */
-	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
-	    check_retry_zonelist(zonelist_iter_cookie))
-		goto restart;
-
-	/* Ensure kswapd doesn't accidentally go to sleep as long as we loop */
-	if (alloc_flags & ALLOC_KSWAPD)
-		wake_all_kswapds(order, gfp_mask, ac);
-
 	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
 	if (reserve_flags)
 		alloc_flags = gfp_to_alloc_flags_cma(gfp_mask, reserve_flags) |
@@ -4788,12 +4778,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		ac->nodemask = NULL;
 		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
 					ac->highest_zoneidx, ac->nodemask);
-	}
 
-	/* Attempt with potentially adjusted zonelist and alloc_flags */
-	page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac);
-	if (page)
-		goto got_pg;
+		/*
+		 * The first time we adjust anything due to being allowed to
+		 * ignore memory policies or watermarks, retry immediately. This
+		 * allows us to keep the first allocation attempt optimistic so
+		 * it can succeed in a zone that is still above watermarks.
+		 */
+		if (can_retry_reserves) {
+			can_retry_reserves = false;
+			goto retry;
+		}
+	}
 
 	/* Caller is not willing to reclaim, we can't balance anything */
 	if (!can_direct_reclaim)
@@ -4857,6 +4853,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 			     !(gfp_mask & __GFP_RETRY_MAYFAIL)))
 		goto nopage;
 
+	/*
+	 * Deal with possible cpuset update races or zonelist updates to avoid
+	 * infinite retries. No "goto retry;" can go above this check unless
+	 * it can execute just once.
+	 */
+	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
+	    check_retry_zonelist(zonelist_iter_cookie))
+		goto restart;
+
 	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
 				 did_some_progress > 0, &no_progress_loops))
 		goto retry;

-- 
2.52.0