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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <225868db-b9b0-3614-de0f-4b264023df2b@suse.cz>
Date:   Tue, 14 Apr 2020 13:49:45 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Roman Gushchin <guro@...com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Michal Hocko <mhocko@...nel.org>, linux-mm@...ck.org,
        kernel-team@...com, linux-kernel@...r.kernel.org,
        Rik van Riel <riel@...riel.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Qian Cai <cai@....pw>, Andrea Arcangeli <aarcange@...hat.com>,
        Joonsoo Kim <js1304@...il.com>
Subject: Re: [PATCH RFC] mm: compaction: avoid migrating non-cma pages to a
 cma area

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