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

[David Hildenbrand]

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@xxxxxxxxxx>
> > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>
> Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
>
> > +	/*
> > +	 * 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