Re: [PATCH RESEND v2 4/6] mm/page_alloc: sort out the alloc_contig_range() gfp flags mess

[David Hildenbrand <david@redhat.com>]

On 03.12.24 14:55, Vlastimil Babka wrote:
> On 12/3/24 10:47, David Hildenbrand wrote:
>> It's all a bit complicated for alloc_contig_range(). For example, we don't
>> support many flags, so let's start bailing out on unsupported
>> ones -- ignoring the placement hints, as we are already given the range
>> to allocate.
>>
>> While we currently set cc.gfp_mask, in __alloc_contig_migrate_range() we
>> simply create yet another GFP mask whereby we ignore the reclaim flags
>> specify by the caller. That looks very inconsistent.
>>
>> Let's clean it up, constructing the gfp flags used for
>> compaction/migration exactly once. Update the documentation of the
>> gfp_mask parameter for alloc_contig_range() and alloc_contig_pages().
>>
>> Acked-by: Zi Yan <ziy@nvidia.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> 
>> +	/*
>> +	 * Flags to control page compaction/migration/reclaim, to free up our
>> +	 * page range. Migratable pages are movable, __GFP_MOVABLE is implied
>> +	 * for them.
>> +	 *
>> +	 * Traditionally we always had __GFP_HARDWALL|__GFP_RETRY_MAYFAIL set,
>> +	 * keep doing that to not degrade callers.
>> +	 */
> 
> Wonder if we could revisit that eventually. Why limit migration targets by
> cpuset via __GFP_HARDWALL if we were not called with __GFP_HARDWALL? And why
> weaken the attempts with __GFP_RETRY_MAYFAIL if we didn't specify it?

See below.

> 
> Unless I'm missing something, cc->gfp is only checked for __GFP_FS and
> __GFP_NOWARN in few places, so it's mostly migration_target_control the
> callers could meaningfully influence.

Note the fist change in the file, where we now use the mask instead of coming up
with another one out of the blue. :)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ce7589a4ec01..54594cc4f650 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6294,7 +6294,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
  	int ret = 0;
  	struct migration_target_control mtc = {
  		.nid = zone_to_nid(cc->zone),
-		.gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL,
+		.gfp_mask = cc->gfp_mask,
  		.reason = MR_CONTIG_RANGE,
  	};

GFP_USER contains __GFP_HARDWALL. I am not sure if that matters here, but
likely the thing we are assuming here is that we are migrating a page, and
usually, these are user allocation (except maybe balloon and some other non-lru
movable things).

The __GFP_RETRY_MAYFAIL should be moved to relevant callers a some point,
__GFP_HARDWALL, I really don't know ...

Thanks!

-- 
Cheers,

David / dhildenb