[PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Petr Tesarik]

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

This leads to the following translation of the old macro
identifiers to the new enum values:

- NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
- NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
- NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
- NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS

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 <ptesarik@suse.com>

--

Changes from v1:
- use an enum instead of a bool
---
 mm/internal.h   | 17 +++++++++--------
 mm/page_alloc.c | 19 ++++---------------
 mm/vmscan.c     | 18 +++++++++---------
 3 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 181e79f1d6a20..89b0ea28051c1 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1373,23 +1373,24 @@ 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
+enum node_reclaim {
+	NODE_RECLAIM_NONE,
+	NODE_RECLAIM_SUCCESS,
+};
 
 #ifdef CONFIG_NUMA
 extern int node_reclaim_mode;
 
-extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
+extern enum node_reclaim 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 enum node_reclaim node_reclaim(struct pglist_data *pgdat,
+					     gfp_t mask, unsigned int order)
 {
-	return NODE_RECLAIM_NOSCAN;
+	return NODE_RECLAIM_NONE;
 }
 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..83a1caac5ac9c 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) == NODE_RECLAIM_NONE)
 				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..d5bd55620ee9a 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)
+enum node_reclaim 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 NODE_RECLAIM_NONE;
 
 	/*
 	 * 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 NODE_RECLAIM_NONE;
 
 	/*
 	 * 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 NODE_RECLAIM_NONE;
 
 	if (test_and_set_bit_lock(PGDAT_RECLAIM_LOCKED, &pgdat->flags))
-		return NODE_RECLAIM_NOSCAN;
+		return NODE_RECLAIM_NONE;
 
-	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 NODE_RECLAIM_SUCCESS;
 }
 
 #else
-- 
2.54.0

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Brendan Jackman]

On Fri Jun 12, 2026 at 8:50 AM UTC, Petr Tesarik wrote:
> Change node_reclaim() to return an enum indicating whether any
> pages have been reclaimed, because that's all the information
> needed by the only caller, get_page_from_freelist().
>
> This leads to the following translation of the old macro
> identifiers to the new enum values:
>
> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
>
> 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 <ptesarik@suse.com>
>
> --
>
> Changes from v1:
> - use an enum instead of a bool
> ---
>  mm/internal.h   | 17 +++++++++--------
>  mm/page_alloc.c | 19 ++++---------------
>  mm/vmscan.c     | 18 +++++++++---------
>  3 files changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 181e79f1d6a20..89b0ea28051c1 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1373,23 +1373,24 @@ 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
> +enum node_reclaim {
> +	NODE_RECLAIM_NONE,
> +	NODE_RECLAIM_SUCCESS,
> +};
>  
>  #ifdef CONFIG_NUMA
>  extern int node_reclaim_mode;
>  
> -extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
> +extern enum node_reclaim 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 enum node_reclaim node_reclaim(struct pglist_data *pgdat,
> +					     gfp_t mask, unsigned int order)
>  {
> -	return NODE_RECLAIM_NOSCAN;
> +	return NODE_RECLAIM_NONE;
>  }
>  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..83a1caac5ac9c 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) == NODE_RECLAIM_NONE)
>  				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..d5bd55620ee9a 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)
> +enum node_reclaim 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 NODE_RECLAIM_NONE;
>  
>  	/*
>  	 * 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 NODE_RECLAIM_NONE;
>  
>  	/*
>  	 * 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 NODE_RECLAIM_NONE;
>  
>  	if (test_and_set_bit_lock(PGDAT_RECLAIM_LOCKED, &pgdat->flags))
> -		return NODE_RECLAIM_NOSCAN;
> +		return NODE_RECLAIM_NONE;
>  
> -	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 NODE_RECLAIM_SUCCESS;

Should that be returning NODE_RECLAIM_NONE when !ret?

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Petr Tesarik]

On Fri, 12 Jun 2026 10:10:20 +0000
"Brendan Jackman" <brendan.jackman@linux.dev> wrote:

> On Fri Jun 12, 2026 at 8:50 AM UTC, Petr Tesarik wrote:
> > Change node_reclaim() to return an enum indicating whether any
> > pages have been reclaimed, because that's all the information
> > needed by the only caller, get_page_from_freelist().
> >
> > This leads to the following translation of the old macro
> > identifiers to the new enum values:
> >
> > - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
> > - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
> > - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
> > - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
> >
> > 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 <ptesarik@suse.com>
> >
> > --
> >
> > Changes from v1:
> > - use an enum instead of a bool
> > ---
> >  mm/internal.h   | 17 +++++++++--------
> >  mm/page_alloc.c | 19 ++++---------------
> >  mm/vmscan.c     | 18 +++++++++---------
> >  3 files changed, 22 insertions(+), 32 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 181e79f1d6a20..89b0ea28051c1 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -1373,23 +1373,24 @@ 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
> > +enum node_reclaim {
> > +	NODE_RECLAIM_NONE,
> > +	NODE_RECLAIM_SUCCESS,
> > +};
> >  
> >  #ifdef CONFIG_NUMA
> >  extern int node_reclaim_mode;
> >  
> > -extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
> > +extern enum node_reclaim 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 enum node_reclaim node_reclaim(struct pglist_data *pgdat,
> > +					     gfp_t mask, unsigned int order)
> >  {
> > -	return NODE_RECLAIM_NOSCAN;
> > +	return NODE_RECLAIM_NONE;
> >  }
> >  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..83a1caac5ac9c 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) == NODE_RECLAIM_NONE)
> >  				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..d5bd55620ee9a 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)
> > +enum node_reclaim 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 NODE_RECLAIM_NONE;
> >  
> >  	/*
> >  	 * 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 NODE_RECLAIM_NONE;
> >  
> >  	/*
> >  	 * 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 NODE_RECLAIM_NONE;
> >  
> >  	if (test_and_set_bit_lock(PGDAT_RECLAIM_LOCKED, &pgdat->flags))
> > -		return NODE_RECLAIM_NOSCAN;
> > +		return NODE_RECLAIM_NONE;
> >  
> > -	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 NODE_RECLAIM_SUCCESS;  
> 
> Should that be returning NODE_RECLAIM_NONE when !ret?

No. That's the thing. Before my patch, the return value here was either
NODE_RECLAIM_SUCCESS (when at least nr_pages were reclaimed), or
NODE_RECLAIM_SOME (if less than nr_pages were reclaimed), but the caller
makes no distinction, handling both cases in the default label of a
switch statement.

Petr T

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Brendan Jackman]

On Fri Jun 12, 2026 at 11:31 AM UTC, Petr Tesarik wrote:
>> >
>> > - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
>> > - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
>> > - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
>> > - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS

>> > --- a/mm/internal.h
>> > +++ b/mm/internal.h
>> > @@ -1373,23 +1373,24 @@ 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
>> > +enum node_reclaim {
>> > +	NODE_RECLAIM_NONE,
>> > +	NODE_RECLAIM_SUCCESS,
>> > +};

>> > -	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 NODE_RECLAIM_SUCCESS;  
>> 
>> Should that be returning NODE_RECLAIM_NONE when !ret?
>
> No. That's the thing. Before my patch, the return value here was either
> NODE_RECLAIM_SUCCESS (when at least nr_pages were reclaimed), or
> NODE_RECLAIM_SOME (if less than nr_pages were reclaimed), but the caller
> makes no distinction, handling both cases in the default label of a
> switch statement.

Agh! Sorry. It's good that you are cleaning this up :D

So in that case my question is: is it intended that we call
zone_watermark_ok() in the case that we reclaimed 0 pages?

If not, maybe what we want here is:

- One patch to stop doing that.

- Another patch to switch over to the enum.

And then it _would_ return NODE_RECLAIM_NONE when !ret?

Have a weird feeling I'm still being stupid here, let's see...

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[David Hildenbrand (Arm)]

On 6/12/26 10:50, Petr Tesarik wrote:
> Change node_reclaim() to return an enum indicating whether any
> pages have been reclaimed, because that's all the information
> needed by the only caller, get_page_from_freelist().
> 
> This leads to the following translation of the old macro
> identifiers to the new enum values:
> 
> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS

Why not simply return the number of reclaimed pages (0 vs > 0)? I agree that a
bool is not good.

Or if that is not good enough (for some reason) return 0 (success) vs. -ENOENT?

-- 
Cheers,

David

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Zi Yan]

On 12 Jun 2026, at 4:50, Petr Tesarik wrote:

> Change node_reclaim() to return an enum indicating whether any
> pages have been reclaimed, because that's all the information
> needed by the only caller, get_page_from_freelist().
>
> This leads to the following translation of the old macro
> identifiers to the new enum values:
>
> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
>
> 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 <ptesarik@suse.com>
>
> --
>
> Changes from v1:
> - use an enum instead of a bool
> ---
>  mm/internal.h   | 17 +++++++++--------
>  mm/page_alloc.c | 19 ++++---------------
>  mm/vmscan.c     | 18 +++++++++---------
>  3 files changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 181e79f1d6a20..89b0ea28051c1 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1373,23 +1373,24 @@ 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
> +enum node_reclaim {
> +	NODE_RECLAIM_NONE,
> +	NODE_RECLAIM_SUCCESS,
> +};
>
>  #ifdef CONFIG_NUMA
>  extern int node_reclaim_mode;
>
> -extern int node_reclaim(struct pglist_data *, gfp_t, unsigned int);
> +extern enum node_reclaim 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 enum node_reclaim node_reclaim(struct pglist_data *pgdat,
> +					     gfp_t mask, unsigned int order)
>  {
> -	return NODE_RECLAIM_NOSCAN;
> +	return NODE_RECLAIM_NONE;
>  }
>  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..83a1caac5ac9c 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) == NODE_RECLAIM_NONE)
>  				continue;

NODE_RECLAIM_NOSCAN and NODE_RECLAIM_FULL both continue here and they appear
nowhere else, so they can be combined into NODE_RECLAIM_NONE like you did.

<snip>

>
> -	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)

I was wondering why ret is changed to nr_reclaimed >= nr_pages,
which seems to change the original semantics, until I see the “>= nr_pages”
at the end of __node_reclaim().

>  		count_vm_event(PGSCAN_ZONE_RECLAIM_SUCCESS);
>  	else
>  		count_vm_event(PGSCAN_ZONE_RECLAIM_FAILED);
>
> -	return ret;
> +	return NODE_RECLAIM_SUCCESS;
>  }
>
>  #else

The patch keeps the original semantics, so
Reviewed-by: Zi Yan <ziy@nvidia.com>

In terms of the question raised by Brendan about the return value by
__node_reclaim(), I will follow the discussion on the other thread.

Best Regards,
Yan, Zi

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Zi Yan]

On 12 Jun 2026, at 10:28, Brendan Jackman wrote:

> On Fri Jun 12, 2026 at 11:31 AM UTC, Petr Tesarik wrote:
>>>>
>>>> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
>>>> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
>>>> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
>>>> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
>
>>>> --- a/mm/internal.h
>>>> +++ b/mm/internal.h
>>>> @@ -1373,23 +1373,24 @@ 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
>>>> +enum node_reclaim {
>>>> +	NODE_RECLAIM_NONE,
>>>> +	NODE_RECLAIM_SUCCESS,
>>>> +};
>
>>>> -	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 NODE_RECLAIM_SUCCESS;
>>>
>>> Should that be returning NODE_RECLAIM_NONE when !ret?
>>
>> No. That's the thing. Before my patch, the return value here was either
>> NODE_RECLAIM_SUCCESS (when at least nr_pages were reclaimed), or
>> NODE_RECLAIM_SOME (if less than nr_pages were reclaimed), but the caller
>> makes no distinction, handling both cases in the default label of a
>> switch statement.
>
> Agh! Sorry. It's good that you are cleaning this up :D
>
> So in that case my question is: is it intended that we call
> zone_watermark_ok() in the case that we reclaimed 0 pages?

It seems to me that zone_watermark_ok() here is trying to
confirm the number of reclaimed pages is enough for the
allocation, although the return value of __node_reclaim()
might be able to tell the same thing like you suggested.
But nr_reclaimed >= nr_pages might not be equivalent
to zone_watermark_ok().

>
> If not, maybe what we want here is:
>
> - One patch to stop doing that.
>
> - Another patch to switch over to the enum.
>
> And then it _would_ return NODE_RECLAIM_NONE when !ret?
>
> Have a weird feeling I'm still being stupid here, let's see...


Best Regards,
Yan, Zi

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Brendan Jackman]

On Fri Jun 12, 2026 at 3:01 PM UTC, Zi Yan wrote:
> On 12 Jun 2026, at 10:28, Brendan Jackman wrote:
>
>> On Fri Jun 12, 2026 at 11:31 AM UTC, Petr Tesarik wrote:
>>>>>
>>>>> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
>>>>> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
>>>>> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
>>>>> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
>>
>>>>> --- a/mm/internal.h
>>>>> +++ b/mm/internal.h
>>>>> @@ -1373,23 +1373,24 @@ 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
>>>>> +enum node_reclaim {
>>>>> +	NODE_RECLAIM_NONE,
>>>>> +	NODE_RECLAIM_SUCCESS,
>>>>> +};
>>
>>>>> -	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 NODE_RECLAIM_SUCCESS;
>>>>
>>>> Should that be returning NODE_RECLAIM_NONE when !ret?
>>>
>>> No. That's the thing. Before my patch, the return value here was either
>>> NODE_RECLAIM_SUCCESS (when at least nr_pages were reclaimed), or
>>> NODE_RECLAIM_SOME (if less than nr_pages were reclaimed), but the caller
>>> makes no distinction, handling both cases in the default label of a
>>> switch statement.
>>
>> Agh! Sorry. It's good that you are cleaning this up :D
>>
>> So in that case my question is: is it intended that we call
>> zone_watermark_ok() in the case that we reclaimed 0 pages?
>
> It seems to me that zone_watermark_ok() here is trying to
> confirm the number of reclaimed pages is enough for the
> allocation, although the return value of __node_reclaim()
> might be able to tell the same thing like you suggested.
> But nr_reclaimed >= nr_pages might not be equivalent
> to zone_watermark_ok().

Yeah but in the more specific case of nr_reclaimed == 0, would it make
sense to assume !zone_watermark_ok()? It doesn't seem that important in
terms of behaviour but I think it would be a clarity win.

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Petr Tesarik]

On Fri, 12 Jun 2026 16:57:08 +0200
"David Hildenbrand (Arm)" <david@kernel.org> wrote:

> On 6/12/26 10:50, Petr Tesarik wrote:
> > Change node_reclaim() to return an enum indicating whether any
> > pages have been reclaimed, because that's all the information
> > needed by the only caller, get_page_from_freelist().
> > 
> > This leads to the following translation of the old macro
> > identifiers to the new enum values:
> > 
> > - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
> > - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
> > - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
> > - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS  
> 
> Why not simply return the number of reclaimed pages (0 vs > 0)? I agree that a
> bool is not good.
> 
> Or if that is not good enough (for some reason) return 0 (success) vs. -ENOENT?

I don't have a strong opinion, except I hate the current code. ;-)

If we can agree that get_page_from_freelist() need not recheck with
zone_watermark_ok() after __node_reclaim() returns zero, then I believe
the cleanest option is to return the number of pages (zero if reclaim
was not even attempted).

Petr T

Re: [PATCH v2 1/1] mm: reduce NODE_RECLAIM_xxx and change to enum

[Zi Yan]

On 12 Jun 2026, at 11:10, Brendan Jackman wrote:

> On Fri Jun 12, 2026 at 3:01 PM UTC, Zi Yan wrote:
>> On 12 Jun 2026, at 10:28, Brendan Jackman wrote:
>>
>>> On Fri Jun 12, 2026 at 11:31 AM UTC, Petr Tesarik wrote:
>>>>>>
>>>>>> - NODE_RECLAIM_NOSCAN  -> NODE_RECLAIM_NONE
>>>>>> - NODE_RECLAIM_FULL    -> NODE_RECLAIM_NONE
>>>>>> - NODE_RECLAIM_SOME    -> NODE_RECLAIM_SUCCESS
>>>>>> - NODE_RECLAIM_SUCCESS -> NODE_RECLAIM_SUCCESS
>>>
>>>>>> --- a/mm/internal.h
>>>>>> +++ b/mm/internal.h
>>>>>> @@ -1373,23 +1373,24 @@ 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
>>>>>> +enum node_reclaim {
>>>>>> +	NODE_RECLAIM_NONE,
>>>>>> +	NODE_RECLAIM_SUCCESS,
>>>>>> +};
>>>
>>>>>> -	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 NODE_RECLAIM_SUCCESS;
>>>>>
>>>>> Should that be returning NODE_RECLAIM_NONE when !ret?
>>>>
>>>> No. That's the thing. Before my patch, the return value here was either
>>>> NODE_RECLAIM_SUCCESS (when at least nr_pages were reclaimed), or
>>>> NODE_RECLAIM_SOME (if less than nr_pages were reclaimed), but the caller
>>>> makes no distinction, handling both cases in the default label of a
>>>> switch statement.
>>>
>>> Agh! Sorry. It's good that you are cleaning this up :D
>>>
>>> So in that case my question is: is it intended that we call
>>> zone_watermark_ok() in the case that we reclaimed 0 pages?
>>
>> It seems to me that zone_watermark_ok() here is trying to
>> confirm the number of reclaimed pages is enough for the
>> allocation, although the return value of __node_reclaim()
>> might be able to tell the same thing like you suggested.
>> But nr_reclaimed >= nr_pages might not be equivalent
>> to zone_watermark_ok().
>
> Yeah but in the more specific case of nr_reclaimed == 0, would it make
> sense to assume !zone_watermark_ok()? It doesn't seem that important in
> terms of behaviour but I think it would be a clarity win.

I agree. If nr_reclaimed == 0, unless some magic happens in parallel,
it is a waste to call zone_watermark_ok(). Now what you said
is clear and makes sense to me.

Best Regards,
Yan, Zi