fix THP migration for CMA allocations

PATCH In review Rik van Riel 3 participants 11 messages Feb 27, 2020 – Mar 2, 2020

Subsystems (3)

Memory management MM core Page allocator

lore

Share & tools

lore

Rik van Riel

author

2020-02-27 21:32 UTC #0 lore raw

v2 cover fix THP migration for CMA allocations

Transparent huge pages are allocated with __GFP_MOVABLE, and can end up in CMA memory blocks. Transparent huge pages also have most of the infrastructure in place to allow migration.

However, a few pieces were missing, causing THP migration to fail when attempting to use CMA to allocate 1GB hugepages.

With these patches in place, THP migration from CMA blocks seems to work, both for anonymous THPs and for tmpfs/shmem THPs.

v2:
- addressed comments by Vlastimil Babka and Zi Yan

Rik van Riel (2):
mm,compaction,cma: add alloc_contig flag to compact_control mm,thp,compaction,cma: allow THP migration for CMA allocations

 mm/compaction.c | 16 +++++++++-------
 mm/internal.h   |  1 +
 mm/page_alloc.c |  7 +++++--
 3 files changed, 15 insertions(+), 9 deletions(-)

Signature · 1 line

2.24.1

Rik van Riel

author

2020-02-27 21:32 UTC #1 lore raw

PATCH v2 1/2 mm,compaction,cma: add alloc_contig flag to compact_control

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]>

Reviewed-by Vlastimil Babka <[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);

Signature · 1 line

2.24.1

Rik van Riel

author

2020-02-27 21:32 UTC #2 lore raw

PATCH v2 2/2 mm,thp,compaction,cma: allow THP migration for CMA allocations

In reply to Rik van Riel

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]>

Reviewed-by Zi Yan <[email protected]>

---
 mm/compaction.c | 22 +++++++++++++---------
 mm/page_alloc.c |  9 +++++++--
 2 files changed, 20 insertions(+), 11 deletions(-)

mm/compaction.c +13 -9

index 672d3c78c6ab..000ade085b89 100644
@@ -894,12 +894,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/*
 		 * Regardless of being on LRU, compound pages such as THP and
-		 * hugetlbfs are not to be compacted. We can potentially save
-		 * a lot of iterations if we skip them at once. The check is
-		 * racy, but we can consider only valid values and the only
-		 * danger is skipping too much.
+		 * hugetlbfs are not to be compacted unless we are attempting
+		 * an allocation much larger than the huge page size (eg CMA).
+		 * We can potentially save a lot of iterations if we skip them
+		 * at once. The check is racy, but we can consider only valid
+		 * values and the only danger is skipping too much.
 		 */
-		if (PageCompound(page)) {
+		if (PageCompound(page) && !cc->alloc_contig) {
 			const unsigned int order = compound_order(page);
 
 			if (likely(order < MAX_ORDER))

@@ -969,7 +970,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,12 +982,15 @@ 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));
-		inc_node_page_state(page,
-				NR_ISOLATED_ANON + page_is_file_cache(page));
+		mod_node_page_state(page_pgdat(page),
+				NR_ISOLATED_ANON + page_is_file_cache(page),
+				hpage_nr_pages(page));
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);

mm/page_alloc.c +7 -2

index a36736812596..6257c849cc00 100644
@@ -8253,14 +8253,19 @@ 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 (PageHuge(page) || PageTransCompound(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;
+
+			if (!PageLRU(head) && !__PageMovable(head))
 				return page;
 
 			skip_pages = compound_nr(head) - (page - head);

Signature · 1 line

2.24.1

Mike Kravetz

2020-02-27 23:41 UTC #3 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

Quoted 80 lines · Rik van Riel

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]>

Reviewed-by Zi Yan <[email protected]>

---
 mm/compaction.c | 22 +++++++++++++---------
 mm/page_alloc.c |  9 +++++++--
 2 files changed, 20 insertions(+), 11 deletions(-)

mm/compaction.c +13 -9

index 672d3c78c6ab..000ade085b89 100644
@@ -894,12 +894,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/*
 		 * Regardless of being on LRU, compound pages such as THP and
-		 * hugetlbfs are not to be compacted. We can potentially save
-		 * a lot of iterations if we skip them at once. The check is
-		 * racy, but we can consider only valid values and the only
-		 * danger is skipping too much.
+		 * hugetlbfs are not to be compacted unless we are attempting
+		 * an allocation much larger than the huge page size (eg CMA).
+		 * We can potentially save a lot of iterations if we skip them
+		 * at once. The check is racy, but we can consider only valid
+		 * values and the only danger is skipping too much.
 		 */
-		if (PageCompound(page)) {
+		if (PageCompound(page) && !cc->alloc_contig) {
 			const unsigned int order = compound_order(page);
 
 			if (likely(order < MAX_ORDER))

@@ -969,7 +970,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,12 +982,15 @@ 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));
-		inc_node_page_state(page,
-				NR_ISOLATED_ANON + page_is_file_cache(page));
+		mod_node_page_state(page_pgdat(page),
+				NR_ISOLATED_ANON + page_is_file_cache(page),
+				hpage_nr_pages(page));
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);

mm/page_alloc.c +1 -1

index a36736812596..6257c849cc00 100644
@@ -8253,14 +8253,19 @@ 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)) {

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

Quoted 10 lines

mm/page_alloc.c +6 -1

+		if (PageHuge(page) || PageTransCompound(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;
+
+			if (!PageLRU(head) && !__PageMovable(head))

Pretty sure this is going to be true for hugetlb pages. So, this will change behavior and make all hugetlb pages look unmovable. Perhaps, only check this condition for THP pages?

Signature · 1 line

Mike Kravetz

Quoted 4 lines

mm/page_alloc.c +0 -0

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

Rik van Riel

author

2020-02-28 01:21 UTC #4 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

Quoted 18 lines · Mike Kravetz

Quoted 17 lines · Rik van Riel

+++ b/mm/page_alloc.c

mm/page_alloc.c +2 -1

@@ -8253,14 +8253,19 @@ 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 (PageHuge(page) || PageTransCompound(page)) {
 			struct page *head = compound_head(page);
 			unsigned int skip_pages;

Quoted 14 lines

mm/page_alloc.c struct page *has_unmovable_pages(struct zone *zone, struct page *page, · line 8268
Quoted 8 lines
(quoted diff) +2 -1
-			if
 (!hugepage_migration_supported(page_hstate(head)))
+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(h
ead)))
mm/page_alloc.c +3 -0
+				return page;
+
+			if (!PageLRU(head) && !__PageMovable(head))
Pretty sure this is going to be true for hugetlb pages. So, this will change
behavior and make all hugetlb pages look unmovable. Perhaps, only check this
condition for THP pages?

Does that need to be the following, then?

     if (PageTransHuge(head) && !PageHuge(page) && !PageLRU(head) &&
!__PageMovable(head))
                 return page;

That's an easy one liner I would be happy to send in if everybody agrees that should fix things :)

Signature · 1 line

All Rights Reversed.

Mike Kravetz

2020-02-28 04:30 UTC #5 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

Quoted 34 lines · Rik van Riel

Quoted 18 lines · Mike Kravetz
Quoted 17 lines · Rik van Riel
+++ b/mm/page_alloc.c
mm/page_alloc.c +2 -1
@@ -8253,14 +8253,19 @@ 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 (PageHuge(page) || PageTransCompound(page)) {
 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
Quoted 14 lines
mm/page_alloc.c struct page *has_unmovable_pages(struct zone *zone, struct page *page, · line 8268
Quoted 8 lines
(quoted diff) +2 -1
-			if
 (!hugepage_migration_supported(page_hstate(head)))
+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(h
ead)))
mm/page_alloc.c +3 -0
+				return page;
+
+			if (!PageLRU(head) && !__PageMovable(head))
Pretty sure this is going to be true for hugetlb pages. So, this will change
behavior and make all hugetlb pages look unmovable. Perhaps, only check this
condition for THP pages?

Quoted 5 lines

Does that need to be the following, then?

     if (PageTransHuge(head) && !PageHuge(page) && !PageLRU(head) &&
!__PageMovable(head))
                 return page;

Yes, that is what I would suggest. Results of a simple test on small VM.

Unmodified Kernel
-----------------

# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
50
# hugeadm --hard --pool-pages-min DEFAULT:4G
# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
50

V2 patches
----------

# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
50
# hugeadm --hard --pool-pages-min DEFAULT:4G
# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
14

V2 patches + above modification
-------------------------------

# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
50
# hugeadm --hard --pool-pages-min DEFAULT:4G
# cat /sys/devices/system/memory/memory*/removable | grep 1 | wc -l
50

Quoted 2 lines

That's an easy one liner I would be happy to send in if everybody agrees that should fix things :)

Another option might be to make hugetlb pages more like other pages and __SetPageMovable on movable hugetlb pages. This could be used instead of hugepage_migration_supported() calls. That is only a quick thought and would be beyond the scope of this patch. If people think that is worth doing, and I have not overlooked something, I could look into it separately.

Signature · 1 line

Mike Kravetz

Vlastimil Babka

2020-02-28 08:25 UTC #6 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

In reply to Rik van Riel

Quoted 19 lines · Rik van Riel

Quoted 18 lines · Mike Kravetz

Quoted 17 lines · Rik van Riel

+++ b/mm/page_alloc.c

mm/page_alloc.c +2 -1

@@ -8253,14 +8253,19 @@ 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 (PageHuge(page) || PageTransCompound(page)) {
 			struct page *head = compound_head(page);
 			unsigned int skip_pages;

Quoted 14 lines

Quoted 6 lines
Quoted 6 lines
(quoted diff) +2 -1
-			if
 (!hugepage_migration_supported(page_hstate(head)))
+			if (PageHuge(page) &&
+			    !hugepage_migration_supported(page_hstate(h
ead)))
mm/page_alloc.c +1 -0
+				return page;
Quoted 8 lines
mm/page_alloc.c struct page *has_unmovable_pages(struct zone *zone, struct page *page, · line 8268
Quoted 2 lines
mm/page_alloc.c +2 -0
+
+			if (!PageLRU(head) && !__PageMovable(head))
Pretty sure this is going to be true for hugetlb pages. So, this will change
behavior and make all hugetlb pages look unmovable. Perhaps, only check this
condition for THP pages?

Oh right you are.

Quoted 5 lines

Does that need to be the following, then?

     if (PageTransHuge(head) && !PageHuge(page) && !PageLRU(head) &&
!__PageMovable(head))
                 return page;

I would instead make it an "else if" to the "if (PageHuge(page)...)" above.

Quoted 3 lines

That's an easy one liner I would be happy to send in if everybody agrees that should fix things :)

Rik van Riel

author

2020-02-28 14:32 UTC #7 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

Quoted 47 lines · Vlastimil Babka

Quoted 33 lines · Rik van Riel
Quoted 32 lines · Mike Kravetz
Quoted 31 lines · Rik van Riel
+++ b/mm/page_alloc.c
(quoted diff) +0 -0
@@ -8253,14 +8253,19 @@ struct page *has_unmovable_pages(struct
 zone *zone, struct page *page,
 
 		/*
 		 * Hugepages are not in LRU lists, but they're
movable.
(quoted diff) +1 -0
+		 * 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.
mm/page_alloc.c +1 -1
 		 */
-		if (PageHuge(page)) {
+		if (PageHuge(page) || PageTransCompound(page))
{
struct page *head =
compound_head(page);
mm/page_alloc.c +0 -1
 			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 +3 -0
+				return page;
+
+			if (!PageLRU(head) &&
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
!__PageMovable(head))
Pretty sure this is going to be true for hugetlb pages. So, this will change
behavior and make all hugetlb pages look unmovable. Perhaps, only
check this
condition for THP pages?
Oh right you are.
Quoted 2 lines
Does that need to be the following, then?

Quoted 7 lines

Quoted 4 lines
     if (PageTransHuge(head) && !PageHuge(page) && !PageLRU(head)
&&
!__PageMovable(head))
                 return page;
I would instead make it an "else if" to the "if (PageHuge(page)...)" above.

That was my first thought too, but that could break on pages that are PageHuge when hugepage_migration_supported returns true.

Signature · 1 line

All Rights Reversed.

Vlastimil Babka

2020-02-28 14:39 UTC #8 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

Quoted 7 lines · Rik van Riel

Quoted 7 lines
Quoted 6 lines
Does that need to be the following, then?
     if (PageTransHuge(head) && !PageHuge(page) && !PageLRU(head)
&&
!__PageMovable(head))
                 return page;

Quoted 6 lines

Quoted 2 lines
I would instead make it an "else if" to the "if (PageHuge(page)...)" above.
That was my first thought too, but that could break on pages that are PageHuge when hugepage_migration_supported returns true.

Right, so then

if (PageHuge()) {
	if (!migration_supported) return false;
} else if (!PageLRU(head) ...) {

etc...

IMHO it's better than adding more tests to the second if.

Rik van Riel

author

2020-02-28 15:47 UTC #9 lore raw

PATCH 1/1 fix mmthpcompactioncma-allow-thp-migration-for-cma-allocations.patch

Thank you Mike & Vlastimil!

---8<---

commit 27f3cd5473d8bbf591b61d8b93b98bc333980d0d

Author: Rik van Riel <[email protected]>
Date:   Fri Feb 28 10:41:48 2020 -0500

Subject: fix mmthpcompactioncma-allow-thp-migration-for-cma-allocations.patch

Mike Kravetz pointed out that the second if condition could do the wrong thing for hugetlbfs pages, and that check really only needs to run on THPs.

Cleanup suggested by Vlastimil.

Thank you both!

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

mm/page_alloc.c +5 -5

index 4afa13dd3738..71f78a590236 100644
@@ -8274,12 +8274,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page,
 			struct page *head = compound_head(page);
 			unsigned int skip_pages;
 
-			if (PageHuge(page) &&
-			    !hugepage_migration_supported(page_hstate(head)))
-				return page;
-
-			if (!PageLRU(head) && !__PageMovable(head))
+			if (PageHuge(page)) {
+				if (!hugepage_migration_supported(page_hstate(head)))
+					return page;
+			} else if (!PageLRU(head) && !__PageMovable(head)) {
 				return page;
+			}
 
 			skip_pages = compound_nr(head) - (page - head);
 			iter += skip_pages - 1;

Vlastimil Babka

Reviewed-by on v2 2/2

2020-03-02 09:32 UTC #10 lore raw

Re: [PATCH v2 2/2] mm,thp,compaction,cma: allow THP migration for CMA allocations

In reply to Rik van Riel

Quoted 11 lines · Rik van Riel

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]>
Reviewed-by Zi Yan <[email protected]>

With the followup fix,

Reviewed-by Vlastimil Babka <[email protected]>

Thanks.