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 */
> };
>
> /*
>
