mm: make __paginginit based on CONFIG_MEMORY_HOTPLUG

Commit Message

Oscar Salvador July 31, 2018, 12:45 p.m. UTC

From: Oscar Salvador <[e-mail]>

__pagininit macro is being used to mark functions for:

a) Functions that we do not need to keep once the system is fully
   initialized with regard to memory.
b) Functions that will be needed for the memory-hotplug code,
   and because of that we need to keep them after initialization.

Right now, the condition to choose between one or the other is based on
CONFIG_SPARSEMEM, but I think that this should be changed to be based
on CONFIG_MEMORY_HOTPLUG.

The reason behind this is that it can very well be that we have CONFIG_SPARSEMEM
enabled, but not CONFIG_MEMORY_HOTPLUG, and thus, we will not need the
functions marked as __paginginit to stay around, since no
memory-hotplug code will call them.

Although the amount of freed bytes is not that big, I think it will
become more clear what __paginginit is used for.

Signed-off-by: Oscar Salvador <[e-mail]>
---
 mm/internal.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Pavel Tatashin July 31, 2018, 12:49 p.m. UTC | #1

Hi Oscar,

Have you looked into replacing __paginginit via __meminit ? What is
the reason to keep both?

Thank you,
Pavel
On Tue, Jul 31, 2018 at 8:45 AM <[e-mail]> wrote:
>
> From: Oscar Salvador <[e-mail]>
>
> __pagininit macro is being used to mark functions for:
>
> a) Functions that we do not need to keep once the system is fully
>    initialized with regard to memory.
> b) Functions that will be needed for the memory-hotplug code,
>    and because of that we need to keep them after initialization.
>
> Right now, the condition to choose between one or the other is based on
> CONFIG_SPARSEMEM, but I think that this should be changed to be based
> on CONFIG_MEMORY_HOTPLUG.
>
> The reason behind this is that it can very well be that we have CONFIG_SPARSEMEM
> enabled, but not CONFIG_MEMORY_HOTPLUG, and thus, we will not need the
> functions marked as __paginginit to stay around, since no
> memory-hotplug code will call them.
>
> Although the amount of freed bytes is not that big, I think it will
> become more clear what __paginginit is used for.
>
> Signed-off-by: Oscar Salvador <[e-mail]>
> ---
>  mm/internal.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 33c22754d282..c9170b4f7699 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -392,10 +392,11 @@ static inline struct page *mem_map_next(struct page *iter,
>  /*
>   * FLATMEM and DISCONTIGMEM configurations use alloc_bootmem_node,
>   * so all functions starting at paging_init should be marked __init
> - * in those cases. SPARSEMEM, however, allows for memory hotplug,
> - * and alloc_bootmem_node is not used.
> + * in those cases.
> + * In case that MEMORY_HOTPLUG is enabled, we need to keep those
> + * functions around since they can be called when hot-adding memory.
>   */
> -#ifdef CONFIG_SPARSEMEM
> +#ifdef CONFIG_MEMORY_HOTPLUG
>  #define __paginginit __meminit
>  #else
>  #define __paginginit __init
> --
> 2.13.6
>

Michal Hocko July 31, 2018, 1:04 p.m. UTC | #2

On Tue 31-07-18 08:49:11, Pavel Tatashin wrote:
> Hi Oscar,
> 
> Have you looked into replacing __paginginit via __meminit ? What is
> the reason to keep both?

All these init variants make my head spin so reducing their number is
certainly a desirable thing to do. b5a0e01132943 has added this variant
so it might give a clue about the dependencies.

Oscar Salvador July 31, 2018, 1:17 p.m. UTC | #3

On Tue, Jul 31, 2018 at 03:04:34PM +0200, Michal Hocko wrote:
> On Tue 31-07-18 08:49:11, Pavel Tatashin wrote:
> > Hi Oscar,
> > 
> > Have you looked into replacing __paginginit via __meminit ? What is
> > the reason to keep both?
> 
> All these init variants make my head spin so reducing their number is
> certainly a desirable thing to do. b5a0e01132943 has added this variant
> so it might give a clue about the dependencies.

Looking at b5a0e011329431b90d315eaf6ca5fdb41df7a117, I cannot really see why
this was not done in init.h
Maybe the comitter did not want to hack directly into __meminit.

I think that __paginginit was a way to abstract the whole thing without having
to modify init.h directly.

I guess we could get rid of it and so something like:

#ifdef CONFIG_MEMORY_HOTPLUG
 #define __meminit        __section(.meminit.text) __cold notrace \
                                                  __latent_entropy
#else
#define __meminit       __init
#endif

And then we would have to replace __paginginit with __meminit.

But honestly, puting an #ifdef in init.h feels a bit wierd to me,
although I do not really have a strong opinion here.

Thanks

Oscar Salvador July 31, 2018, 2:41 p.m. UTC | #4

On Tue, Jul 31, 2018 at 08:49:11AM -0400, Pavel Tatashin wrote:
> Hi Oscar,
> 
> Have you looked into replacing __paginginit via __meminit ? What is
> the reason to keep both?
Hi Pavel,

Actually, thinking a bit more about this, it might make sense to remove
__paginginit altogether and keep only __meminit.
Looking at the original commit, I think that it was put as a way to abstract it.

After the patchset [1] has been applied, only two functions marked as __paginginit
remain, so it will be less hassle to replace that with __meminit.

I will send a v2 tomorrow to be applied on top of [1].

[1] https://patchwork.kernel.org/patch/10548861/

Thanks

Pavel Tatashin July 31, 2018, 2:43 p.m. UTC | #5

Hi Oscar,

There is a simpler way. I will send it out in a bit. No need for your
first function, only  setup_usemap() needs to be changed to __ref.

Pavel
On Tue, Jul 31, 2018 at 10:42 AM Oscar Salvador
<[e-mail]> wrote:
>
> On Tue, Jul 31, 2018 at 08:49:11AM -0400, Pavel Tatashin wrote:
> > Hi Oscar,
> >
> > Have you looked into replacing __paginginit via __meminit ? What is
> > the reason to keep both?
> Hi Pavel,
>
> Actually, thinking a bit more about this, it might make sense to remove
> __paginginit altogether and keep only __meminit.
> Looking at the original commit, I think that it was put as a way to abstract it.
>
> After the patchset [1] has been applied, only two functions marked as __paginginit
> remain, so it will be less hassle to replace that with __meminit.
>
> I will send a v2 tomorrow to be applied on top of [1].
>
> [1] https://patchwork.kernel.org/patch/10548861/
>
> Thanks
> --
> Oscar Salvador
> SUSE L3
>

Pavel Tatashin July 31, 2018, 2:43 p.m. UTC | #6

On Tue, Jul 31, 2018 at 10:43 AM Pavel Tatashin
<[e-mail]> wrote:
>
> Hi Oscar,
>
> There is a simpler way. I will send it out in a bit. No need for your first function, only  setup_usemap() needs to be changed to __ref.

I meant first patch  not function :)

>
> Pavel
> On Tue, Jul 31, 2018 at 10:42 AM Oscar Salvador <[e-mail]> wrote:
> >
> > On Tue, Jul 31, 2018 at 08:49:11AM -0400, Pavel Tatashin wrote:
> > > Hi Oscar,
> > >
> > > Have you looked into replacing __paginginit via __meminit ? What is
> > > the reason to keep both?
> > Hi Pavel,
> >
> > Actually, thinking a bit more about this, it might make sense to remove
> > __paginginit altogether and keep only __meminit.
> > Looking at the original commit, I think that it was put as a way to abstract it.
> >
> > After the patchset [1] has been applied, only two functions marked as __paginginit
> > remain, so it will be less hassle to replace that with __meminit.
> >
> > I will send a v2 tomorrow to be applied on top of [1].
> >
> > [1] https://patchwork.kernel.org/patch/10548861/
> >
> > Thanks
> > --
> > Oscar Salvador
> > SUSE L3
> >

Pavel Tatashin July 31, 2018, 2:45 p.m. UTC | #7

On 18-07-31 16:41:57, Oscar Salvador wrote:
> On Tue, Jul 31, 2018 at 08:49:11AM -0400, Pavel Tatashin wrote:
> > Hi Oscar,
> > 
> > Have you looked into replacing __paginginit via __meminit ? What is
> > the reason to keep both?
> Hi Pavel,
> 
> Actually, thinking a bit more about this, it might make sense to remove
> __paginginit altogether and keep only __meminit.
> Looking at the original commit, I think that it was put as a way to abstract it.
> 
> After the patchset [1] has been applied, only two functions marked as __paginginit
> remain, so it will be less hassle to replace that with __meminit.
> 
> I will send a v2 tomorrow to be applied on top of [1].
> 
> [1] https://patchwork.kernel.org/patch/10548861/
> 
> Thanks
> -- 
> Oscar Salvador
> SUSE L3
> 

Here the patch would look like this:

From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
From: Pavel Tatashin <[e-mail]>
Date: Tue, 31 Jul 2018 10:37:44 -0400
Subject: [PATCH] mm: remove __paginginit

__paginginit is the same thing as __meminit except for platforms without
sparsemem, there it is defined as __init.

Remove __paginginit and use __meminit. Use __ref in one single function
that merges __meminit and __init sections: setup_usemap().

Signed-off-by: Pavel Tatashin <[e-mail]>
---
 mm/internal.h   | 12 ------------
 mm/page_alloc.c | 19 ++++++++++---------
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 33c22754d282..87256ae1bef8 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -389,18 +389,6 @@ static inline struct page *mem_map_next(struct page *iter,
 	return iter + 1;
 }
 
-/*
- * FLATMEM and DISCONTIGMEM configurations use alloc_bootmem_node,
- * so all functions starting at paging_init should be marked __init
- * in those cases. SPARSEMEM, however, allows for memory hotplug,
- * and alloc_bootmem_node is not used.
- */
-#ifdef CONFIG_SPARSEMEM
-#define __paginginit __meminit
-#else
-#define __paginginit __init
-#endif
-
 /* Memory initialisation debug and verification */
 enum mminit_level {
 	MMINIT_WARNING,
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 02e4b84038f8..92abe3eb151d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6122,7 +6122,7 @@ static unsigned long __init usemap_size(unsigned long zone_start_pfn, unsigned l
 	return usemapsize / 8;
 }
 
-static void __init setup_usemap(struct pglist_data *pgdat,
+static void __ref setup_usemap(struct pglist_data *pgdat,
 				struct zone *zone,
 				unsigned long zone_start_pfn,
 				unsigned long zonesize)
@@ -6142,7 +6142,7 @@ static inline void setup_usemap(struct pglist_data *pgdat, struct zone *zone,
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE
 
 /* Initialise the number of pages represented by NR_PAGEBLOCK_BITS */
-void __paginginit set_pageblock_order(void)
+void __meminit set_pageblock_order(void)
 {
 	unsigned int order;
 
@@ -6170,14 +6170,14 @@ void __paginginit set_pageblock_order(void)
  * include/linux/pageblock-flags.h for the values of pageblock_order based on
  * the kernel config
  */
-void __paginginit set_pageblock_order(void)
+void __meminit set_pageblock_order(void)
 {
 }
 
 #endif /* CONFIG_HUGETLB_PAGE_SIZE_VARIABLE */
 
-static unsigned long __paginginit calc_memmap_size(unsigned long spanned_pages,
-						   unsigned long present_pages)
+static unsigned long __meminit calc_memmap_size(unsigned long spanned_pages,
+						unsigned long present_pages)
 {
 	unsigned long pages = spanned_pages;
 
@@ -6204,7 +6204,7 @@ static unsigned long __paginginit calc_memmap_size(unsigned long spanned_pages,
  *
  * NOTE: pgdat should get zeroed by caller.
  */
-static void __paginginit free_area_init_core(struct pglist_data *pgdat)
+static void __meminit free_area_init_core(struct pglist_data *pgdat)
 {
 	enum zone_type j;
 	int nid = pgdat->node_id;
@@ -6344,8 +6344,9 @@ static void __ref alloc_node_mem_map(struct pglist_data *pgdat)
 static void __ref alloc_node_mem_map(struct pglist_data *pgdat) { }
 #endif /* CONFIG_FLAT_NODE_MEM_MAP */
 
-void __paginginit free_area_init_node(int nid, unsigned long *zones_size,
-		unsigned long node_start_pfn, unsigned long *zholes_size)
+void __meminit free_area_init_node(int nid, unsigned long *zones_size,
+				   unsigned long node_start_pfn,
+				   unsigned long *zholes_size)
 {
 	pg_data_t *pgdat = NODE_DATA(nid);
 	unsigned long start_pfn = 0;
@@ -6390,7 +6391,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size,
  * may be accessed (for example page_to_pfn() on some configuration accesses
  * flags). We must explicitly zero those struct pages.
  */
-void __paginginit zero_resv_unavail(void)
+void __meminit zero_resv_unavail(void)
 {
 	phys_addr_t start, end;
 	unsigned long pfn;

Oscar Salvador July 31, 2018, 2:51 p.m. UTC | #8

On Tue, Jul 31, 2018 at 10:45:45AM -0400, Pavel Tatashin wrote:
> Here the patch would look like this:
> 
> From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
> From: Pavel Tatashin <[e-mail]>
> Date: Tue, 31 Jul 2018 10:37:44 -0400
> Subject: [PATCH] mm: remove __paginginit
> 
> __paginginit is the same thing as __meminit except for platforms without
> sparsemem, there it is defined as __init.
> 
> Remove __paginginit and use __meminit. Use __ref in one single function
> that merges __meminit and __init sections: setup_usemap().
> 
> Signed-off-by: Pavel Tatashin <[e-mail]>

Uhm, I am probably missing something, but with this change, the functions will not be freed up
while freeing init memory, right?

Thanks

Pavel Tatashin July 31, 2018, 2:53 p.m. UTC | #9

Thats correct on arches where no sparsemem setup_usemap() will not be
freed up. It is a tiny function, just a few instructions. Not a big
deal.

Pavel
On Tue, Jul 31, 2018 at 10:51 AM Oscar Salvador
<[e-mail]> wrote:
>
> On Tue, Jul 31, 2018 at 10:45:45AM -0400, Pavel Tatashin wrote:
> > Here the patch would look like this:
> >
> > From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
> > From: Pavel Tatashin <[e-mail]>
> > Date: Tue, 31 Jul 2018 10:37:44 -0400
> > Subject: [PATCH] mm: remove __paginginit
> >
> > __paginginit is the same thing as __meminit except for platforms without
> > sparsemem, there it is defined as __init.
> >
> > Remove __paginginit and use __meminit. Use __ref in one single function
> > that merges __meminit and __init sections: setup_usemap().
> >
> > Signed-off-by: Pavel Tatashin <[e-mail]>
>
> Uhm, I am probably missing something, but with this change, the functions will not be freed up
> while freeing init memory, right?
>
> Thanks
> --
> Oscar Salvador
> SUSE L3
>

Oscar Salvador July 31, 2018, 3:01 p.m. UTC | #10

On Tue, Jul 31, 2018 at 10:53:52AM -0400, Pavel Tatashin wrote:
> Thats correct on arches where no sparsemem setup_usemap() will not be
> freed up. It is a tiny function, just a few instructions. Not a big
> deal.
> 
> Pavel
> On Tue, Jul 31, 2018 at 10:51 AM Oscar Salvador
> <[e-mail]> wrote:
> >
> > On Tue, Jul 31, 2018 at 10:45:45AM -0400, Pavel Tatashin wrote:
> > > Here the patch would look like this:
> > >
> > > From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
> > > From: Pavel Tatashin <[e-mail]>
> > > Date: Tue, 31 Jul 2018 10:37:44 -0400
> > > Subject: [PATCH] mm: remove __paginginit
> > >
> > > __paginginit is the same thing as __meminit except for platforms without
> > > sparsemem, there it is defined as __init.
> > >
> > > Remove __paginginit and use __meminit. Use __ref in one single function
> > > that merges __meminit and __init sections: setup_usemap().
> > >
> > > Signed-off-by: Pavel Tatashin <[e-mail]>
> >
> > Uhm, I am probably missing something, but with this change, the functions will not be freed up
> > while freeing init memory, right?
> Thats correct on arches where no sparsemem setup_usemap() will not be
> freed up. It is a tiny function, just a few instructions. Not a big
> deal.

I must be missing something.

What about:

calc_memmap_size
free_area_init_node
free_area_init_core
 
These functions are marked with __meminit now.
If we have CONFIG_PARSEMEM but not CONFIG_MEMORY_HOTPLUG, these functions will
be left there.

I mean, it is not that it is a big amount, but still.

Do not we need something like:

diff --git a/include/linux/init.h b/include/linux/init.h
index 2538d176dd1f..3b3a88ba80ed 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -83,8 +83,12 @@
 #define __exit          __section(.exit.text) __exitused __cold notrace
 
 /* Used for MEMORY_HOTPLUG */
+#ifdef CONFIG_MEMORY_HOTPLUG
 #define __meminit        __section(.meminit.text) __cold notrace \
 						  __latent_entropy
+#else
+#define __meminit	 __init
+#endif
 #define __meminitdata    __section(.meminit.data)
 #define __meminitconst   __section(.meminit.rodata)
 #define __memexit        __section(.memexit.text) __exitused __cold notrace

on top?

Thanks

Pavel Tatashin July 31, 2018, 3:06 p.m. UTC | #11

On Tue, Jul 31, 2018 at 11:01 AM Oscar Salvador
<[e-mail]> wrote:
>
> On Tue, Jul 31, 2018 at 10:53:52AM -0400, Pavel Tatashin wrote:
> > Thats correct on arches where no sparsemem setup_usemap() will not be
> > freed up. It is a tiny function, just a few instructions. Not a big
> > deal.
> >
> > Pavel
> > On Tue, Jul 31, 2018 at 10:51 AM Oscar Salvador
> > <[e-mail]> wrote:
> > >
> > > On Tue, Jul 31, 2018 at 10:45:45AM -0400, Pavel Tatashin wrote:
> > > > Here the patch would look like this:
> > > >
> > > > From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
> > > > From: Pavel Tatashin <[e-mail]>
> > > > Date: Tue, 31 Jul 2018 10:37:44 -0400
> > > > Subject: [PATCH] mm: remove __paginginit
> > > >
> > > > __paginginit is the same thing as __meminit except for platforms without
> > > > sparsemem, there it is defined as __init.
> > > >
> > > > Remove __paginginit and use __meminit. Use __ref in one single function
> > > > that merges __meminit and __init sections: setup_usemap().
> > > >
> > > > Signed-off-by: Pavel Tatashin <[e-mail]>
> > >
> > > Uhm, I am probably missing something, but with this change, the functions will not be freed up
> > > while freeing init memory, right?
> > Thats correct on arches where no sparsemem setup_usemap() will not be
> > freed up. It is a tiny function, just a few instructions. Not a big
> > deal.
>
> I must be missing something.
>
> What about:
>
> calc_memmap_size
> free_area_init_node
> free_area_init_core
>
> These functions are marked with __meminit now.
> If we have CONFIG_PARSEMEM but not CONFIG_MEMORY_HOTPLUG, these functions will
> be left there.

I hope we free meminit section if no hotplug configured. If not, than
sure we should have something like what you suggest not only for these
functions, but for all other meminit functions in kernel.

>
> I mean, it is not that it is a big amount, but still.
>
> Do not we need something like:
>
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 2538d176dd1f..3b3a88ba80ed 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -83,8 +83,12 @@
>  #define __exit          __section(.exit.text) __exitused __cold notrace
>
>  /* Used for MEMORY_HOTPLUG */
> +#ifdef CONFIG_MEMORY_HOTPLUG
>  #define __meminit        __section(.meminit.text) __cold notrace \
>                                                   __latent_entropy
> +#else
> +#define __meminit       __init
> +#endif
>  #define __meminitdata    __section(.meminit.data)
>  #define __meminitconst   __section(.meminit.rodata)
>  #define __memexit        __section(.memexit.text) __exitused __cold notrace
>
> on top?
>
> Thanks
> --
> Oscar Salvador
> SUSE L3
>

Pavel Tatashin July 31, 2018, 3:23 p.m. UTC | #12

Yes we free meminit when no CONFIG_MEMORY_HOTPLUG
See here:
http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#107

Pavel
On Tue, Jul 31, 2018 at 11:06 AM Pavel Tatashin
<[e-mail]> wrote:
>
> On Tue, Jul 31, 2018 at 11:01 AM Oscar Salvador
> <[e-mail]> wrote:
> >
> > On Tue, Jul 31, 2018 at 10:53:52AM -0400, Pavel Tatashin wrote:
> > > Thats correct on arches where no sparsemem setup_usemap() will not be
> > > freed up. It is a tiny function, just a few instructions. Not a big
> > > deal.
> > >
> > > Pavel
> > > On Tue, Jul 31, 2018 at 10:51 AM Oscar Salvador
> > > <[e-mail]> wrote:
> > > >
> > > > On Tue, Jul 31, 2018 at 10:45:45AM -0400, Pavel Tatashin wrote:
> > > > > Here the patch would look like this:
> > > > >
> > > > > From e640b32dbd329bba5a785cc60050d5d7e1ca18ce Mon Sep 17 00:00:00 2001
> > > > > From: Pavel Tatashin <[e-mail]>
> > > > > Date: Tue, 31 Jul 2018 10:37:44 -0400
> > > > > Subject: [PATCH] mm: remove __paginginit
> > > > >
> > > > > __paginginit is the same thing as __meminit except for platforms without
> > > > > sparsemem, there it is defined as __init.
> > > > >
> > > > > Remove __paginginit and use __meminit. Use __ref in one single function
> > > > > that merges __meminit and __init sections: setup_usemap().
> > > > >
> > > > > Signed-off-by: Pavel Tatashin <[e-mail]>
> > > >
> > > > Uhm, I am probably missing something, but with this change, the functions will not be freed up
> > > > while freeing init memory, right?
> > > Thats correct on arches where no sparsemem setup_usemap() will not be
> > > freed up. It is a tiny function, just a few instructions. Not a big
> > > deal.
> >
> > I must be missing something.
> >
> > What about:
> >
> > calc_memmap_size
> > free_area_init_node
> > free_area_init_core
> >
> > These functions are marked with __meminit now.
> > If we have CONFIG_PARSEMEM but not CONFIG_MEMORY_HOTPLUG, these functions will
> > be left there.
>
> I hope we free meminit section if no hotplug configured. If not, than
> sure we should have something like what you suggest not only for these
> functions, but for all other meminit functions in kernel.
>
> >
> > I mean, it is not that it is a big amount, but still.
> >
> > Do not we need something like:
> >
> > diff --git a/include/linux/init.h b/include/linux/init.h
> > index 2538d176dd1f..3b3a88ba80ed 100644
> > --- a/include/linux/init.h
> > +++ b/include/linux/init.h
> > @@ -83,8 +83,12 @@
> >  #define __exit          __section(.exit.text) __exitused __cold notrace
> >
> >  /* Used for MEMORY_HOTPLUG */
> > +#ifdef CONFIG_MEMORY_HOTPLUG
> >  #define __meminit        __section(.meminit.text) __cold notrace \
> >                                                   __latent_entropy
> > +#else
> > +#define __meminit       __init
> > +#endif
> >  #define __meminitdata    __section(.meminit.data)
> >  #define __meminitconst   __section(.meminit.rodata)
> >  #define __memexit        __section(.memexit.text) __exitused __cold notrace
> >
> > on top?
> >
> > Thanks
> > --
> > Oscar Salvador
> > SUSE L3
> >

Oscar Salvador July 31, 2018, 8:50 p.m. UTC | #13

On Tue, Jul 31, 2018 at 11:23:33AM -0400, Pavel Tatashin wrote:
> Yes we free meminit when no CONFIG_MEMORY_HOTPLUG
> See here:
> http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#107

Oh, I got the point now.
Somehow I missed that we were freeing up the memory when CONFIG_MEMORY_HOTPLUG
was not in place.

So your patch makes sense to me now, sorry.

Since my patchset [1] + cleanup patch [2] remove almost all __paginginit,
leaving only pgdat_init_internals() and zone_init_internals(), I think
it would be great if you base your patch on top of that.
Or since the patchset has some cleanups already, I could add your patch
into it (as we did for the zone_to/set_nid() patch) and send a v6 with it.

What do you think?

[1] https://patchwork.kernel.org/patch/10548861/
[2] <[e-mail]>

Thanks

Pavel Tatashin July 31, 2018, 9:33 p.m. UTC | #14

On 07/31/2018 04:50 PM, Oscar Salvador wrote:
> On Tue, Jul 31, 2018 at 11:23:33AM -0400, Pavel Tatashin wrote:
>> Yes we free meminit when no CONFIG_MEMORY_HOTPLUG
>> See here:
>> http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#107
> 
> Oh, I got the point now.
> Somehow I missed that we were freeing up the memory when CONFIG_MEMORY_HOTPLUG
> was not in place.
> 
> So your patch makes sense to me now, sorry.
> 
> Since my patchset [1] + cleanup patch [2] remove almost all __paginginit,
> leaving only pgdat_init_internals() and zone_init_internals(), I think
> it would be great if you base your patch on top of that.
> Or since the patchset has some cleanups already, I could add your patch
> into it (as we did for the zone_to/set_nid() patch) and send a v6 with it.
> 
> What do you think?

Sure, please go ahead include it in v6. Let me know if you need any help. Thank you for this work, I really like how this improves hotplug/memory-init code.

Pavel

> 
> [1] https://patchwork.kernel.org/patch/10548861/
> [2] <[e-mail]>
> 
> Thanks
>

Patch

diff --git a/mm/internal.h b/mm/internal.h
index 33c22754d282..c9170b4f7699 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -392,10 +392,11 @@  static inline struct page *mem_map_next(struct page *iter,
 /*
  * FLATMEM and DISCONTIGMEM configurations use alloc_bootmem_node,
  * so all functions starting at paging_init should be marked __init
- * in those cases. SPARSEMEM, however, allows for memory hotplug,
- * and alloc_bootmem_node is not used.
+ * in those cases.
+ * In case that MEMORY_HOTPLUG is enabled, we need to keep those
+ * functions around since they can be called when hot-adding memory.
  */
-#ifdef CONFIG_SPARSEMEM
+#ifdef CONFIG_MEMORY_HOTPLUG
 #define __paginginit __meminit
 #else
 #define __paginginit __init