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

[Petr Tesarik <ptesarik@suse.com>]

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.