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

[Petr Tesarik <ptesarik@suse.com>]

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