Re: [PATCH 1/2] mm: migrate: restore the nmask after successfully allocating on the target node

[SeongJae Park <sj@kernel.org>]

+ Jagdish, since seems the behavior that this patch tries to change is
apparently made by Jagdish's commit 320080272892 ("mm/demotion: demote pages
according to allocation fallback order").

On Wed, 28 May 2025 19:10:37 +0800 wangchuanguo <wangchuanguo@inspur.com> wrote:

> If memory is successfully allocated on the target node and the
> function directly returns without value restore for nmask,
> non-first migration operations in migrate_pages() by again label
> may ignore the nmask settings,

Nice finding!

> thereby allowing new memory
> allocations for migration on any node.

But, isn't the consequence of this behavior is the opposite?  That is, I think
this behavior restricts to use only the specified node (mtc->nid) in the case,
ignoring more allowed fallback nodes (mtc->nmask)?

Anyway, to me, this seems not an intended behavior but a bug.  Cc-ing Jagdish,
who authored the commit 320080272892 ("mm/demotion: demote pages according to
allocation fallback order"), which apparently made this behavior initially,
though, since I may misreading the original author's intention.

> 
> Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>
> ---
>  mm/vmscan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f8dfd2864bbf..e13f17244279 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1035,11 +1035,11 @@ struct folio *alloc_migrate_folio(struct folio *src, unsigned long private)
>  	mtc->nmask = NULL;
>  	mtc->gfp_mask |= __GFP_THISNODE;
>  	dst = alloc_migration_target(src, (unsigned long)mtc);
> +	mtc->nmask = allowed_mask;
>  	if (dst)
>  		return dst;

Restoring ->nmask looks right behavior to me.  But, if so, shouldn't we also
restore ->gfp_mask?

>  
>  	mtc->gfp_mask &= ~__GFP_THISNODE;
> -	mtc->nmask = allowed_mask;
>  
>  	return alloc_migration_target(src, (unsigned long)mtc);
>  }
> -- 
> 2.39.3


Thanks,
SJ