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

["Brendan Jackman" <brendan.jackman@linux.dev>]

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.