[PATCH] mm/hotplug: fix offline undo_isolate_page_range()

[PATCH] mm/hotplug: fix offline undo_isolate_page_range()

[Qian Cai]

The commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
memory to zones until online") introduced move_pfn_range_to_zone() which
calls memmap_init_zone() during onlining a memory block.
memmap_init_zone() will reset pagetype flags and makes migrate type to
be MOVABLE.

However, in __offline_pages(), it also call undo_isolate_page_range()
after offline_isolated_pages() to do the same thing. Due to
the commit 2ce13640b3f4 ("mm: __first_valid_page skip over offline
pages") changed __first_valid_page() to skip offline pages,
undo_isolate_page_range() here just waste CPU cycles looping around the
offlining PFN range while doing nothing, because __first_valid_page()
will return NULL as offline_isolated_pages() has already marked all
memory sections within the pfn range as offline via
offline_mem_sections().

Also, after calling the "useless" undo_isolate_page_range() here, it
reaches the point of no returning by notifying MEM_OFFLINE. Those pages
will be marked as MIGRATE_MOVABLE again once onlining. The only thing
left to do is to decrease the number of isolated pageblocks zone
counter which would make some paths of the page allocation slower that
the above commit introduced. Fix an incorrect comment along the way.

Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/memory_hotplug.c | 18 ++++++++++++++++--
 mm/sparse.c         |  2 +-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index cd23c081924d..260a8e943483 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1661,8 +1661,22 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	/* Ok, all of our target is isolated.
 	   We cannot do rollback at this point. */
 	offline_isolated_pages(start_pfn, end_pfn);
-	/* reset pagetype flags and makes migrate type to be MOVABLE */
-	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
+
+	/*
+	 * Onlining will reset pagetype flags and makes migrate type
+	 * MOVABLE, so just need to decrease the number of isolated
+	 * pageblocks zone counter here.
+	 */
+	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
+		int i;
+
+		for (i = 0; i < pageblock_nr_pages; i++)
+			if (pfn_valid_within(pfn + i)) {
+				zone->nr_isolate_pageblock--;
+				break;
+			}
+	}
+
 	/* removal success */
 	adjust_managed_page_count(pfn_to_page(start_pfn), -offlined_pages);
 	zone->present_pages -= offlined_pages;
diff --git a/mm/sparse.c b/mm/sparse.c
index 69904aa6165b..56e057c432f9 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -567,7 +567,7 @@ void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn)
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-/* Mark all memory sections within the pfn range as online */
+/* Mark all memory sections within the pfn range as offline */
 void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] mm/hotplug: fix offline undo_isolate_page_range()

[Oscar Salvador]

On Tue, Mar 12, 2019 at 09:42:16PM -0400, Qian Cai wrote:
> +
> +	/*
> +	 * Onlining will reset pagetype flags and makes migrate type
> +	 * MOVABLE, so just need to decrease the number of isolated
> +	 * pageblocks zone counter here.
> +	 */
> +	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
> +		int i;
> +
> +		for (i = 0; i < pageblock_nr_pages; i++)
> +			if (pfn_valid_within(pfn + i)) {
> +				zone->nr_isolate_pageblock--;
> +				break;
> +			}
> +	}
> +

I do not really like this.

I first thought about saving the value before entering start_isolate_page_range,
but that could race with alloc_contig_range for instance.
So, why not make start_isolate_page_range to return the actual number of isolated
pageblocks?
Sure, that would mean to change a bit how we threat its return code, but
I think that it is pretty simple.
In that way, we would only have to substract the value start_isolate_page_range
gave us at the end of __offline__pages() to set nr_isolate_pageblock back to
its original value.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH] mm/hotplug: fix offline undo_isolate_page_range()

[Michal Hocko]

On Wed 13-03-19 08:52:16, Oscar Salvador wrote:
> On Tue, Mar 12, 2019 at 09:42:16PM -0400, Qian Cai wrote:
> > +
> > +	/*
> > +	 * Onlining will reset pagetype flags and makes migrate type
> > +	 * MOVABLE, so just need to decrease the number of isolated
> > +	 * pageblocks zone counter here.
> > +	 */
> > +	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
> > +		int i;
> > +
> > +		for (i = 0; i < pageblock_nr_pages; i++)
> > +			if (pfn_valid_within(pfn + i)) {
> > +				zone->nr_isolate_pageblock--;
> > +				break;
> > +			}
> > +	}
> > +
> 
> I do not really like this.
> 
> I first thought about saving the value before entering start_isolate_page_range,
> but that could race with alloc_contig_range for instance.

Yup. We need to take the zone lock.

> So, why not make start_isolate_page_range to return the actual number of isolated
> pageblocks?

That makes more sense indeed.

-- 
Michal Hocko
SUSE Labs