Patchwork [v3,1/2] memory-hotplug: introduce CONFIG_HAVE_BOOTMEM_INFO_NODE and revert register_page_bootmem_info_node() when platform not support

login
register
mail settings
Submitter Lin Feng
Date Jan. 16, 2013, 8:14 a.m.
Message ID <1358324059-9608-2-git-send-email-linfeng@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/212482/
State Not Applicable
Headers show

Comments

Lin Feng - Jan. 16, 2013, 8:14 a.m.
It's implemented by adding a new Kconfig option named
CONFIG_HAVE_BOOTMEM_INFO_NODE, which will be automatically selected by
memory-hotplug feature fully supported archs(currently only on x86_64).

Reported-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Lin Feng <linfeng@cn.fujitsu.com>
---
ChangeLog v2->v3:
- Rename the patch title to conform it's content.
- Update memory_hotplug.h and remove the misleading TODO pointed out by Michal.

ChangeLog v1->v2:
- Add a Kconfig option named HAVE_BOOTMEM_INFO_NODE suggested by Michal, which
  will be automatically selected by supported archs(currently only on x86_64).
---
 arch/x86/mm/init_64.c          |    2 +-
 include/linux/memory_hotplug.h |    6 ++++++
 mm/Kconfig                     |    8 ++++++++
 mm/memory_hotplug.c            |    2 ++
 4 files changed, 17 insertions(+), 1 deletions(-)
Michal Hocko - Jan. 16, 2013, 2:14 p.m.
On Wed 16-01-13 16:14:18, Lin Feng wrote:
[...]
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 278e3ab..f8c5799 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -162,10 +162,18 @@ config MOVABLE_NODE
>  	  Say Y here if you want to hotplug a whole node.
>  	  Say N here if you want kernel to use memory on all nodes evenly.
>  
> +#
> +# Only be set on architectures that have completely implemented memory hotplug
> +# feature. If you are not sure, don't touch it.
> +#
> +config HAVE_BOOTMEM_INFO_NODE
> +	def_bool n
> +
>  # eventually, we can have this option just 'select SPARSEMEM'
>  config MEMORY_HOTPLUG
>  	bool "Allow for memory hot-add"
>  	select MEMORY_ISOLATION
> +	select HAVE_BOOTMEM_INFO_NODE if X86_64
>  	depends on SPARSEMEM || X86_64_ACPI_NUMA
>  	depends on HOTPLUG && ARCH_ENABLE_MEMORY_HOTPLUG
>  	depends on (IA64 || X86 || PPC_BOOK3S_64 || SUPERH || S390)

I am still not sure I understand the relation to MEMORY_HOTREMOVE.
Is register_page_bootmem_info_node required/helpful even if
!CONFIG_MEMORY_HOTREMOVE?

Also, now that I am thinking about that more, maybe it would
be cleaner to put the select into arch/x86/Kconfig and do it
same as ARCH_ENABLE_MEMORY_{HOTPLUG,HOTREMOVE} (and name it
ARCH_HAVE_BOOTMEM_INFO_NODE).
Lin Feng - Jan. 17, 2013, 10:37 a.m.
Hi Michal,

On 01/16/2013 10:14 PM, Michal Hocko wrote:
> On Wed 16-01-13 16:14:18, Lin Feng wrote:
> [...]
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index 278e3ab..f8c5799 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -162,10 +162,18 @@ config MOVABLE_NODE
>>  	  Say Y here if you want to hotplug a whole node.
>>  	  Say N here if you want kernel to use memory on all nodes evenly.
>>  
>> +#
>> +# Only be set on architectures that have completely implemented memory hotplug
>> +# feature. If you are not sure, don't touch it.
>> +#
>> +config HAVE_BOOTMEM_INFO_NODE
>> +	def_bool n
>> +
>>  # eventually, we can have this option just 'select SPARSEMEM'
>>  config MEMORY_HOTPLUG
>>  	bool "Allow for memory hot-add"
>>  	select MEMORY_ISOLATION
>> +	select HAVE_BOOTMEM_INFO_NODE if X86_64
>>  	depends on SPARSEMEM || X86_64_ACPI_NUMA
>>  	depends on HOTPLUG && ARCH_ENABLE_MEMORY_HOTPLUG
>>  	depends on (IA64 || X86 || PPC_BOOK3S_64 || SUPERH || S390)
> 
> I am still not sure I understand the relation to MEMORY_HOTREMOVE.
> Is register_page_bootmem_info_node required/helpful even if
> !CONFIG_MEMORY_HOTREMOVE?
From old kenrel's view register_page_bootmem_info_node() is defined in 
CONFIG_MEMORY_HOTPLUG_SPARSE, it registers some info for 
memory hotplug/remove. If we don't use MEMORY_HOTPLUG feature, this
function is empty, we don't need the info at all.
So this info is not required/helpful if !CONFIG_MEMORY_HOTREMOVE.
> 
> Also, now that I am thinking about that more, maybe it would
> be cleaner to put the select into arch/x86/Kconfig and do it
> same as ARCH_ENABLE_MEMORY_{HOTPLUG,HOTREMOVE} (and name it
> ARCH_HAVE_BOOTMEM_INFO_NODE).
> 
Maybe put it in mm/Kconfig is a better choice, because if one day someone implements
the register_page_bootmem_info_node() for other archs they will get some clues
here, that's it has been implemented on x86_64. 
But I'm not so sure...

thanks,
linfeng
Michal Hocko - Jan. 17, 2013, 1:05 p.m.
On Thu 17-01-13 18:37:10, Lin Feng wrote:
[...]
> > I am still not sure I understand the relation to MEMORY_HOTREMOVE.
> > Is register_page_bootmem_info_node required/helpful even if
> > !CONFIG_MEMORY_HOTREMOVE?
> From old kenrel's view register_page_bootmem_info_node() is defined in 
> CONFIG_MEMORY_HOTPLUG_SPARSE, it registers some info for 
> memory hotplug/remove. If we don't use MEMORY_HOTPLUG feature, this
> function is empty, we don't need the info at all.
> So this info is not required/helpful if !CONFIG_MEMORY_HOTREMOVE.

OK, then I suggest moving it under CONFIG_MEMORY_HOTREMOVE guards rather
than CONFIG_MEMORY_HOTPLUG.

> > Also, now that I am thinking about that more, maybe it would
> > be cleaner to put the select into arch/x86/Kconfig and do it
> > same as ARCH_ENABLE_MEMORY_{HOTPLUG,HOTREMOVE} (and name it
> > ARCH_HAVE_BOOTMEM_INFO_NODE).
> > 
> Maybe put it in mm/Kconfig is a better choice, because if one day
> someone implements the register_page_bootmem_info_node() for other
> archs they will get some clues here, that's it has been implemented on
> x86_64.
> But I'm not so sure...

My understanding is that doing that in arch code is more appropriate
because it makes the generic code less complicated. But I do not have
any strong opinion on that. Looking at other ARCH_ENABLE_MEMORY_HOTPLUG
and others suggests that we should be consistent with that.

Thanks!
Lin Feng - Jan. 18, 2013, 7:58 a.m.
Hi Michal,

On 01/17/2013 09:05 PM, Michal Hocko wrote:
> On Thu 17-01-13 18:37:10, Lin Feng wrote:
> [...]
>>> > > I am still not sure I understand the relation to MEMORY_HOTREMOVE.
>>> > > Is register_page_bootmem_info_node required/helpful even if
>>> > > !CONFIG_MEMORY_HOTREMOVE?
>> > From old kenrel's view register_page_bootmem_info_node() is defined in 
>> > CONFIG_MEMORY_HOTPLUG_SPARSE, it registers some info for 
>> > memory hotplug/remove. If we don't use MEMORY_HOTPLUG feature, this
>> > function is empty, we don't need the info at all.
>> > So this info is not required/helpful if !CONFIG_MEMORY_HOTREMOVE.
> OK, then I suggest moving it under CONFIG_MEMORY_HOTREMOVE guards rather
> than CONFIG_MEMORY_HOTPLUG.
I can't agree more ;-) 
I also find that page_isolation.c selected by MEMORY_ISOLATION under MEMORY_HOTPLUG
is also such case, I fix it by the way.

thanks,
linfeng
>

Patch

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 07d6966..b539015 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1317,7 +1317,7 @@  vmemmap_populate(struct page *start_page, unsigned long size, int node)
 	return 0;
 }
 
-#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HAVE_BOOTMEM_INFO_NODE)
 void register_page_bootmem_memmap(unsigned long section_nr,
 				  struct page *start_page, unsigned long size)
 {
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index f60e728..69903cc 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -174,7 +174,13 @@  static inline void arch_refresh_nodedata(int nid, pg_data_t *pgdat)
 #endif /* CONFIG_NUMA */
 #endif /* CONFIG_HAVE_ARCH_NODEDATA_EXTENSION */
 
+#ifdef CONFIG_HAVE_BOOTMEM_INFO_NODE
 extern void register_page_bootmem_info_node(struct pglist_data *pgdat);
+#else
+static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
+{
+}
+#endif
 extern void put_page_bootmem(struct page *page);
 extern void get_page_bootmem(unsigned long ingo, struct page *page,
 			     unsigned long type);
diff --git a/mm/Kconfig b/mm/Kconfig
index 278e3ab..f8c5799 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -162,10 +162,18 @@  config MOVABLE_NODE
 	  Say Y here if you want to hotplug a whole node.
 	  Say N here if you want kernel to use memory on all nodes evenly.
 
+#
+# Only be set on architectures that have completely implemented memory hotplug
+# feature. If you are not sure, don't touch it.
+#
+config HAVE_BOOTMEM_INFO_NODE
+	def_bool n
+
 # eventually, we can have this option just 'select SPARSEMEM'
 config MEMORY_HOTPLUG
 	bool "Allow for memory hot-add"
 	select MEMORY_ISOLATION
+	select HAVE_BOOTMEM_INFO_NODE if X86_64
 	depends on SPARSEMEM || X86_64_ACPI_NUMA
 	depends on HOTPLUG && ARCH_ENABLE_MEMORY_HOTPLUG
 	depends on (IA64 || X86 || PPC_BOOK3S_64 || SUPERH || S390)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 8aa2b56..daf111f 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -189,6 +189,7 @@  static void register_page_bootmem_info_section(unsigned long start_pfn)
 }
 #endif
 
+#ifdef CONFIG_HAVE_BOOTMEM_INFO_NODE
 void register_page_bootmem_info_node(struct pglist_data *pgdat)
 {
 	unsigned long i, pfn, end_pfn, nr_pages;
@@ -230,6 +231,7 @@  void register_page_bootmem_info_node(struct pglist_data *pgdat)
 			register_page_bootmem_info_section(pfn);
 	}
 }
+#endif
 
 static void grow_zone_span(struct zone *zone, unsigned long start_pfn,
 			   unsigned long end_pfn)