Re: [Devel] [PATCH hci-8.1 v2 1/2] mm/memcontrol: allocate numa_migrate targets of the source folio order

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 10/28/26 15:22, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm: memcg numa_migrate uses no-reclaim THISNODE gfp for all large folio orders
> --
> 
> commit dc423622ee7bf287039eede1edeca15b40cb3108
> Author: Konstantin Khorenko <khorenko@virtuozzo.com>
> 
> mm/memcontrol: allocate numa_migrate targets of the source folio order
> 
> This patch makes memcg_numa_migrate_new_folio() allocate the destination
> folio with the same order as the source large folio, instead of always
> using HPAGE_PMD_ORDER. It also drops the thp_migration_supported() check.
> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 0734c0fa21d05..d8a87913ace37 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
> 
> [ ... ]
> 
>> @@ -3487,13 +3488,13 @@ static struct folio *memcg_numa_migrate_new_folio(struct folio *old_folio,
>>  		VM_BUG_ON(ms->current_node >= MAX_NUMNODES);
>>  	}
>>  
>> -	if (thp_migration_supported() && folio_test_large(old_folio)) {
>> -		return __folio_alloc(GFP_TRANSHUGE_LIGHT | __GFP_THISNODE,
>> -				HPAGE_PMD_ORDER, ms->current_node,
>> -				ms->target_nodes);
>> +	if (folio_test_large(old_folio)) {
>> +		gfp_mask = GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
>> +		order = folio_order(old_folio);
>>  	}
> 
> [Severity: Medium]
> The order fix looks right, but the no-reclaim, single-node policy is now
> applied to every large folio, not only PMD-sized THPs.
> 
> GFP_TRANSHUGE_LIGHT has no __GFP_DIRECT_RECLAIM, and __GFP_THISNODE pins
> the allocation to ms->current_node. For a small-order file folio or an
> mTHP, can the allocation now fail on a fragmented node, with no fallback
> to the other nodes in target_nodes? Before this patch, such folios got an
> order-0 destination allocated with GFP_HIGHUSER_MOVABLE | __GFP_NORETRY,
> which does allow direct reclaim.
> 
> If the allocation fails, migrate_folio_unmap() returns -ENOMEM. In
> migrate_pages_batch() the -ENOMEM case then does this:
> 
>     case -ENOMEM:
>         ...
>         if (is_large && !nosplit) {
>             int ret = try_split_folio(folio, split_folios, mode);
>             ...
>         }
>         ...
>         rc_saved = rc;
>         if (list_empty(&unmap_folios))
>             goto out;
> 
> When the split also fails (for example -EAGAIN from an elevated refcount,
> or a mapping whose minimum folio order is above 0), migrate_pages()
> returns a negative value. Then in __memcg_numa_migrate_pages():
> 
>     ret = migrate_pages(&pages, memcg_numa_migrate_new_folio, ...);
>     putback_movable_pages(&pages);
>     if (ret < 0)
>         return ret;
> 
> Does this abort the whole memory.numa_migrate write with -ENOMEM and
> leave the remaining folios of the memcg unmigrated?
> 
> The old PMD-THP path had the same failure semantics. This patch widens
> it to all large folio orders, and the commit message doesn't mention the
> change in allocation policy.
> 
> Would it be reasonable to fall back to order 0 or to a reclaim-capable
> gfp mask when the large allocation fails?

The allocation policy for large folios does not change here. Before
the patch every folio_test_large() folio already took the
GFP_TRANSHUGE_LIGHT | __GFP_THISNODE branch, only with HPAGE_PMD_ORDER.
The order-0 path with reclaim was reached for a large folio only when
thp_migration_supported() was false, which it never is on x86_64. So a
fragmented node is now asked for an order-4 folio where it used to be
asked for an order-9 one.

The fallback you suggest is in migrate_pages() already. On -ENOMEM
from the allocator migrate_pages_batch() calls try_split_folio() and
goes on with the batch, and migrate_pages() then runs the split pieces
through the same allocator as order-0 folios: GFP_HIGHUSER_MOVABLE |
__GFP_NORETRY, the whole target nodemask, no __GFP_THISNODE. Only when
the split fails too does the write return -ENOMEM with the rest
unmigrated, which is what a PMD THP did before the patch.

Whether large destinations should drop __GFP_THISNODE and allow direct
reclaim, as upstream alloc_migration_target() does with GFP_TRANSHUGE
and the nodemask, is a separate change, not part of this fix.


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel