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

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

[Simon Wang (王传国)]

> On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory allocations for migration
> > on any node.
> 
> I have no opinion on whether this is the right thing to do or not, but if it is
>

I don't think so. When memory allocation fails on the target node, there is already a recovery operation for the nmask value below. Therefore, the nmask value should only be restored when memory allocation is successfully completed on the target node.

> > +++ b/mm/vmscan.c
> > @@ -1026,8 +1026,10 @@ 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);
> > -	if (dst)
> > +	if (dst) {
> > +		mtc->nmask = allowed_mask;
> >  		return dst;
> > +	}
> >
> >  	mtc->gfp_mask &= ~__GFP_THISNODE;
> >  	mtc->nmask = allowed_mask;
> 
> this should be:
> 
> 	dst = alloc_migration_target(src, (unsigned long)mtc);
> +	mtc->nmask = allowed_mask;
> 	if (dst)
> 		return dst;
> 
> 	mtc->gfp_mask &= ~__GFP_THISNODE;
> -	mtc->nmask = allowed_mask;

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

[Oscar Salvador]

On Wed, Mar 26, 2025 at 05:54:35AM +0000, Simon Wang (王传国) wrote:
> 
> > On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory allocations for migration
> > > on any node.
> > 
> > I have no opinion on whether this is the right thing to do or not, but if it is
> >
> 
> I don't think so. When memory allocation fails on the target node, there is already a recovery operation for the nmask value below. Therefore, the nmask value should only be restored when memory allocation is successfully completed on the target node.

But that is not what the code is doing, is it? With the changes applied I mean.
You are restoring mtc->nmask in case you managed to allocate for __GFP_THISNODE
and after you clear the flag, so we might as well do it just once at the beginning
after calling alloc_migration_target for the first time.


-- 
Oscar Salvador
SUSE Labs

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

[Simon Wang (王传国)]

> 
> On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory allocations for migration
> > on any node.
> >
> > Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>
> 
> Unless I am missing something this looks reasonable, but I whonder why
> nobody noticed it before.
> It is a path that should be pretty exercised.
> 
> 
> --
> Oscar Salvador
> SUSE Labs
I tested it, and even when the nmask is set to all zeros, memory allocation still occurs on other nodes. 
I suspect this is because the bug ​expands the range of eligible nodes for memory allocation instead of causing allocation failures, which is why it has gone unnoticed.

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

[Simon Wang (王传国)]

> >
> > > On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory
> > > > allocations for migration on any node.
> > >
> > > I have no opinion on whether this is the right thing to do or not,
> > > but if it is
> > >
> >
> > I don't think so. When memory allocation fails on the target node, there is
> already a recovery operation for the nmask value below. Therefore, the nmask
> value should only be restored when memory allocation is successfully
> completed on the target node.
> 
> But that is not what the code is doing, is it? With the changes applied I mean.
> You are restoring mtc->nmask in case you managed to allocate for
> __GFP_THISNODE and after you clear the flag, so we might as well do it just
> once at the beginning after calling alloc_migration_target for the first time.
> 
> 
> --
> Oscar Salvador
> SUSE Labs
Yes, you're right. My apologies—I overlooked a line of code earlier.

[PATCH] mm: migrate: restore the nmask after successfully allocating on the target node

[wangchuanguo]

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, thereby allowing new memory
allocations for migration on any node.

Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>
---
 mm/vmscan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b620d74b0f66..9467b2acef28 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1026,8 +1026,10 @@ 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);
-	if (dst)
+	if (dst) {
+		mtc->nmask = allowed_mask;
 		return dst;
+	}
 
 	mtc->gfp_mask &= ~__GFP_THISNODE;
 	mtc->nmask = allowed_mask;
-- 
2.39.3

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

[Matthew Wilcox]

On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory
> allocations for migration on any node.

I have no opinion on whether this is the right thing to do or not, but
if it is:

> +++ b/mm/vmscan.c
> @@ -1026,8 +1026,10 @@ 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);
> -	if (dst)
> +	if (dst) {
> +		mtc->nmask = allowed_mask;
>  		return dst;
> +	}
>  
>  	mtc->gfp_mask &= ~__GFP_THISNODE;
>  	mtc->nmask = allowed_mask;

this should be:

	dst = alloc_migration_target(src, (unsigned long)mtc);
+	mtc->nmask = allowed_mask;
	if (dst)
		return dst;

	mtc->gfp_mask &= ~__GFP_THISNODE;
-	mtc->nmask = allowed_mask;

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

[Oscar Salvador]

On Wed, Mar 26, 2025 at 11:12:18AM +0800, wangchuanguo 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, thereby allowing new memory
> allocations for migration on any node.
> 
> Signed-off-by: wangchuanguo <wangchuanguo@inspur.com>

Unless I am missing something this looks reasonable, but I whonder why nobody
noticed it before.
It is a path that should be pretty exercised.


-- 
Oscar Salvador
SUSE Labs