Demotion cleanup and fixes

[PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Alexandre Ghiti 3 weeks, 6 days ago

GFP_TRANSHUGE sets __GFP_DIRECT_RECLAIM so we must clear GFP_RECLAIM
after, not before.

Reported-by: Bing Jiao <bingjiao@google.com>
Closes: https://lore.kernel.org/linux-mm/aXlKOxGGI9zne8sl@google.com/
Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 2c3d489ecf51..ee533a4d38db 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2190,12 +2190,12 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
 	}
 
 	if (folio_test_large(src)) {
+		gfp_mask |= GFP_TRANSHUGE;
 		/*
 		 * clear __GFP_RECLAIM to make the migration callback
 		 * consistent with regular THP allocations.
 		 */
 		gfp_mask &= ~__GFP_RECLAIM;
-		gfp_mask |= GFP_TRANSHUGE;
 		order = folio_order(src);
 	}
 	zidx = folio_zonenum(src);
-- 
2.53.0

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Johannes Weiner 3 weeks, 6 days ago

On Wed, Mar 11, 2026 at 12:02:42PM +0100, Alexandre Ghiti wrote:
> GFP_TRANSHUGE sets __GFP_DIRECT_RECLAIM so we must clear GFP_RECLAIM
> after, not before.
> 
> Reported-by: Bing Jiao <bingjiao@google.com>
> Closes: https://lore.kernel.org/linux-mm/aXlKOxGGI9zne8sl@google.com/
> Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> ---
>  mm/migrate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 2c3d489ecf51..ee533a4d38db 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2190,12 +2190,12 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
>  	}
>  
>  	if (folio_test_large(src)) {
> +		gfp_mask |= GFP_TRANSHUGE;
>  		/*
>  		 * clear __GFP_RECLAIM to make the migration callback
>  		 * consistent with regular THP allocations.
>  		 */
>  		gfp_mask &= ~__GFP_RECLAIM;
> -		gfp_mask |= GFP_TRANSHUGE;

I don't think this is right.

The Fixes: did it this way to disable kswapd for THP allocations,
while still allowing the customary direct reclaim. Maybe a better
comment would have been: /* GFP_TRANSHUGE has its own reclaim policy */

After your fix, direct reclaim isn't allowed either, which makes the
request unnecessarily wimpy.

The Closes: refers to reclaim that should be avoided during demotion.
But if this path is taken during demotion it will already not recurse
into direct reclaim due to PF_MEMALLOC.

So I don't see a bug in the existing code. But maybe the comment could
be clearer.

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Alexandre Ghiti 3 weeks, 4 days ago

Hi Johannes,

On 3/11/26 18:54, Johannes Weiner wrote:
> On Wed, Mar 11, 2026 at 12:02:42PM +0100, Alexandre Ghiti wrote:
>> GFP_TRANSHUGE sets __GFP_DIRECT_RECLAIM so we must clear GFP_RECLAIM
>> after, not before.
>>
>> Reported-by: Bing Jiao <bingjiao@google.com>
>> Closes: https://lore.kernel.org/linux-mm/aXlKOxGGI9zne8sl@google.com/
>> Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
>> ---
>>   mm/migrate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 2c3d489ecf51..ee533a4d38db 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -2190,12 +2190,12 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
>>   	}
>>   
>>   	if (folio_test_large(src)) {
>> +		gfp_mask |= GFP_TRANSHUGE;
>>   		/*
>>   		 * clear __GFP_RECLAIM to make the migration callback
>>   		 * consistent with regular THP allocations.
>>   		 */
>>   		gfp_mask &= ~__GFP_RECLAIM;
>> -		gfp_mask |= GFP_TRANSHUGE;
> I don't think this is right.
>
> The Fixes: did it this way to disable kswapd for THP allocations,
> while still allowing the customary direct reclaim. Maybe a better
> comment would have been: /* GFP_TRANSHUGE has its own reclaim policy */
>
> After your fix, direct reclaim isn't allowed either, which makes the
> request unnecessarily wimpy.
>
> The Closes: refers to reclaim that should be avoided during demotion.
> But if this path is taken during demotion it will already not recurse
> into direct reclaim due to PF_MEMALLOC.
>
> So I don't see a bug in the existing code. But maybe the comment could
> be clearer.


Makes sense, I had not understood the comment indeed. I'll drop this fix 
in the next version then.

Thanks,

Alex

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Gregory Price 3 weeks, 5 days ago

On Wed, Mar 11, 2026 at 01:54:50PM -0400, Johannes Weiner wrote:
> On Wed, Mar 11, 2026 at 12:02:42PM +0100, Alexandre Ghiti wrote:
> > GFP_TRANSHUGE sets __GFP_DIRECT_RECLAIM so we must clear GFP_RECLAIM
> > after, not before.
> > 
> > Reported-by: Bing Jiao <bingjiao@google.com>
> > Closes: https://lore.kernel.org/linux-mm/aXlKOxGGI9zne8sl@google.com/
> > Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> > ---
> >  mm/migrate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 2c3d489ecf51..ee533a4d38db 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2190,12 +2190,12 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
> >  	}
> >  
> >  	if (folio_test_large(src)) {
> > +		gfp_mask |= GFP_TRANSHUGE;
> >  		/*
> >  		 * clear __GFP_RECLAIM to make the migration callback
> >  		 * consistent with regular THP allocations.
> >  		 */
> >  		gfp_mask &= ~__GFP_RECLAIM;
> > -		gfp_mask |= GFP_TRANSHUGE;
> 
> I don't think this is right.
> 
> The Fixes: did it this way to disable kswapd for THP allocations,
> while still allowing the customary direct reclaim. Maybe a better
> comment would have been: /* GFP_TRANSHUGE has its own reclaim policy */
> 

The bigger issue how many times we see this particular flag getting
masked and apparently added back in at multiple layers. We saw two
or three paths (some unreachable) that can twiddle RECLAIM flags in
the stack for demotion (which is in reclaim already, so do the flags
matter?).

It makes it difficult to reason about what the GFP flags actually
are at any given point.

But yeah I wasn't sure to make of this code, it could be as you
suggested just a bad comment.

~Gregory

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Andrew Morton 3 weeks, 6 days ago

On Wed, 11 Mar 2026 12:02:42 +0100 Alexandre Ghiti <alex@ghiti.fr> wrote:

> Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
> Cc: stable@vger.kernel.org

Please let's have the cc:stable fixes separated out from the cleanups,
and prepared against current -linus mainline.

Also, when proposing backportable fixes please ensure that the
changelogs carefully describe the userspace-visible runtime effects of
the bug.

Thanks.

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by Alexandre Ghiti 3 weeks, 4 days ago

Hi Andrew,

On 3/11/26 18:06, Andrew Morton wrote:
> On Wed, 11 Mar 2026 12:02:42 +0100 Alexandre Ghiti <alex@ghiti.fr> wrote:
>
>> Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
>> Cc: stable@vger.kernel.org
> Please let's have the cc:stable fixes separated out from the cleanups,
> and prepared against current -linus mainline.


I'll split the series in the next version.


>
> Also, when proposing backportable fixes please ensure that the
> changelogs carefully describe the userspace-visible runtime effects of
> the bug.


I was unaware of that requirement, I'll do.

Thanks,

Alex


>
> Thanks.

Re: [PATCH 3/4] mm: Fix demotion gfp by clearing GFP_RECLAIM after setting GFP_TRANSHUGE

Posted by David Hildenbrand (Arm) 3 weeks, 5 days ago

On 3/11/26 18:06, Andrew Morton wrote:
> On Wed, 11 Mar 2026 12:02:42 +0100 Alexandre Ghiti <alex@ghiti.fr> wrote:
> 
>> Fixes: 9933a0c8a539 ("mm/migrate: clear __GFP_RECLAIM to make the migration callback consistent with regular THP allocations")
>> Cc: stable@vger.kernel.org
> 
> Please let's have the cc:stable fixes separated out from the cleanups,
> and prepared against current -linus mainline.
> 
> Also, when proposing backportable fixes please ensure that the
> changelogs carefully describe the userspace-visible runtime effects of
> the bug.

Also, please move fixes to the very beginning of the patch set, or
better, send them independently.

-- 
Cheers,

David