few memory offlining enhancements

From: Michal Hocko <[email protected]>

do_migrate_range has been limiting the number of pages to migrate to 256 for some reason which is not documented. Even if the limit made some sense back then when it was introduced it doesn't really serve a good purpose these days. If the range contains huge pages then
we break out of the loop too early and go through LRU and pcp caches draining and scan_movable_pages is quite suboptimal.

The only reason to limit the number of pages I can think of is to reduce the potential time to react on the fatal signal. But even then the number of pages is a questionable metric because even a single page might migration block in a non-killable state (e.g. __unmap_and_move).

Remove the limit and offline the full requested range (this is one membblock worth of pages with the current code). Should we ever get a report that offlining takes too long to react on fatal signal then we should rather fix the core migration to use killable waits and bailout on a signal.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/memory_hotplug.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

mm/memory_hotplug.c +2 -5

index c82193db4be6..6263c8cd4491 100644
@@ -1339,18 +1339,16 @@ static struct page *new_node_page(struct page *page, unsigned long private)
 	return new_page_nodemask(page, nid, &nmask);
 }
 
-#define NR_OFFLINE_AT_ONCE_PAGES	(256)
 static int
 do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
 	struct page *page;
-	int move_pages = NR_OFFLINE_AT_ONCE_PAGES;
 	int not_managed = 0;
 	int ret = 0;
 	LIST_HEAD(source);
 
-	for (pfn = start_pfn; pfn < end_pfn && move_pages > 0; pfn++) {
+	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
 		if (!pfn_valid(pfn))
 			continue;
 		page = pfn_to_page(pfn);

@@ -1362,8 +1360,7 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 				ret = -EBUSY;
 				break;
 			}
-			if (isolate_huge_page(page, &source))
-				move_pages -= 1 << compound_order(head);
+			isolate_huge_page(page, &source);
 			continue;
 		} else if (PageTransHuge(page))
 			pfn = page_to_pfn(compound_head(page))

(we could also check for pending signals inside that function if really required)

Reviewed-by David Hildenbrand <[email protected]>

On Tue 20-11-18 15:18:41, David Hildenbrand wrote:
[...]

Quoted 2 lines

(we could also check for pending signals inside that function if really required)

do_migrate_pages is not the proper layer to check signals. Because the loop only isolates pages and that is not expensive. The most expensive part is deeper down in the migration core. We wait for page lock or writeback and that can take a long. None of that is killable wait which is a larger surgery but something that we should consider should there be any need to address this.

Quoted 2 lines

Reviewed-by David Hildenbrand <[email protected]>

Thanks!

From: Michal Hocko <[email protected]>

do_migrate_range has been limiting the number of pages to migrate to 256 for some reason which is not documented. Even if the limit made some sense back then when it was introduced it doesn't really serve a good purpose these days. If the range contains huge pages then
we break out of the loop too early and go through LRU and pcp caches draining and scan_movable_pages is quite suboptimal.

The only reason to limit the number of pages I can think of is to reduce the potential time to react on the fatal signal. But even then the number of pages is a questionable metric because even a single page might migration block in a non-killable state (e.g. __unmap_and_move).

Remove the limit and offline the full requested range (this is one membblock worth of pages with the current code). Should we ever get a report that offlining takes too long to react on fatal signal then we should rather fix the core migration to use killable waits and bailout on a signal.

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

do_migrate_range has been limiting the number of pages to migrate to 256
for some reason which is not documented.

Signed-off-by Michal Hocko <[email protected]>

Quoted 5 lines · Michal Hocko

From: Michal Hocko <[email protected]>

do_migrate_range has been limiting the number of pages to migrate to 256
for some reason which is not documented.

When looking back at old memory-hotplug commits one feels pretty sad about the brevity of the changelogs.

Quoted 1 line

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

From: Michal Hocko <[email protected]>

Memory migration might fail during offlining and we keep retrying in that case. This is currently obfuscate by goto retry loop. The code is hard to follow and as a result it is even suboptimal becase each retry round scans the full range from start_pfn even though we have successfully scanned/migrated [start_pfn, pfn] range already. This is all only because check_pages_isolated failure has to rescan the full range again.

De-obfuscate the migration retry loop by promoting it to a real for loop. In fact remove the goto altogether by making it a proper double loop (yeah, gotos are nasty in this specific case). In the end we will get a slightly more optimal code which is better readable.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/memory_hotplug.c | 60 +++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

mm/memory_hotplug.c +0 -1

index 6263c8cd4491..9cd161db3061 100644
@@ -1591,38 +1591,40 @@ static int __ref __offline_pages(unsigned long start_pfn,
 		goto failed_removal_isolated;
 	}
 
-	pfn = start_pfn;

mm/memory_hotplug.c +7 -3

+			if (signal_pending(current)) {
+				reason = "signal backoff";
+				goto failed_removal_isolated;
+			}
 
-	cond_resched();
-	lru_add_drain_all();
-	drain_all_pages(zone);
+			cond_resched();
+			lru_add_drain_all();
+			drain_all_pages(zone);

mm/memory_hotplug.c +15 -6

+			}
+		}
+
+		/*
+		 * dissolve free hugepages in the memory block before doing offlining
+		 * actually in order to make hugetlbfs's object counting consistent.
+		 */
+		ret = dissolve_free_huge_pages(start_pfn, end_pfn);
+		if (ret) {
+			reason = "failure to dissolve huge pages";
+			goto failed_removal_isolated;
+		}
+		/* check again */
+		offlined_pages = check_pages_isolated(start_pfn, end_pfn);
+	} while (offlined_pages < 0);
 
-	/*
-	 * dissolve free hugepages in the memory block before doing offlining
-	 * actually in order to make hugetlbfs's object counting consistent.
-	 */
-	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
-	if (ret) {

Review

Quoted 3 lines

(quoted diff) +3 -0

+	do {
+		for (pfn = start_pfn; pfn;)
+		{

{ on a new line looks weird.

Review

Quoted 2 lines

(quoted diff) +2 -0

+			/* start memory hot removal */
+			ret = -EINTR;

I think we can move that into the "if (signal_pending(current))"

(if my eyes are not wrong, this will not be touched otherwise)

On Tue 20-11-18 15:26:43, David Hildenbrand wrote:
[...]

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	do {
+		for (pfn = start_pfn; pfn;)
+		{

{ on a new line looks weird.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+			/* start memory hot removal */
+			ret = -EINTR;

I think we can move that into the "if (signal_pending(current))"

(if my eyes are not wrong, this will not be touched otherwise)

Better?

mm/memory_hotplug.c +0 -0

index 9cd161db3061..6bc3aee30f5e 100644
@@ -1592,11 +1592,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	}
 
 	do {

Signed-off-by Michal Hocko <[email protected]>

mm/memory_hotplug.c +11 -8

+			ret = -EINTR;
+			if (signal_pending(current)) {
+				reason = "signal backoff";
+				goto failed_removal_isolated;
+			}
 
-	cond_resched();
-	lru_add_drain_all();
-	drain_all_pages(zone);
+			cond_resched();
+			lru_add_drain_all();
+			drain_all_pages(zone);
 
-	pfn = scan_movable_pages(start_pfn, end_pfn);
-	if (pfn) { /* We have movable pages */
-		ret = do_migrate_range(pfn, end_pfn);
-		goto repeat;
-	}
+			pfn = scan_movable_pages(pfn, end_pfn);
+			if (pfn) {
+				/* TODO fatal migration failures

should bail out */

mm/memory_hotplug.c +6 -0

+				do_migrate_range(pfn, end_pfn);
+			}
+		}
+
+		/*
+		 * dissolve free hugepages in the memory block

before doing offlining

(quoted diff) +1 -0

+		 * actually in order to make hugetlbfs's object

counting consistent.

mm/memory_hotplug.c +8 -0

+		 */
+		ret = dissolve_free_huge_pages(start_pfn, end_pfn);
+		if (ret) {
+			reason = "failure to dissolve huge pages";
+			goto failed_removal_isolated;
+		}
+		/* check again */
+		offlined_pages = check_pages_isolated(start_pfn,

end_pfn);

mm/memory_hotplug.c +1 -2

+	} while (offlined_pages < 0);
 
-	/*
-	 * dissolve free hugepages in the memory block before doing

offlining

(quoted diff) +0 -1

-	 * actually in order to make hugetlbfs's object counting

consistent.

Quoted 2 lines

Signed-off-by Michal Hocko <[email protected]>

[...]

mm/memory_hotplug.c static int __ref __offline_pages(unsigned long start_pfn, · line 1597

Quoted 4 lines

mm/memory_hotplug.c +4 -0

+	do {
+		for (pfn = start_pfn; pfn;)
+		{
+			/* start memory hot removal */

Should we change thAT comment? I mean, this is not really the hot- removal stage.

Maybe "start memory migration" suits better? or memory offlining?

This indeed looks much nicer and it is easier to follow. With the changes commented by David:

Reviewed-by Oscar Salvador <[email protected]>

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 9 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Looks fine to me.

But please drop a line of comment in the code. As is it might be confusing for a reader.

Quoted 11 lines · Kirill A. Shutemov

Quoted 9 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 1 line

Looks fine to me.

Thanks for the review.

Quoted 2 lines

But please drop a line of comment in the code. As is it might be confusing for a reader.

This?

mm/filemap.c +4 -0

index c76d6a251770..7c4e439a2e85 100644
@@ -2554,6 +2554,10 @@ void filemap_map_pages(struct vm_fault *vmf,
 
 		head = compound_head(page);
 
+		/*
+		 * Check the locked pages before taking a reference to not
+		 * go in the way of migration.
+		 */
 		if (PageLocked(head))
 			goto next;
 		if (!page_cache_get_speculative(head))

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 16 lines · Michal Hocko

Quoted 11 lines · Kirill A. Shutemov

Quoted 9 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 1 line

Looks fine to me.

Thanks for the review.

Quoted 4 lines

Quoted 2 lines

But please drop a line of comment in the code. As is it might be confusing for a reader.

This?

Yep.

Acked-by Kirill A. Shutemov <[email protected]>

mm/filemap.c void filemap_map_pages(struct vm_fault *vmf, · line 2560

Quoted 8 lines · Michal Hocko

mm/filemap.c +4 -0

+		/*
+		 * Check the locked pages before taking a reference to not
+		 * go in the way of migration.
+		 */

Could you make this a tiny bit more explanative, something like:

Reviewed-by William Kucharski <[email protected]>

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Cc "Kirill A. Shutemov" <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/filemap.c | 3 +++
 1 file changed, 3 insertions(+)

mm/filemap.c +0 -0

index 81adec8ee02c..c76d6a251770 100644

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Cc "Kirill A. Shutemov" <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/filemap.c | 3 +++
 1 file changed, 3 insertions(+)

mm/filemap.c +3 -0

index 81adec8ee02c..c76d6a251770 100644
@@ -2553,6 +2553,9 @@ void filemap_map_pages(struct vm_fault *vmf,
 			goto next;
 
 		head = compound_head(page);
+
+		if (PageLocked(head))
+			goto next;
 		if (!page_cache_get_speculative(head))
 			goto next;
 

Signature · 1 line

2.19.1

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 3 lines

Cc "Kirill A. Shutemov" <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Hugh Dickins <[email protected]>

though I think this patch is more useful to the avoid atomic ops, and unnecessary dirtying of the cacheline, than to avoid the very transient elevation of refcount, which will not affect page migration very much.

Quoted 12 lines · Hugh Dickins

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 5 lines

Quoted 3 lines

Cc "Kirill A. Shutemov" <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Hugh Dickins <[email protected]>

Thanks!

Quoted 4 lines

though I think this patch is more useful to the avoid atomic ops, and unnecessary dirtying of the cacheline, than to avoid the very transient elevation of refcount, which will not affect page migration very much.

Are you sure it would really be transient? In other words is it possible that the fault around can block migration repeatedly under refault heavy workload? I just couldn't convince myself, to be honest.

Quoted 21 lines · Michal Hocko

Quoted 12 lines · Hugh Dickins

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

filemap_map_pages takes a speculative reference to each page in the range before it tries to lock that page. While this is correct it also can influence page migration which will bail out when seeing an elevated reference count. The faultaround code would bail on seeing a locked page so we can pro-actively check the PageLocked bit before page_cache_get_speculative and prevent from pointless reference count churn.

Quoted 5 lines

Quoted 3 lines

Cc "Kirill A. Shutemov" <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Hugh Dickins <[email protected]>

Thanks!

Quoted 8 lines

Quoted 4 lines

though I think this patch is more useful to the avoid atomic ops, and unnecessary dirtying of the cacheline, than to avoid the very transient elevation of refcount, which will not affect page migration very much.

Are you sure it would really be transient? In other words is it possible that the fault around can block migration repeatedly under refault heavy workload? I just couldn't convince myself, to be honest.

I don't deny that it is possible: I expect that, using fork() (which does not copy the ptes in a shared file vma), you can construct a test case where each child faults one or another page near a page of no interest, and that page of no interest is a target of migration perpetually frustrated by filemap_map_pages()'s briefly raised refcount.

But I suggest that's a third-order effect: well worth fixing because it's easily and uncontroversially dealt with, as you have; but not of great importance.

The first-order effect is migration conspiring to defeat itself: that's what my put_and_wait_on_page_locked() patch, in other thread, is about.

The second order effect is when a page that is really wanted is waited on - the target of a fault, for which page refcount is raised maybe long before it finally gets into the page table (whereupon it becomes visible to try_to_unmap(), and its mapcount matches refcount so that migration can fully account for the page). One class of that can be well dealt with by using put_and_wait_on_page_locked_killable() in lock_page_or_retry(), but I was keeping that as a future instalment.

But I shouldn't denigrate the transient case by referring so lightly to migrate_pages()' 10 attempts: each of those failed attempts can be very expensive, unmapping and TLB flushing (including IPIs) and remapping. It may well be that 2 or 3 would be a more cost-effective number of attempts, at least when the page is mapped.

If you want some update to the comment in this function or to the changelog, I am open of course. Right now I have

(quoted diff) +2 -0

+                * Check for a locked page first, as a speculative
+                * reference may adversely influence page migration.

as suggested by William.