* [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline
@ 2019-03-10 20:01 Qian Cai
2019-03-12 15:06 ` Michal Hocko
2019-03-12 15:35 ` Oscar Salvador
0 siblings, 2 replies; 4+ messages in thread
From: Qian Cai @ 2019-03-10 20:01 UTC (permalink / raw)
To: akpm; +Cc: mhocko, vbabka, linux-mm, linux-kernel, 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. In addition, fix
an incorrect comment along the way.
Signed-off-by: Qian Cai <cai@lca.pw>
---
mm/memory_hotplug.c | 2 --
mm/sparse.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6b05576fb4ec..46017040b2f8 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1655,8 +1655,6 @@ 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);
/* 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 77a0554fa5bd..b3771f35a0ed 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -556,7 +556,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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline
2019-03-10 20:01 [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline Qian Cai
@ 2019-03-12 15:06 ` Michal Hocko
2019-03-12 15:35 ` Oscar Salvador
1 sibling, 0 replies; 4+ messages in thread
From: Michal Hocko @ 2019-03-12 15:06 UTC (permalink / raw)
To: Qian Cai; +Cc: akpm, vbabka, linux-mm, linux-kernel
On Sun 10-03-19 16:01:02, Qian Cai wrote:
> 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. In addition, fix
> an incorrect comment along the way.
>
> Signed-off-by: Qian Cai <cai@lca.pw>
Well spotted. Thanks!
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memory_hotplug.c | 2 --
> mm/sparse.c | 2 +-
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6b05576fb4ec..46017040b2f8 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1655,8 +1655,6 @@ 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);
> /* 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 77a0554fa5bd..b3771f35a0ed 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -556,7 +556,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)
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline
2019-03-10 20:01 [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline Qian Cai
2019-03-12 15:06 ` Michal Hocko
@ 2019-03-12 15:35 ` Oscar Salvador
2019-03-12 15:57 ` Michal Hocko
1 sibling, 1 reply; 4+ messages in thread
From: Oscar Salvador @ 2019-03-12 15:35 UTC (permalink / raw)
To: Qian Cai; +Cc: akpm, mhocko, vbabka, linux-mm, linux-kernel
On Sun, Mar 10, 2019 at 04:01:02PM -0400, Qian Cai wrote:
> 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. In addition, fix
> an incorrect comment along the way.
>
> Signed-off-by: Qian Cai <cai@lca.pw>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
One thing I noticed when looking at start_isolate_page_range and
undo_isolate_page_range functions, is that start_isolate_page_range increments
the number of isolated pageblocks, while undo_isolate_page_range does the counter
part.
Since undo_isolate_page_range is really never called during offlining,
we leave zone->nr_isolate_pageblock with a stale value.
I __think__ this does not matter much.
We only get to check whether a zone got isolated pageblocks in
has_isolate_pageblock(), and this is called from:
free_one_page
free_pcppages_bulk
__free_one_page
With a quick glance, the only difference in has_isolate_pageblock() returning
true or false, seems to be that those functions perform some extra checks in
case the zone reports to have isolated pageblocks.
I wonder if we should set nr_isolate_pageblock back to its original value
before start_isolate_page_range.
> ---
> mm/memory_hotplug.c | 2 --
> mm/sparse.c | 2 +-
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6b05576fb4ec..46017040b2f8 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1655,8 +1655,6 @@ 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);
> /* 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 77a0554fa5bd..b3771f35a0ed 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -556,7 +556,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)
>
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline
2019-03-12 15:35 ` Oscar Salvador
@ 2019-03-12 15:57 ` Michal Hocko
0 siblings, 0 replies; 4+ messages in thread
From: Michal Hocko @ 2019-03-12 15:57 UTC (permalink / raw)
To: Oscar Salvador; +Cc: Qian Cai, akpm, vbabka, linux-mm, linux-kernel
On Tue 12-03-19 16:35:06, Oscar Salvador wrote:
[...]
> One thing I noticed when looking at start_isolate_page_range and
> undo_isolate_page_range functions, is that start_isolate_page_range increments
> the number of isolated pageblocks, while undo_isolate_page_range does the counter
> part.
> Since undo_isolate_page_range is really never called during offlining,
> we leave zone->nr_isolate_pageblock with a stale value.
>
> I __think__ this does not matter much.
> We only get to check whether a zone got isolated pageblocks in
> has_isolate_pageblock(), and this is called from:
>
> free_one_page
> free_pcppages_bulk
> __free_one_page
It forces those into slow(er) path. So it makes a difference.
> With a quick glance, the only difference in has_isolate_pageblock() returning
> true or false, seems to be that those functions perform some extra checks in
> case the zone reports to have isolated pageblocks.
>
> I wonder if we should set nr_isolate_pageblock back to its original value
> before start_isolate_page_range.
Yes. And that would be a fixup to my 2ce13640b3f4.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-12 15:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-10 20:01 [RESEND PATCH] mm/hotplug: don't reset pagetype flags for offline Qian Cai
2019-03-12 15:06 ` Michal Hocko
2019-03-12 15:35 ` Oscar Salvador
2019-03-12 15:57 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).