Date:   Thu, 25 May 2017 08:27:22 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...e.de>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Jerome Glisse <jglisse@...hat.com>,
        Reza Arbab <arbab@...ux.vnet.ibm.com>,
        Yasuaki Ishimatsu <yasu.isimatu@...il.com>,
        qiuxishi@...wei.com, Kani Toshimitsu <toshi.kani@....com>,
        slaoub@...il.com, Joonsoo Kim <js1304@...il.com>,
        Andi Kleen <ak@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Daniel Kiper <daniel.kiper@...cle.com>,
        Igor Mammedov <imammedo@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] mm, memory_hotplug: drop CONFIG_MOVABLE_NODE

On Wed 24-05-17 17:17:08, Vlastimil Babka wrote:
> On 05/24/2017 03:42 PM, Michal Hocko wrote:
[...]
> >>> --- a/mm/Kconfig
> >>> +++ b/mm/Kconfig
> >>> @@ -149,32 +149,6 @@ config NO_BOOTMEM
> >>>  config MEMORY_ISOLATION
> >>>  	bool
> >>>  
> >>> -config MOVABLE_NODE
> >>> -	bool "Enable to assign a node which has only movable memory"
> >>> -	depends on HAVE_MEMBLOCK
> >>> -	depends on NO_BOOTMEM
> >>> -	depends on X86_64 || OF_EARLY_FLATTREE || MEMORY_HOTPLUG
> >>> -	depends on NUMA
> >>
> >> That's a lot of depends. What happens if some of them are not met and
> >> the movable_node bootparam is used?
> > 
> > Good question. I haven't explored that, to be honest. Now that I am looking closer
> > I am not even sure why all those dependencies are thre. MEMORY_HOTPLUG
> > is clear and OF_EARLY_FLATTREE is explained by 41a9ada3e6b4 ("of/fdt:
> > mark hotpluggable memory"). NUMA is less clear to me because
> > MEMORY_HOTPLUG doesn't really depend on NUMA systems. Dependency on
> > NO_BOOTMEM is also not clear to me because zones layout
> > doesn't really depend on the specific boot time allocator.
> > 
> > So we are left with HAVE_MEMBLOCK which seems to be there because
> > movable_node_enabled is defined there while the parameter handling is in
> > the hotplug proper. But there is no real reason to have it like that.
> > This compiles but I will have to put throw my full compile battery on it
> > to be sure. I will make it a separate patch.
> 
> I'd expect stuff might compile and work (run without crash), just in
> some cases the boot option could be effectively ignored? In that case
> it's just a matter of documenting the option, possibly also some warning
> when used, e.g. "node_movable was ignored because CONFIG_FOO is not
> enabled"?

Hmm, I can make the cmd parameter available only when
CONFIG_HAVE_MEMBLOCK_NODE_MAP but I am not sure how helpful it would be.
AFAIR unrecognized options are just ignored. On the other hand debugging
why the parameter doesn't do anything might be really frustrating. Here
is the patch I will put on top of the two posted. Strictly speaking it
breaks the bisection but swithing the order would be kind of pointless
ifdefery game and I do not see it would matter all that much. I can
rework if you guys think otherwise though.
---
>From 4ed5cca9399f9b1e616478160ed5320d3951ec29 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@...e.com>
Date: Wed, 24 May 2017 15:43:49 +0200
Subject: [PATCH] mm, memory_hotplug: move movable_node to the hotplug proper

movable_node_is_enabled is defined in memblock proper while it
is initialized from the memory hotplug proper. This is quite messy
and it makes a dependency between the two so move movable_node along
with the helper functions to memory_hotplug.

To make it more entertaining the kernel parameter is ignored unless
CONFIG_HAVE_MEMBLOCK_NODE_MAP=y because we do not have the node
information for each memblock otherwise. So let's warn when the option
is disabled.

Signed-off-by: Michal Hocko <mhocko@...e.com>
---
 include/linux/memblock.h       |  7 -------
 include/linux/memory_hotplug.h | 10 ++++++++++
 mm/memblock.c                  |  1 -
 mm/memory_hotplug.c            |  6 ++++++
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 9622fb8c101b..071692894254 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -57,8 +57,6 @@ struct memblock {
 
 extern struct memblock memblock;
 extern int memblock_debug;
-/* If movable_node boot option specified */
-extern bool movable_node_enabled;
 
 #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
 #define __init_memblock __meminit
@@ -171,11 +169,6 @@ static inline bool memblock_is_hotpluggable(struct memblock_region *m)
 	return m->flags & MEMBLOCK_HOTPLUG;
 }
 
-static inline bool __init_memblock movable_node_is_enabled(void)
-{
-	return movable_node_enabled;
-}
-
 static inline bool memblock_is_mirror(struct memblock_region *m)
 {
 	return m->flags & MEMBLOCK_MIRROR;
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 9e0249d0f5e4..d6e5e63b31d5 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -115,6 +115,12 @@ extern void __online_page_free(struct page *page);
 extern int try_online_node(int nid);
 
 extern bool memhp_auto_online;
+/* If movable_node boot option specified */
+extern bool movable_node_enabled;
+static inline bool movable_node_is_enabled(void)
+{
+	return movable_node_enabled;
+}
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
 extern bool is_pageblock_removable_nolock(struct page *page);
@@ -266,6 +272,10 @@ static inline void put_online_mems(void) {}
 static inline void mem_hotplug_begin(void) {}
 static inline void mem_hotplug_done(void) {}
 
+static inline bool movable_node_is_enabled(void)
+{
+	return false;
+}
 #endif /* ! CONFIG_MEMORY_HOTPLUG */
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
diff --git a/mm/memblock.c b/mm/memblock.c
index 4895f5a6cf7e..8c52fb11510c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -54,7 +54,6 @@ struct memblock memblock __initdata_memblock = {
 };
 
 int memblock_debug __initdata_memblock;
-bool movable_node_enabled __initdata_memblock = false;
 static bool system_has_some_mirror __initdata_memblock = false;
 static int memblock_can_resize __initdata_memblock;
 static int memblock_memory_in_slab __initdata_memblock = 0;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2a14f8c18a22..1a148b35e8a3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -79,6 +79,8 @@ static struct {
 #define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
 #define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
 
+bool movable_node_enabled = false;
+
 #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
 bool memhp_auto_online;
 #else
@@ -1561,7 +1563,11 @@ check_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
 
 static int __init cmdline_parse_movable_node(char *p)
 {
+#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
 	movable_node_enabled = true;
+#else
+	pr_warn("movable_node parameter depends on CONFIG_HAVE_MEMBLOCK_NODE_MAP to work properly\n");
+#endif
 	return 0;
 }
 early_param("movable_node", cmdline_parse_movable_node);
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists