[PATCH 1/1] mm: remove NODE_RECLAIM_xxx macros

[PATCH 1/1] mm: remove NODE_RECLAIM_xxx macros

[Petr Tesarik]

Change node_reclaim() to return a bool indicating whether any
pages have been reclaimed, because that's the only information
needed by the only caller, get_page_from_freelist().

Originally, I wanted to convert the preprocessor macros to an
enum, but I couldn't find any explicit use of NODE_RECLAIM_SOME
and NODE_RECLAIM_SUCCESS. That's because they are typecast from
a bool. This seemed a bit fragile, so I dug a bit deeper and
came up with this proposed cleanup.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 mm/internal.h   | 12 ++++--------
 mm/page_alloc.c | 19 ++++---------------
 mm/vmscan.c     | 18 +++++++++---------
 3 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 181e79f1d6a20..6e09157a97692 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1373,23 +1373,19 @@ static inline void mminit_verify_zonelist(void)
 }
 #endif /* CONFIG_DEBUG_MEMORY_INIT */
 
-#define NODE_RECLAIM_NOSCAN	-2
-#define NODE_RECLAIM_FULL	-1
-#define NODE_RECLAIM_SOME	0
-#define NODE_RECLAIM_SUCCESS	1
-
 #ifdef CONFIG_NUMA
 extern int node_reclaim_mode;
 
-extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
+extern bool 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,
+static inline bool node_reclaim(struct pglist_data *pgdat, gfp_t mask,
 				unsigned int order)
 {
-	return NODE_RECLAIM_NOSCAN;
+	return false;
 }
 static inline int find_next_best_node(int node, nodemask_t *used_node_mask)
 {
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f7db8f049bd23..9db7eaa7091c9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3899,8 +3899,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;
 
@@ -3921,22 +3919,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:
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3f3ff25e561ac..64f6b649eeac1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -7786,9 +7786,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)
+bool node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
 {
-	int ret;
+	unsigned long nr_reclaimed;
 	/* Minimum pages needed in order to stay on node */
 	const unsigned long nr_pages = 1 << order;
 	struct scan_control sc = {
@@ -7815,13 +7815,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 false;
 
 	/*
 	 * 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 false;
 
 	/*
 	 * Only run node reclaim on the local node or on nodes that do not
@@ -7830,20 +7830,20 @@ 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 false;
 
 	if (test_and_set_bit_lock(PGDAT_RECLAIM_LOCKED, &pgdat->flags))
-		return NODE_RECLAIM_NOSCAN;
+		return false;
 
-	ret = __node_reclaim(pgdat, gfp_mask, nr_pages, &sc) >= nr_pages;
+	nr_reclaimed = __node_reclaim(pgdat, gfp_mask, nr_pages, &sc);
 	clear_bit_unlock(PGDAT_RECLAIM_LOCKED, &pgdat->flags);
 
-	if (ret)
+	if (nr_reclaimed >= nr_pages)
 		count_vm_event(PGSCAN_ZONE_RECLAIM_SUCCESS);
 	else
 		count_vm_event(PGSCAN_ZONE_RECLAIM_FAILED);
 
-	return ret;
+	return true;
 }
 
 #else
-- 
2.54.0

Re: [PATCH 1/1] mm: remove NODE_RECLAIM_xxx macros

[Brendan Jackman]

On Thu Jun 11, 2026 at 12:45 PM UTC, Petr Tesarik wrote:
> Change node_reclaim() to return a bool indicating whether any
> pages have been reclaimed, because that's the only information
> needed by the only caller, get_page_from_freelist().
>
> Originally, I wanted to convert the preprocessor macros to an
> enum, but I couldn't find any explicit use of NODE_RECLAIM_SOME
> and NODE_RECLAIM_SUCCESS. That's because they are typecast from
> a bool. 

I'm slightly confused by the "typecast from a bool" thing - nr_reclaim
returns nr_reclaimed and then node_reclaim()'s caller gets that value
directly - is that what you're referring to?

> This seemed a bit fragile, 

.. Which, yeah, is awkward, thanks for fixing it.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3f3ff25e561ac..64f6b649eeac1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -7786,9 +7786,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)
> +bool node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)

I really dislike returning bools with no obvious polarity. Personally I
would keep NODE_RECLAIM_*, (optionally convert to an enum), move the
comments to the definition of the NODE_RECLAIM_ thingies instead of
get_page_from_freelist(), and fix node_reclaim() to return
NODE_RECLAIM_{SUCCESS,SOME} expliticly.

I realise this philosophy is not favourable to concision though, I won't
die on that hill but could we at least get a comment on node_reclaim()'s
defintion...


> -			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;
> -			}
>  		}

... or keep the intermediate variable and do:

	bool reclaimed_some = node_reclaim(...):

Since then the variable name at least tells you what you're looking at
without needing to jump into the function implementation.

Re: [PATCH 1/1] mm: remove NODE_RECLAIM_xxx macros

[Petr Tesarik]

On Thu, 11 Jun 2026 13:32:04 +0000
"Brendan Jackman" <brendan.jackman@linux.dev> wrote:

> On Thu Jun 11, 2026 at 12:45 PM UTC, Petr Tesarik wrote:
> > Change node_reclaim() to return a bool indicating whether any
> > pages have been reclaimed, because that's the only information
> > needed by the only caller, get_page_from_freelist().
> >
> > Originally, I wanted to convert the preprocessor macros to an
> > enum, but I couldn't find any explicit use of NODE_RECLAIM_SOME
> > and NODE_RECLAIM_SUCCESS. That's because they are typecast from
> > a bool.   
> 
> I'm slightly confused by the "typecast from a bool" thing - nr_reclaim
> returns nr_reclaimed and then node_reclaim()'s caller gets that value
> directly - is that what you're referring to?

My fault. Without my patch, the return value is calculated as follows:

ret = __node_reclaim(pgdat, gfp_mask, nr_pages, &sc) >= nr_pages

The result type of a relational operator is an int, not a bool; and
since I'm introducing a bool type elsewhere, no wonder you are confused.

I can improve the commit message in a v2 if necessay.

> > This seemed a bit fragile,   
> 
> .. Which, yeah, is awkward, thanks for fixing it.

No problem.

Petr T

> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 3f3ff25e561ac..64f6b649eeac1 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -7786,9 +7786,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) +bool node_reclaim(struct pglist_data *pgdat,
> > gfp_t gfp_mask, unsigned int order)  
> 
> I really dislike returning bools with no obvious polarity. Personally
> I would keep NODE_RECLAIM_*, (optionally convert to an enum), move the
> comments to the definition of the NODE_RECLAIM_ thingies instead of
> get_page_from_freelist(), and fix node_reclaim() to return
> NODE_RECLAIM_{SUCCESS,SOME} expliticly.
> 
> I realise this philosophy is not favourable to concision though, I
> won't die on that hill but could we at least get a comment on
> node_reclaim()'s defintion...
> 
> 
> > -			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;
> > -			}
> >  		}  
> 
> ... or keep the intermediate variable and do:
> 
> 	bool reclaimed_some = node_reclaim(...):
> 
> Since then the variable name at least tells you what you're looking at
> without needing to jump into the function implementation.