fix THP migration for CMA allocations

Add information to struct compact_control to indicate that the allocator would really like to clear out this specific part of memory, used by for example CMA.

Signed-off-by Rik van Riel <[email protected]>

---
 mm/internal.h   | 1 +
 mm/page_alloc.c | 1 +
 2 files changed, 2 insertions(+)

mm/internal.h +1 -0

index 3cf20ab3ca01..78492d9815b4 100644
@@ -206,6 +206,7 @@ struct compact_control {
 	bool whole_zone;		/* Whole zone should/has been scanned */
 	bool contended;			/* Signal lock or sched contention */
 	bool rescan;			/* Rescanning the same pageblock */
+	bool alloc_contig;		/* alloc_contig_range allocation */
 };
 
 /*

mm/page_alloc.c +1 -0

index 3c4eb750a199..a36736812596 100644
@@ -8402,6 +8402,7 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 		.ignore_skip_hint = true,
 		.no_set_skip_hint = true,
 		.gfp_mask = current_gfp_context(gfp_mask),
+		.alloc_contig = true,
 	};
 	INIT_LIST_HEAD(&cc.migratepages);
 

The code to implement THP migrations already exists, and the code for CMA to clear out a region of memory already exists.

Only a few small tweaks are needed to allow CMA to move THP memory when attempting an allocation from alloc_contig_range.

With these changes, migrating THPs from a CMA area works when allocating a 1GB hugepage from CMA memory.

Signed-off-by Rik van Riel <[email protected]>

---
 mm/compaction.c | 16 +++++++++-------
 mm/page_alloc.c |  6 ++++--
 2 files changed, 13 insertions(+), 9 deletions(-)

mm/compaction.c +0 -0

index 672d3c78c6ab..f3e05c91df62 100644
@@ -894,12 +894,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/*

mm/compaction.c +5 -3

-		if (PageCompound(page)) {
+		if (PageCompound(page) && !cc->alloc_contig) {
 			const unsigned int order = compound_order(page);
 
 			if (likely(order < MAX_ORDER))

@@ -969,7 +969,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 			 * and it's on LRU. It can only be a THP so the order
 			 * is safe to read and it's 0 for tail pages.
 			 */
-			if (unlikely(PageCompound(page))) {
+			if (unlikely(PageCompound(page) && !cc->alloc_contig)) {
 				low_pfn += compound_nr(page) - 1;
 				goto isolate_fail;
 			}

@@ -981,7 +981,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		if (__isolate_lru_page(page, isolate_mode) != 0)
 			goto isolate_fail;
 
-		VM_BUG_ON_PAGE(PageCompound(page), page);
+		/* The whole page is taken off the LRU; skip the tail pages. */
+		if (PageCompound(page))
+			low_pfn += compound_nr(page) - 1;
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));

mm/page_alloc.c +4 -2

index a36736812596..38c8ddfcecc8 100644
@@ -8253,14 +8253,16 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page,
 
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
+		 * THPs are on the LRU, but need to be counted as #small pages.
 		 * We need not scan over tail pages because we don't
 		 * handle each tail page individually in migration.
 		 */
-		if (PageHuge(page)) {
+		if (PageTransHuge(page)) {
 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
-			if (!hugepage_migration_supported(page_hstate(head)))
+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(head)))
 				return page;
 
 			skip_pages = compound_nr(head) - (page - head);

Signature · 1 line

2.24.1

Quoted 18 lines · Rik van Riel

+++ b/mm/compaction.c

(quoted diff) +0 -0

@@ -894,12 +894,12 @@ isolate_migratepages_block(struct
 compact_control *cc, unsigned long low_pfn,
 
 		/*
 		 * Regardless of being on LRU, compound pages such as

THP and

(quoted diff) +0 -1

-		 * hugetlbfs are not to be compacted. We can

potentially save

(quoted diff) +0 -1

-		 * a lot of iterations if we skip them at once. The

check is

(quoted diff) +0 -1

-		 * racy, but we can consider only valid values and the

only

(quoted diff) +1 -1

-		 * danger is skipping too much.
+		 * hugetlbfs are not to be compacted most of the time.

We can

(quoted diff) +1 -0

+		 * potentially save a lot of iterations if we skip them

at

mm/compaction.c isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, · line 901

Quoted 4 lines

(quoted diff) +1 -0

+		 * once. The check is racy, but we can consider only

valid

mm/compaction.c +1 -0

+		 * values and the only danger is skipping too much.
 		 */

Maybe add “we do want to move them when allocating contiguous memory using CMA” to help
people understand the context of using cc->alloc_contig?

mm/page_alloc.c +0 -0

index a36736812596..38c8ddfcecc8 100644

mm/page_alloc.c +2 -1

 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
-			if (!hugepage_migration_supported(page_hstate(head)))
+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(head)))
 				return page;
 
 			skip_pages = compound_nr(head) - (page - head);

mm/compaction.c isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, · line 989

Quoted 13 lines · Rik van Riel

mm/compaction.c +1 -1

@@ -981,7 +981,9 @@ isolate_migratepages_block(struct
 compact_control *cc, unsigned long low_pfn,
 		if (__isolate_lru_page(page, isolate_mode) != 0)
 			goto isolate_fail;
 
-		VM_BUG_ON_PAGE(PageCompound(page), page);
+		/* The whole page is taken off the LRU; skip the tail

pages. */

mm/compaction.c +2 -0

+		if (PageCompound(page))
+			low_pfn += compound_nr(page) - 1;
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));

This continues by:
inc_node_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));

I think it now needs to use mod_node_page_state() with hpage_nr_pages(page) otherwise the counter will underflow after the migration?

Quoted 7 lines

mm/page_alloc.c +0 -0

index a36736812596..38c8ddfcecc8 100644
@@ -8253,14 +8253,16 @@ struct page *has_unmovable_pages(struct
 zone *zone, struct page *page,
 

mm/page_alloc.c struct page *has_unmovable_pages(struct zone *zone, struct page *page, · line 8260

Quoted 9 lines

mm/page_alloc.c +2 -1

 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
+		 * THPs are on the LRU, but need to be counted as
 #small pages.
 		 * We need not scan over tail pages because we don't
 		 * handle each tail page individually in migration.
 		 */
-		if (PageHuge(page)) {
+		if (PageTransHuge(page)) {

Hmm, PageTransHuge() has VM_BUG_ON() for tail pages, while this code is
written so that it can encounter a tail page and skip the rest of the compound page properly. So I would be worried about this.

Also PageTransHuge() is basically just a PageHead() so for each non-hugetlbfs compound page this will assume it's a THP, while correctly
it should reach the __PageMovable() || PageLRU(page) tests below.

So probably this should do something like.

if (PageHuge(page) || PageTransCompound(page)) {
...
if (PageHuge(page) && !hpage_migration_supported)) return page.

   if (!PageLRU(head) && !__PageMovable(head)) return page

...

Quoted 12 lines

mm/page_alloc.c +0 -1

 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
-			if

(!hugepage_migration_supported(page_hstate(head)))

mm/page_alloc.c +2 -0

+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(h

ead)))

mm/page_alloc.c +0 -0

 				return page;
 
 			skip_pages = compound_nr(head) - (page - head);

Quoted 10 lines

Also PageTransHuge() is basically just a PageHead() so for each non-hugetlbfs compound page this will assume it's a THP, while correctly
it should reach the __PageMovable() || PageLRU(page) tests below.

So probably this should do something like.

if (PageHuge(page) || PageTransCompound(page)) {
...
if (PageHuge(page) && !hpage_migration_supported)) return page.

So far so good.

Quoted 1 line

   if (!PageLRU(head) && !__PageMovable(head)) return page

I don't get this one, though. What about a THP that has not made it onto the LRU list yet for some reason?

I don't think anonymous pages are marked __PageMovable, are they? It looks like they only have the PAGE_MAPPING_ANON flag set, not the PAGE_MAPPING_MOVABLE one.

What am I missing?

Quoted 17 lines

...

Quoted 12 lines

mm/page_alloc.c +0 -1

 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
-			if

(!hugepage_migration_supported(page_hstate(head)))

(quoted diff) +2 -0

+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(h

ead)))

mm/page_alloc.c +0 -0

 				return page;
 
 			skip_pages = compound_nr(head) - (page - head);

Quoted 14 lines · Rik van Riel

Quoted 11 lines

Also PageTransHuge() is basically just a PageHead() so for each non-hugetlbfs compound page this will assume it's a THP, while correctly
it should reach the __PageMovable() || PageLRU(page) tests below.

So probably this should do something like.

if (PageHuge(page) || PageTransCompound(page)) {
...
if (PageHuge(page) && !hpage_migration_supported)) return
page.

So far so good.

Quoted 4 lines

Quoted 1 line

   if (!PageLRU(head) && !__PageMovable(head)) return page

I don't get this one, though. What about a THP that has not made it onto the LRU list yet for some reason?

Uh, is it any different from base pages which have to pass the same check? I
guess the caller could do e.g. lru_add_drain_all() first.

Quoted 5 lines

I don't think anonymous pages are marked __PageMovable, are they? It looks like they only have the PAGE_MAPPING_ANON flag set, not the PAGE_MAPPING_MOVABLE one.

What am I missing?

My point is that we should not accept compound pages that are neither a
migratable hugetlbfs page nor a THP, as movable.

Quoted 5 lines · Vlastimil Babka

On 2/25/20 7:44 PM, Rik van Riel wrote:

Uh, is it any different from base pages which have to pass the same check? I
guess the caller could do e.g. lru_add_drain_all() first.

You are right, it is not different.

As for lru_add_drain_all(), I wonder at what point that should happen?

It appears that the order in which things are done does not really provide a good moment:
1) decide to attempt allocating a range of memory
2) scan each page block for unmovable pages
3) if no unmovable pages are found, mark the page block MIGRATE_ISOLATE

I wonder if we should do things the opposite way, first marking the page block MIGRATE_ISOLATE (to prevent new allocations), then scanning it, and calling lru_add_drain_all if we encounter a page that looks like it could benefit from that.

If we still see unmovable pages after that, it is cheap enough to set the page block back to its previous state.

Quoted 8 lines · Rik van Riel

It appears that the order in which things are done does not really provide a good moment:
1) decide to attempt allocating a range of memory
2) scan each page block for unmovable pages
3) if no unmovable pages are found, mark the page block MIGRATE_ISOLATE

Quoted 8 lines

I wonder if we should do things the opposite way, first marking the page block MIGRATE_ISOLATE (to prevent new allocations), then scanning it, and calling lru_add_drain_all if we encounter a page that looks like it could benefit from that.

If we still see unmovable pages after that, it is cheap enough to set the page block back to its previous state.

Yeah seems like the whole has_unmovable_pages() thing isn't much useful
here. It might prevent some unnecessary action like isolating something,
then finding non-movable page and rolling back the isolation. But maybe
it's not worth the savings, and also has_unmovable_pages() being false
doesn't guarantee succeed in the actual isolate+migrate attempt. And if
it can cause a false negative due to lru pages not drained, then it's actually worse than if it wasn't called at all.