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

[Zi Yan <ziy@nvidia.com>]

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