Re: [PATCH RFC] mm: compaction: avoid migrating non-cma pages to a cma area

[Vlastimil Babka]

On 4/8/20 9:41 PM, Roman Gushchin wrote:
> Compaction does treat cma pageblocks on pair with any movable
> pageblocks. It means it can easily move non-cma pages into a cma zone.
> 
> It can create problems for the cma allocator.
> 
> The particular problem I'm looking at is related to btrfs metadata
> pages, which are allocated without __GFP_MOVABLE, but beside that
> are generic pagecache pages. In fact, they are sometimes movable
> and sometimes not, depending on whether they are dirty and also
> on the extent buffer reference counter.

Hm I think I'd rather make such pages really unmovable (by a pin?) than deny the
whole CMA area to compaction. Would it be feasible?

> Compaction moves them to the hugetlb_cma area, and then sometimes
> the cma allocator fails to move them back from the cma area. It
> results in failures of gigantic hugepages allocations.
> 
> Also in general cma areas are reserved close to the end of a zone,
> and it's where compaction tries to migrate pages. It means
> compaction will aggressively fill cma areas, which makes not much
> sense.

So now the free page scanner will have to skip those areas, which is not much
effective. But I suspect a worse problem in __compaction_suitable() which will
now falsely report that there are enough free pages, so compaction will start
but fail to do anytning. Minimally the __zone_watermark_ok() check there would
have to lose ALLOC_CMA, but there might be other similar checks that would need
adjusting.

And long-term what happens if the "CMA using ZONE_MOVABLE" approach is merged
and there are not more CMA migratetypes to test? Might this change actually also
avoid your issue, as said pages without __GFP_MOVABLE won't end up in a
ZONE_MOVABLE?

> So to avoid it, let's preserve non-cma pages from being moved into
> a cma area. Because cma areas are usually quite large and the number
> of areas is small, it should not significantly affect the memory
> fragmentation.
> 
> Signed-off-by: Roman Gushchin <guro@xxxxxx>
> ---
>  mm/compaction.c | 6 ++++++
>  mm/internal.h   | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 46f0fcc93081..9b047cbb1c74 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1159,6 +1159,10 @@ static bool suitable_migration_target(struct compact_control *cc,
>  			return false;
>  	}
>  
> +	/* Do not bring pages non-cma pages into a cma area */
> +	if (is_migrate_cma(get_pageblock_migratetype(page)) && !cc->cma)
> +		return false;
> +
>  	if (cc->ignore_block_suitable)
>  		return true;
>  
> @@ -1832,6 +1836,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>  		if (!low_pfn)
>  			return ISOLATE_ABORT;
>  
> +		cc->cma = is_migrate_cma(get_pageblock_migratetype(page));
> +
>  		/*
>  		 * Either we isolated something and proceed with migration. Or
>  		 * we failed and compact_zone should decide if we should
> diff --git a/mm/internal.h b/mm/internal.h
> index b5634e78f01d..0ce649da824b 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -232,6 +232,7 @@ struct compact_control {
>  	bool contended;			/* Signal lock or sched contention */
>  	bool rescan;			/* Rescanning the same pageblock */
>  	bool alloc_contig;		/* alloc_contig_range allocation */
> +	bool cma;			/* migratepages contains cma pages */
>  };
>  
>  /*
>