mm/memory_hotplug: online/offline_pages refactorings
Subsystems (4)
cover mm/memory_hotplug: online/offline_pages refactorings
While looking into onlining/offlining of subsections, I noticed that online/offlining code can in its current form only deal with whole sections and that onlining/offlining of sections that are already online/offline is problematic. So let's add some additional checks (that also serve as implicit documentation) and do some cleanups.
David Hildenbrand (5):
mm/memory_hotplug: drop intermediate __offline_pages mm/memory_hotplug: enforce section alignment when onlining/offlining mm/memory_hotplug: check if sections are already online/offline mm/memory_hotplug: onlining pages can only fail due to notifiers mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
include/linux/mmzone.h | 2 ++ mm/memory_hotplug.c | 43 ++++++++++++++++++++++-------------------- mm/sparse.c | 28 +++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 20 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/5 mm/memory_hotplug: drop intermediate __offline_pages
Let's avoid this indirection and just call the function offline_pages().
--- mm/memory_hotplug.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
mm/memory_hotplug.c +3 -10
@@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)node_clear_state (node,N_MEMORY ); } -static int __ref __offline_pages (unsigned long start_pfn, -unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages (unsigned long start_pfn,unsigned long nr_pages) { -unsigned long pfn, nr_pages; +unsigned long pfn, end_pfn = start_pfn + nr_pages;long offlined_pages;int ret, node;unsigned long flags;
@@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn, zone =page_zone (pfn_to_page (valid_start)); node =zone_to_nid (zone); - nr_pages = end_pfn - start_pfn;/* set above range as isolated */ ret = start_isolate_page_range(start_pfn, end_pfn,
@@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn,undo_isolate_page_range (start_pfn, end_pfn,MIGRATE_MOVABLE ); return ret; } - -/* Must be protected by mem_hotplug_begin() or a device_lock */ -int offline_pages (unsigned long start_pfn,unsigned long nr_pages) -{ -return __offline_pages (start_pfn, start_pfn + nr_pages); -}#endif /* CONFIG_MEMORY_HOTREMOVE */ /**
Signature · 1 line
2.17.1
Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages
Hi David,
mm/memory_hotplug.c static void node_states_clear_node(int node, struct memory_notify *arg) · line 1593
Quoted 5 lines · On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <[email protected]> wrote:
mm/memory_hotplug.c +2
-2-static int __ref __offline_pages (unsigned long start_pfn, -unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages (unsigned long start_pfn,unsigned long nr_pages)
You lose the __ref marking. Does this introduce warnings since offline_pages() calls (at least) zone_pcp_update() which is marked __meminit.
Signature · 2 lines
Cheers, Stephen Rothwell
Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages
Quoted 4 lines · Stephen Rothwell
Hi David,
Quoted 1 line · On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 9 lines
mm/memory_hotplug.c static void node_states_clear_node(int node, struct memory_notify *arg) · line 1593
Quoted 4 lines
mm/memory_hotplug.c +2
-2-static int __ref __offline_pages (unsigned long start_pfn, -unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages (unsigned long start_pfn,unsigned long nr_pages)You lose the
__refmarking. Does this introduce warnings sinceoffline_pages()calls (at least)zone_pcp_update()which is marked__meminit.
Good point, I'll recompile and in case there is a warning, keep the __ref. Thanks!
Signature · 3 lines
Thanks, David / dhildenb
kbuild test robot
build botRe: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages
In reply to David HildenbrandI love your patch! Perhaps something to improve:
Hi David,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.18 next-20180816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Hildenbrand/mm-memory_hotplug-online-offline_pages-refactorings/20180817-002307
config: i386-randconfig-a0-201832 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce:
# save the attached .config to linux build tree
make ARCH=i386 All warnings (new ones prefixed by >>):
>> WARNING: vmlinux.o(.text+0x175eea): Section mismatch in reference from the function offline_pages() to the function .meminit.text:init_per_zone_wmark_min()
The function offline_pages() references
the function __meminit init_per_zone_wmark_min().
This is often because offline_pages lacks a __meminit annotation or the annotation of init_per_zone_wmark_min is wrong.
--
>> WARNING: vmlinux.o(.text+0x17624f): Section mismatch in reference from the function offline_pages() to the function .meminit.text:zone_pcp_update()
The function offline_pages() references
the function __meminit zone_pcp_update().
This is often because offline_pages lacks a __meminit annotation or the annotation of zone_pcp_update is wrong.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages
In reply to David HildenbrandI guess the wrap was done because of __ref, but no reason to have this wrap. So looks good to me.
Quoted 47 lines · David Hildenbrand
Let's avoid this indirection and just call the function
offline_pages().Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)mm/memory_hotplug.c +3
-10@@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)node_clear_state (node,N_MEMORY ); } -static int __ref __offline_pages (unsigned long start_pfn, -unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages (unsigned long start_pfn,unsigned long nr_pages) { -unsigned long pfn, nr_pages; +unsigned long pfn, end_pfn = start_pfn + nr_pages;long offlined_pages;int ret, node;unsigned long flags;@@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn, zone =page_zone (pfn_to_page (valid_start)); node =zone_to_nid (zone); - nr_pages = end_pfn - start_pfn;/* set above range as isolated */ ret = start_isolate_page_range(start_pfn, end_pfn,@@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn,undo_isolate_page_range (start_pfn, end_pfn,MIGRATE_MOVABLE ); return ret; } - -/* Must be protected by mem_hotplug_begin() or a device_lock */ -int offline_pages (unsigned long start_pfn,unsigned long nr_pages) -{ -return __offline_pages (start_pfn, start_pfn + nr_pages); -}#endif /* CONFIG_MEMORY_HOTREMOVE */ /**
Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages
Quoted 20 lines · Pasha Tatashin
I guess the wrap was done because of
__ref, but no reason to have this wrap. So looks good to me.Reviewed-by Pavel Tatashin <[email protected]>>Quoted 15 lines · David Hildenbrand
Let's avoid this indirection and just call the function
offline_pages().Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)mm/memory_hotplug.c +0
-0@@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)node_clear_state (node,N_MEMORY ); }
Quoted 4 lines
mm/memory_hotplug.c static void node_states_clear_node(int node, struct memory_notify *arg) · line 1593
Quoted 4 lines
mm/memory_hotplug.c +2
-2-static int __ref __offline_pages (unsigned long start_pfn, -unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages (unsigned long start_pfn,unsigned long nr_pages)
^^^
I meant to say keep the __ref, otherwise looks good.
Thank you,
Pavel
PATCH 2/5 mm/memory_hotplug: enforce section alignment when onlining/offlining
In reply to David Hildenbrandonlining/offlining code works on whole sections, so let's enforce that. Existing code only allows to add memory in memory block size. And only whole memory blocks can be onlined/offlined. Memory blocks are always aligned to sections, so this should not break anything.
online_pages/offline_pages will implicitly mark whole sections online/offline, so the code really can only handle such granularities.
(especially offlining code cannot deal with pageblock_nr_pages but
theoretically only MAX_ORDER-1)
--- mm/memory_hotplug.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
mm/memory_hotplug.c +7 -3
@@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typstruct memory_notify arg;struct memory_block *mem; +if (!IS_ALIGNED (pfn,PAGES_PER_SECTION )) +return -EINVAL ; +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION )) +return -EINVAL ; +/* * We can't use pfn_to_nid() because nid might be stored in struct page * which is not yet initialized. Instead, we find nid from memory block.
@@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)struct zone *zone;struct memory_notify arg; -/* at least, alignment against pageblock is necessary */ -if (!IS_ALIGNED (start_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (start_pfn,PAGES_PER_SECTION ))return -EINVAL ; -if (!IS_ALIGNED (end_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION ))return -EINVAL ;/* This makes hotplug much easier...and readable. we assume this for now. .*/
Signature · 1 line
2.17.1
Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining
Quoted 5 lines · David Hildenbrand
onlining/offlining code works on whole sections, so let's enforce that. Existing code only allows to add memory in memory block size. And only whole memory blocks can be onlined/offlined. Memory blocks are always aligned to sections, so this should not break anything.
Quoted 7 lines
online_pages/offline_pages will implicitly mark whole sections online/offline, so the code really can only handle such granularities.
(especially offlining code cannot deal with
pageblock_nr_pagesbut
theoretically only MAX_ORDER-1)Signed-off-by David Hildenbrand <[email protected]>
Hi David,
If you are really willing to move the checks from this patch[1] to online/offline_pages, you might consider to put that in as well. So we have a function that checks for everything, and not multiple checks.
Another thing is that I would have prefered to take the checks up to memory_block_action, but offline_pages gets also called from ppc-memtrace code.
Other than that,
[1] https://patchwork.kernel.org/patch/10567277/
Thanks
Signature · 2 lines
Oscar Salvador SUSE L3
Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining
In reply to David HildenbrandHi David,
I am not sure this is needed, because we already have a stricter checker:
check_hotplug_memory_range()
You could call it from online_pages(), if you think there is a reason to do it, but other than that it is done from add_memory_resource() and from remove_memory().
Thank you,
Pavel
Quoted 46 lines · David Hildenbrand
onlining/offlining code works on whole sections, so let's enforce that. Existing code only allows to add memory in memory block size. And only whole memory blocks can be onlined/offlined. Memory blocks are always aligned to sections, so this should not break anything.
online_pages/offline_pages will implicitly mark whole sections online/offline, so the code really can only handle such granularities.
(especially offlining code cannot deal with
pageblock_nr_pagesbut
theoretically only MAX_ORDER-1)Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)mm/memory_hotplug.c +7
-3@@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typstruct memory_notify arg;struct memory_block *mem; +if (!IS_ALIGNED (pfn,PAGES_PER_SECTION )) +return -EINVAL ; +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION )) +return -EINVAL ; +/* * We can't use pfn_to_nid() because nid might be stored in struct page * which is not yet initialized. Instead, we find nid from memory block. @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)struct zone *zone;struct memory_notify arg; -/* at least, alignment against pageblock is necessary */ -if (!IS_ALIGNED (start_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (start_pfn,PAGES_PER_SECTION ))return -EINVAL ; -if (!IS_ALIGNED (end_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION ))return -EINVAL ;/* This makes hotplug much easier...and readable. we assume this for now. .*/
Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining
Quoted 9 lines · Pasha Tatashin
Hi David,
I am not sure this is needed, because we already have a stricter checker:
check_hotplug_memory_range()You could call it from
online_pages(), if you think there is a reason to do it, but other than that it is done fromadd_memory_resource()and fromremove_memory().
Hi,
As offline_pages() is called from a different location for ppc (and I understand why but don't consider this clean) and I used both functions in a prototype, believing they would work with pageblock_nr_pages, I really think that we should at least drop the misleading check from offline_pages() and better also add checks for check_hotplug_memory_range().
Thanks for having a look Pavel!
Quoted 50 lines
Thank you,
PavelQuoted 45 lines · David Hildenbrand
onlining/offlining code works on whole sections, so let's enforce that. Existing code only allows to add memory in memory block size. And only whole memory blocks can be onlined/offlined. Memory blocks are always aligned to sections, so this should not break anything.
online_pages/offline_pages will implicitly mark whole sections online/offline, so the code really can only handle such granularities.
(especially offlining code cannot deal with
pageblock_nr_pagesbut
theoretically only MAX_ORDER-1)Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)mm/memory_hotplug.c +7
-3@@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typstruct memory_notify arg;struct memory_block *mem; +if (!IS_ALIGNED (pfn,PAGES_PER_SECTION )) +return -EINVAL ; +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION )) +return -EINVAL ; +/* * We can't use pfn_to_nid() because nid might be stored in struct page * which is not yet initialized. Instead, we find nid from memory block. @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)struct zone *zone;struct memory_notify arg; -/* at least, alignment against pageblock is necessary */ -if (!IS_ALIGNED (start_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (start_pfn,PAGES_PER_SECTION ))return -EINVAL ; -if (!IS_ALIGNED (end_pfn, pageblock_nr_pages)) +if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION ))return -EINVAL ;/* This makes hotplug much easier...and readable. we assume this for now. .*/
Signature · 3 lines
Thanks, David / dhildenb
PATCH 3/5 mm/memory_hotplug: check if sections are already online/offline
In reply to David HildenbrandLet's add some more sanity checking now that onlining/offlining code works completely on section basis. This will make sure that we will never try to online/offline sections that are already (or partially) in the desired state.
--- include/linux/mmzone.h | 2 ++ mm/memory_hotplug.c | 5 +++++ mm/sparse.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+)
include/linux/mmzone.h +2 -0
@@ -1228,6 +1228,8 @@ static inline int online_section_nr(unsigned long nr) }#ifdef CONFIG_MEMORY_HOTPLUG +bool mem_sections_online (unsigned long pfn,unsigned long end_pfn); +bool mem_sections_offline (unsigned long pfn,unsigned long end_pfn);void online_mem_sections (unsigned long start_pfn,unsigned long end_pfn);#ifdef CONFIG_MEMORY_HOTREMOVE void offline_mem_sections (unsigned long start_pfn,unsigned long end_pfn);
mm/memory_hotplug.c +5 -0
@@ -901,6 +901,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typreturn -EINVAL ;if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION ))return -EINVAL ; +if (!mem_sections_offline (pfn, pfn + nr_pages)) +return -EINVAL ;/* * We can't use pfn_to_nid() because nid might be stored in struct page
@@ -1609,6 +1611,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)return -EINVAL ;if (!IS_ALIGNED (nr_pages,PAGES_PER_SECTION ))return -EINVAL ; +if (!mem_sections_online (start_pfn, end_pfn)) +return -EINVAL ; +/* This makes hotplug much easier...and readable. we assume this for now. .*/ if (!test_pages_in_a_zone (start_pfn, end_pfn, &valid_start, &valid_end))
mm/sparse.c +28 -0
@@ -520,6 +520,34 @@ void __init sparse_init(void)#ifdef CONFIG_MEMORY_HOTPLUG +/* check if all mem sections are online */ +bool mem_sections_online (unsigned long pfn,unsigned long end_pfn) +{ +for (; pfn < end_pfn; pfn +=PAGES_PER_SECTION ) { +unsigned long section_nr =pfn_to_section_nr (pfn); + +if (WARN_ON (!valid_section_nr (section_nr))) +continue ; +if (!online_section_nr (section_nr)) +return false; + } +return true; +} + +/* check if all mem sections are offline */ +bool mem_sections_offline (unsigned long pfn,unsigned long end_pfn) +{ +for (; pfn < end_pfn; pfn +=PAGES_PER_SECTION ) { +unsigned long section_nr =pfn_to_section_nr (pfn); + +if (WARN_ON (!valid_section_nr (section_nr))) +continue ; +if (online_section_nr (section_nr)) +return false; + } +return true; +} +/* Mark all memory sections within the pfn range as online */ void online_mem_sections (unsigned long start_pfn,unsigned long end_pfn) {
Signature · 1 line
2.17.1
Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline
Quoted 4 lines · David Hildenbrand
mm/sparse.c +4
-0+ +/* check if all mem sections are offline */ +bool mem_sections_offline (unsigned long pfn,unsigned long end_pfn) +{
mm/sparse.c void __init sparse_init(void) · line 549
Quoted 10 lines
mm/sparse.c +10
-0+for (; pfn < end_pfn; pfn +=PAGES_PER_SECTION ) { +unsigned long section_nr =pfn_to_section_nr (pfn); + +if (WARN_ON (!valid_section_nr (section_nr))) +continue ; +if (online_section_nr (section_nr)) +return false; + } +return true; +}
AFAICS pages_correctly_probed will catch this first. pages_correctly_probed checks for the section to be:
- present
- valid
- !online
Maybe it makes sense to rename it, and write another pages_correctly_probed routine for the offline case.
So all checks would stay in memory_block_action level, and we would not need the mem_sections_offline/online stuff.
Thanks
Signature · 2 lines
Oscar Salvador SUSE L3
Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline
Quoted 20 lines · Oscar Salvador
Quoted 4 lines · David Hildenbrand
mm/sparse.c +4
-0+ +/* check if all mem sections are offline */ +bool mem_sections_offline (unsigned long pfn,unsigned long end_pfn) +{mm/sparse.c void __init sparse_init(void) · line 549
Quoted 10 lines
mm/sparse.c +10
-0+for (; pfn < end_pfn; pfn +=PAGES_PER_SECTION ) { +unsigned long section_nr =pfn_to_section_nr (pfn); + +if (WARN_ON (!valid_section_nr (section_nr))) +continue ; +if (online_section_nr (section_nr)) +return false; + } +return true; +}AFAICS
pages_correctly_probedwill catch this first.pages_correctly_probedchecks for the section to be:
Quoted 3 lines
- present
- valid
- !online
Right, I missed that function.
Quoted 6 lines
Maybe it makes sense to rename it, and write another
pages_correctly_probedroutine for the offline case.So all checks would stay in
memory_block_actionlevel, and we would not need the mem_sections_offline/online stuff.
I guess I would rather have it all moved into online_pages/offline_pages, so we have a clean interface.
Quoted 3 lines
Thanks
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline
Quoted 21 lines · David Hildenbrand
Quoted 20 lines · Oscar Salvador
Quoted 4 lines · David Hildenbrand
mm/sparse.c +4
-0+ +/* check if all mem sections are offline */ +bool mem_sections_offline (unsigned long pfn,unsigned long end_pfn) +{mm/sparse.c void __init sparse_init(void) · line 549
Quoted 10 lines
mm/sparse.c +10
-0+for (; pfn < end_pfn; pfn +=PAGES_PER_SECTION ) { +unsigned long section_nr =pfn_to_section_nr (pfn); + +if (WARN_ON (!valid_section_nr (section_nr))) +continue ; +if (online_section_nr (section_nr)) +return false; + } +return true; +}AFAICS
pages_correctly_probedwill catch this first.pages_correctly_probedchecks for the section to be:
Quoted 12 lines
Quoted 3 lines
- present
- valid
- !onlineRight, I missed that function.
Quoted 6 lines
Maybe it makes sense to rename it, and write another
pages_correctly_probedroutine for the offline case.So all checks would stay in
memory_block_actionlevel, and we would not need the mem_sections_offline/online stuff.
I am OK with that, but will wait for a patch to review.
Pavel
PATCH 4/5 mm/memory_hotplug: onlining pages can only fail due to notifiers
In reply to David HildenbrandOnlining pages can only fail if a notifier reported a problem (e.g. -ENOMEM). online_pages_range() can never fail.
--- mm/memory_hotplug.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
mm/memory_hotplug.c +2 -7
@@ -933,13 +933,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typsetup_zone_pageset (zone); } - ret =walk_system_ram_range (pfn, nr_pages, &onlined_pages, - online_pages_range); -if (ret) { -if (need_zonelists_rebuild) -zone_pcp_reset (zone); - goto failed_addition; - } +walk_system_ram_range (pfn, nr_pages, &onlined_pages, + online_pages_range); zone->present_pages += onlined_pages;
Signature · 1 line
2.17.1
Re: [PATCH v1 4/5] mm/memory_hotplug: onlining pages can only fail due to notifiers
LGTM
Quoted 29 lines · David Hildenbrand
Onlining pages can only fail if a notifier reported a problem (e.g.
-ENOMEM).online_pages_range()can never fail.Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)mm/memory_hotplug.c +2
-7@@ -933,13 +933,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typsetup_zone_pageset (zone); } - ret =walk_system_ram_range (pfn, nr_pages, &onlined_pages, - online_pages_range); -if (ret) { -if (need_zonelists_rebuild) -zone_pcp_reset (zone); - goto failed_addition; - } +walk_system_ram_range (pfn, nr_pages, &onlined_pages, + online_pages_range); zone->present_pages += onlined_pages;
PATCH 5/5 mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
In reply to David HildenbrandLet's try to minimze the noise.
--- mm/memory_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)
mm/memory_hotplug.c +6 -0
@@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typreturn 0 ; failed_addition: +#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_ONLINE , &arg);return ret; }
@@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) offlined_pages =check_pages_isolated (start_pfn, end_pfn);if (offlined_pages <0 ) goto repeat; +#ifdef CONFIG_DEBUG_VM pr_info ("Offlined Pages %ld\n" , offlined_pages); +#endif /* Ok, all of our target is isolated. We cannot do rollback at this point. */ offline_isolated_pages (start_pfn, end_pfn);
@@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)return 0 ; failed_removal: +#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_OFFLINE , &arg);/* pushback to free area */ undo_isolate_page_range (start_pfn, end_pfn,MIGRATE_MOVABLE );
Signature · 1 line
2.17.1
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif
I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.
But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1706
Quoted 5 lines
mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif
Same goes here.
Thanks
Signature · 2 lines
Oscar Salvador SUSE L3
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
Quoted 11 lines · Oscar Salvador
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think thatpr_errwould be better thanpr_debugand withoutCONFIG_DEBUG_VM.
Quoted 2 lines
But at least, if not, envolve it with a
CONFIG_DEBUG_VM, but changepr_debugtopr_info.
I don't have a clear rule about these debug macro neither.
While when you look at the page related logs in calculate_node_totalpages(), it is KERNEL_DEBUG level and without any config macro.
Maybe we should leave them at the same state?
Quoted 12 lines
mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1706
Quoted 5 lines
mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif Same goes here.
Thanks
Signature · 2 lines
Oscar Salvador SUSE L3
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
Quoted 15 lines · Wei Yang
Quoted 11 lines · Oscar Salvador
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think thatpr_errwould be better thanpr_debugand withoutCONFIG_DEBUG_VM.Quoted 2 lines
But at least, if not, envolve it with a
CONFIG_DEBUG_VM, but changepr_debugtopr_info.
Quoted 6 lines
I don't have a clear rule about these debug macro neither.
While when you look at the page related logs in
calculate_node_totalpages(), it isKERNEL_DEBUGlevel and without any config macro.Maybe we should leave them at the same state?
I guess we can do that for the to debug messages.
When offlining memory right now:
:/# echo 0 > /sys/devices/system/memory/memory9/online [ 24.476207] Offlined Pages 32768 [ 24.477200] remove from free list 48000 1024 50000 [ 24.477896] remove from free list 48400 1024 50000 [ 24.478584] remove from free list 48800 1024 50000 [ 24.479454] remove from free list 48c00 1024 50000 [ 24.480192] remove from free list 49000 1024 50000 [ 24.480957] remove from free list 49400 1024 50000 [ 24.481752] remove from free list 49800 1024 50000 [ 24.482578] remove from free list 49c00 1024 50000 [ 24.483302] remove from free list 4a000 1024 50000 [ 24.484300] remove from free list 4a400 1024 50000 [ 24.484902] remove from free list 4a800 1024 50000 [ 24.485462] remove from free list 4ac00 1024 50000 [ 24.486381] remove from free list 4b000 1024 50000 [ 24.487108] remove from free list 4b400 1024 50000 [ 24.487842] remove from free list 4b800 1024 50000 [ 24.488610] remove from free list 4bc00 1024 50000 [ 24.489548] remove from free list 4c000 1024 50000 [ 24.490392] remove from free list 4c400 1024 50000 [ 24.491224] remove from free list 4c800 1024 50000 ...
While "remove from free list" is pr_info under CONFIG_DEBUG_VM, "Offlined Pages ..." is pr_info without CONFIG_DEBUG_VM.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
Quoted 50 lines · David Hildenbrand
Quoted 15 lines · Wei Yang
Quoted 11 lines · Oscar Salvador
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think thatpr_errwould be better thanpr_debugand withoutCONFIG_DEBUG_VM.Quoted 2 lines
But at least, if not, envolve it with a
CONFIG_DEBUG_VM, but changepr_debugtopr_info.Quoted 6 lines
I don't have a clear rule about these debug macro neither.
While when you look at the page related logs in
calculate_node_totalpages(), it isKERNEL_DEBUGlevel and without any config macro.Maybe we should leave them at the same state?
I guess we can do that for the to debug messages.
When offlining memory right now:
:/# echo 0 > /sys/devices/system/memory/memory9/online [ 24.476207] Offlined Pages 32768 [ 24.477200] remove from free list 48000 1024 50000 [ 24.477896] remove from free list 48400 1024 50000 [ 24.478584] remove from free list 48800 1024 50000 [ 24.479454] remove from free list 48c00 1024 50000 [ 24.480192] remove from free list 49000 1024 50000 [ 24.480957] remove from free list 49400 1024 50000 [ 24.481752] remove from free list 49800 1024 50000 [ 24.482578] remove from free list 49c00 1024 50000 [ 24.483302] remove from free list 4a000 1024 50000 [ 24.484300] remove from free list 4a400 1024 50000 [ 24.484902] remove from free list 4a800 1024 50000 [ 24.485462] remove from free list 4ac00 1024 50000 [ 24.486381] remove from free list 4b000 1024 50000 [ 24.487108] remove from free list 4b400 1024 50000 [ 24.487842] remove from free list 4b800 1024 50000 [ 24.488610] remove from free list 4bc00 1024 50000 [ 24.489548] remove from free list 4c000 1024 50000 [ 24.490392] remove from free list 4c400 1024 50000 [ 24.491224] remove from free list 4c800 1024 50000 ...
Quoted 2 lines
While "remove from free list" is
pr_infounderCONFIG_DEBUG_VM, "Offlined Pages ..." ispr_infowithoutCONFIG_DEBUG_VM.
Hmm... yes, don't see the logic between them.
Quoted 6 lines
Signature · 5 lines
-- Thanks, David / dhildenb
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
In reply to Oscar SalvadorQuoted 10 lines · Oscar Salvador
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think thatpr_errwould be better thanpr_debugand withoutCONFIG_DEBUG_VM.
I consider both error messages only partially useful, as
1. They only catch a subset of actual failures the function handles. E.g. onlining will not report an error message if the memory notifier failed.
2. Onlining/Offlining is usually (with exceptions - e.g. onlining during add_memory) triggered from user space, where we present an error code. At any times, the actual state of the memory blocks can be observed by querying the state.
I would even vote for dropping the two error case messages completely. At least I don't consider them very useful.
Quoted 13 lines
But at least, if not, envolve it with a
CONFIG_DEBUG_VM, but changepr_debugtopr_info.mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1706
Quoted 5 lines
mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif Same goes here.
Thanks
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
Quoted 12 lines · David Hildenbrand
Quoted 10 lines · Oscar Salvador
mm/memory_hotplug.c int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ · line 973
Quoted 6 lines
failed_addition:mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think thatpr_errwould be better thanpr_debugand withoutCONFIG_DEBUG_VM.
Quoted 5 lines
I consider both error messages only partially useful, as
1. They only catch a subset of actual failures the function handles. E.g. onlining will not report an error message if the memory notifier failed.
That statement was wrong. It is rather in offline_pages, errors from start_isolate_page_range() are ignored.
Quoted 23 lines
2. Onlining/Offlining is usually (with exceptions - e.g. onlining during
add_memory) triggered from user space, where we present an error code. At any times, the actual state of the memory blocks can be observed by querying the state.I would even vote for dropping the two error case messages completely. At least I don't consider them very useful.
Quoted 13 lines
But at least, if not, envolve it with a
CONFIG_DEBUG_VM, but changepr_debugtopr_info.mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1706
Quoted 5 lines
mm/memory_hotplug.c +2
-0+#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif Same goes here.
Thanks
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
In reply to David HildenbrandQuoted 36 lines · David Hildenbrand
Let's try to minimze the noise.
Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)mm/memory_hotplug.c +4
-0@@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typreturn 0 ; failed_addition: +#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_ONLINE , &arg);return ret; }@@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) offlined_pages =check_pages_isolated (start_pfn, end_pfn);if (offlined_pages <0 ) goto repeat; +#ifdef CONFIG_DEBUG_VM pr_info ("Offlined Pages %ld\n" , offlined_pages); +#endif /* Ok, all of our target is isolated. We cannot do rollback at this point. */ offline_isolated_pages (start_pfn, end_pfn);@@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)return 0 ;
mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1709
Quoted 10 lines
mm/memory_hotplug.c +2
-0failed_removal: +#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_OFFLINE , &arg);/* pushback to free area */ undo_isolate_page_range (start_pfn, end_pfn,MIGRATE_MOVABLE );
I'll drop this patch for now, maybe the error messages are actually useful when debugging a crashdump of a system without CONFIG_DEBUG_VM.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()
Quoted 37 lines · David Hildenbrand
Quoted 36 lines · David Hildenbrand
Let's try to minimze the noise.
Signed-off-by David Hildenbrand <[email protected]>--- mm/memory_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)mm/memory_hotplug.c +4
-0@@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typreturn 0 ; failed_addition: +#ifdef CONFIG_DEBUG_VM pr_debug ("online_pages [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) pfn <<PAGE_SHIFT , (((unsigned long long ) pfn + nr_pages) <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_ONLINE , &arg);return ret; }@@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) offlined_pages =check_pages_isolated (start_pfn, end_pfn);if (offlined_pages <0 ) goto repeat; +#ifdef CONFIG_DEBUG_VM pr_info ("Offlined Pages %ld\n" , offlined_pages); +#endif /* Ok, all of our target is isolated. We cannot do rollback at this point. */ offline_isolated_pages (start_pfn, end_pfn);@@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)return 0 ;
Quoted 14 lines
mm/memory_hotplug.c int offline_pages(unsigned long start_pfn, unsigned long nr_pages) · line 1709
Quoted 10 lines
mm/memory_hotplug.c +2
-0failed_removal: +#ifdef CONFIG_DEBUG_VM pr_debug ("memory offlining [mem %#010llx-%#010llx] failed\n" , (unsigned long long ) start_pfn <<PAGE_SHIFT , ((unsigned long long ) end_pfn <<PAGE_SHIFT ) -1 ); +#endif memory_notify (MEM_CANCEL_OFFLINE , &arg);/* pushback to free area */ undo_isolate_page_range (start_pfn, end_pfn,MIGRATE_MOVABLE );I'll drop this patch for now, maybe the error messages are actually useful when debugging a crashdump of a system without
CONFIG_DEBUG_VM.
Yes, please drop it, no reason to reduce amount of these messages. They are useful.
Thank you,
Pavel