mm: let node_reclaim() return the number of pages reclaimed

PATCH Superseded Petr Tesarik 5 participants 6 messages Jul 10 – Jul 13

Subsystems (5)

Memory management MM core Page allocator MGLRU +1 more subsystems

lore

Share & tools

lore

Petr Tesarik

author

2026-07-10 15:23 UTC #0 lore raw

PATCH v3 1/1 mm: let node_reclaim() return the number of pages reclaimed

There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.

Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.

There is one small change of behavior when __node_reclaim() was attempted but returned zero. The allocation now skips the zone immediately; before this patch, the zone watermarks were checked first. I believe it was an oversight rather than intention, because the chances that zone watermark is OK after __node_reclaim() did not reclaim any pages are very close to zero.

Originally, I was looking for occurences of NODE_RECLAIM_SOME and NODE_RECLAIM_SUCCESS, but I couldn't find any. That's because they are typecast from the result of a relational operator. This seemed a bit fragile, so I dug a bit deeper and came up with this proposed cleanup.

Signed-off-by Petr Tesarik <[email protected]>

Changes from v2:
- remove the enum and return the number of pages reclaimed

Changes from v1:
- use an enum instead of a bool

---
 mm/internal.h   |  9 +++++----
 mm/page_alloc.c | 19 ++++---------------
 mm/vmscan.c     | 16 ++++++++--------
 3 files changed, 17 insertions(+), 27 deletions(-)

mm/internal.h +5 -4

index e5dcab69eb4f6..585705daad1a5 100644
@@ -1102,15 +1102,16 @@ static inline void mlock_drain_remote(int cpu) { }
 #ifdef CONFIG_NUMA
 extern int node_reclaim_mode;
 
-extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
+extern unsigned long node_reclaim(struct pglist_data *pgdat,
+				  gfp_t gfp_mask, unsigned int order);
 extern int find_next_best_node(int node, nodemask_t *used_node_mask);
 #else
 #define node_reclaim_mode 0
 
-static inline int node_reclaim(struct pglist_data *pgdat, gfp_t mask,
-				unsigned int order)
+static inline unsigned long node_reclaim(struct pglist_data *pgdat,
+					 gfp_t mask, unsigned int order)
 {
-	return NODE_RECLAIM_NOSCAN;
+	return 0;
 }
 static inline int find_next_best_node(int node, nodemask_t *used_node_mask)
 {

mm/page_alloc.c +4 ~~-15~~

index 9c97a86da2b9f..af63558391345 100644
@@ -3908,8 +3908,6 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 		if (!zone_watermark_fast(zone, order, mark,
 				       ac->highest_zoneidx, alloc_flags,
 				       gfp_mask)) {
-			int ret;
-
 			if (cond_accept_memory(zone, order, alloc_flags))
 				goto try_this_zone;

@@ -3930,22 +3928,13 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 			    !zone_allows_reclaim(zonelist_zone(ac->preferred_zoneref), zone))
 				continue;
 
-			ret = node_reclaim(zone->zone_pgdat, gfp_mask, order);
-			switch (ret) {
-			case NODE_RECLAIM_NOSCAN:
-				/* did not scan */
-				continue;
-			case NODE_RECLAIM_FULL:
-				/* scanned but unreclaimable */
+			if (!node_reclaim(zone->zone_pgdat, gfp_mask, order))
 				continue;
-			default:
-				/* did we reclaim enough */
-				if (zone_watermark_ok(zone, order, mark,
-					ac->highest_zoneidx, alloc_flags))
-					goto try_this_zone;
 
+			/* did we reclaim enough */
+			if (!zone_watermark_ok(zone, order, mark,
+					       ac->highest_zoneidx, alloc_flags))
 				continue;
-			}
 		}
 
 try_this_zone:

mm/vmscan.c +8 -8

index f8faa7d06329b..639ea6175c98d 100644
@@ -7800,9 +7800,9 @@ static unsigned long __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask,
 	return sc->nr_reclaimed;
 }
 
-int node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
+unsigned long node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
 {
-	int ret;
+	unsigned long ret;
 	/* Minimum pages needed in order to stay on node */
 	const unsigned long nr_pages = 1 << order;
 	struct scan_control sc = {

@@ -7829,13 +7829,13 @@ int node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
 	if (node_pagecache_reclaimable(pgdat) <= pgdat->min_unmapped_pages &&
 	    node_page_state_pages(pgdat, NR_SLAB_RECLAIMABLE_B) <=
 	    pgdat->min_slab_pages)
-		return NODE_RECLAIM_FULL;
+		return 0;
 
 	/*
 	 * Do not scan if the allocation should not be delayed.
 	 */
 	if (!gfpflags_allow_blocking(gfp_mask) || (current->flags & PF_MEMALLOC))
-		return NODE_RECLAIM_NOSCAN;
+		return 0;
 
 	/*
 	 * Only run node reclaim on the local node or on nodes that do not

@@ -7844,15 +7844,15 @@ int node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
 	 * as wide as possible.
 	 */
 	if (node_state(pgdat->node_id, N_CPU) && pgdat->node_id != numa_node_id())
-		return NODE_RECLAIM_NOSCAN;
+		return 0;
 
 	if (test_and_set_bit_lock(PGDAT_RECLAIM_LOCKED, &pgdat->flags))
-		return NODE_RECLAIM_NOSCAN;
+		return 0;
 
-	ret = __node_reclaim(pgdat, gfp_mask, nr_pages, &sc) >= nr_pages;
+	ret = __node_reclaim(pgdat, gfp_mask, nr_pages, &sc);
 	clear_bit_unlock(PGDAT_RECLAIM_LOCKED, &pgdat->flags);
 
-	if (ret)
+	if (ret >= nr_pages)
 		count_vm_event(PGSCAN_ZONE_RECLAIM_SUCCESS);
 	else
 		count_vm_event(PGSCAN_ZONE_RECLAIM_FAILED);

Signature · 1 line

2.54.0

Andrew Morton

maintainer

2026-07-10 23:27 UTC #1 lore raw

Quoted 7 lines · On Fri, 10 Jul 2026 17:23:20 +0200 Petr Tesarik <[email protected]> wrote:

There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.
Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.

AI review
(https://sashiko.dev/#/patchset/[email protected])
points out that the macros weren't actually removed. Perhaps you meant "use of the macros can be removed" or similar. But it does seem they can indeed be removed:

hp2:/usr/src/25> grep NODE_RECLAIM_ mm/*.[ch] include/linux/*.h
mm/internal.h:#define NODE_RECLAIM_NOSCAN -2
mm/internal.h:#define NODE_RECLAIM_FULL -1
mm/internal.h:#define NODE_RECLAIM_SOME 0
mm/internal.h:#define NODE_RECLAIM_SUCCESS 1
mm/vmscan.c:#define NODE_RECLAIM_PRIORITY 4
mm/vmscan.c: .priority = NODE_RECLAIM_PRIORITY,

(NODE_RECLAIM_PRIORITY looks like it belongs, but it's an unrelated thing).

Let's not do an immediate resend to address this please - let's give reviewers a week or so to do their thing.

Johannes Weiner

maintainer Acked-by on v3 1/1

2026-07-13 11:04 UTC #2 lore raw

In reply to Petr Tesarik

Quoted 14 lines · Petr Tesarik

There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.
Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.
There is one small change of behavior when __node_reclaim() was attempted but returned zero. The allocation now skips the zone immediately; before this patch, the zone watermarks were checked first. I believe it was an oversight rather than intention, because the chances that zone watermark is OK after __node_reclaim() did not reclaim any pages are very close to zero.

Quoted 6 lines

Originally, I was looking for occurences of NODE_RECLAIM_SOME and NODE_RECLAIM_SUCCESS, but I couldn't find any. That's because they are typecast from the result of a relational operator. This seemed a bit fragile, so I dug a bit deeper and came up with this proposed cleanup.
Signed-off-by Petr Tesarik <[email protected]>

Looks good to me, and agree with the minor change in behavior being acceptable. With the macros actually removed,

Acked-by Johannes Weiner <[email protected]>

Petr Tesarik

author

2026-07-13 12:55 UTC #3 lore raw

V Mon, 13 Jul 2026 07:04:43 -0400
Johannes Weiner <[email protected]> napsáno:

Quoted 20 lines · Petr Tesarik

Quoted 19 lines
There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.
Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.
There is one small change of behavior when __node_reclaim() was attempted but returned zero. The allocation now skips the zone immediately; before this patch, the zone watermarks were checked first. I believe it was an oversight rather than intention, because the chances that zone watermark is OK after __node_reclaim() did not reclaim any pages are very close to zero.
Originally, I was looking for occurences of NODE_RECLAIM_SOME and NODE_RECLAIM_SUCCESS, but I couldn't find any. That's because they are typecast from the result of a relational operator. This seemed a bit fragile, so I dug a bit deeper and came up with this proposed cleanup.

Quoted 4 lines

Quoted 1 line
Signed-off-by Petr Tesarik <[email protected]>
Looks good to me, and agree with the minor change in behavior being acceptable. With the macros actually removed,

Uh-oh, I missed that hunk when rebasing, and of course, there's no compiler warning about unused macros. :-(

Quoted 1 line

Acked-by Johannes Weiner <[email protected]>

Thank you. Shall I resubmit once more to include the hunk that removes the macros from mm/internal.h?

Petr T

Vlastimil Babka (SUSE)

maintainer Reviewed-by on v3 1/1

2026-07-13 14:27 UTC #4 lore raw

In reply to Petr Tesarik

Quoted 14 lines · Petr Tesarik

There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.
Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.
There is one small change of behavior when __node_reclaim() was attempted but returned zero. The allocation now skips the zone immediately; before this patch, the zone watermarks were checked first. I believe it was an oversight rather than intention, because the chances that zone watermark is OK after __node_reclaim() did not reclaim any pages are very close to zero.

Quoted 6 lines

Originally, I was looking for occurences of NODE_RECLAIM_SOME and NODE_RECLAIM_SUCCESS, but I couldn't find any. That's because they are typecast from the result of a relational operator. This seemed a bit fragile, so I dug a bit deeper and came up with this proposed cleanup.
Signed-off-by Petr Tesarik <[email protected]>

LGTM, so with the macros removed (indeed as a v4 resubmit)

Reviewed-by Vlastimil Babka (SUSE) <[email protected]>

Zi Yan

reviewer Reviewed-by on v3 1/1

2026-07-13 19:31 UTC #5 lore raw

In reply to Petr Tesarik

Quoted 26 lines · Petr Tesarik

There is only one caller, get_page_from_freelist(), and it does not make any use of the reason for skipping the reclaim, nor does it make any distinction between a full and partially successful reclaim.
Therefore, node_reclaim() can simply return the number of pages that have been reclaimed, same as __node_reclaim(), and the NODE_RECLAIM_xxx macros can be removed.
There is one small change of behavior when __node_reclaim() was attempted but returned zero. The allocation now skips the zone immediately; before this patch, the zone watermarks were checked first. I believe it was an oversight rather than intention, because the chances that zone watermark is OK after __node_reclaim() did not reclaim any pages are very close to zero.
Originally, I was looking for occurences of NODE_RECLAIM_SOME and NODE_RECLAIM_SUCCESS, but I couldn't find any. That's because they are typecast from the result of a relational operator. This seemed a bit fragile, so I dug a bit deeper and came up with this proposed cleanup.
Signed-off-by Petr Tesarik <[email protected]>
--
Changes from v2:
- remove the enum and return the number of pages reclaimed

Quoted 8 lines

Changes from v1:
- use an enum instead of a bool

---
 mm/internal.h   |  9 +++++----
 mm/page_alloc.c | 19 ++++---------------
 mm/vmscan.c     | 16 ++++++++--------
 3 files changed, 17 insertions(+), 27 deletions(-)

With the macro removed, feel free to add

Reviewed-by Zi Yan <[email protected]>

Signature · 2 lines

Best Regards,
Yan, Zi