[PATCH 1/3] mm: use zonelist_zone() to get zone

[PATCH 1/3] mm: use zonelist_zone() to get zone

[Wei Yang]

Instead of accessing zoneref->zone directly, use zonelist_zone() like
other places for consistency.

No functional change.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport (IBM) <rppt@kernel.org>
---
 include/linux/mmzone.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index cb7f265c2b96..a34a74f5b113 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1690,7 +1690,7 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
 			zone = zonelist_zone(z))
 
 #define for_next_zone_zonelist_nodemask(zone, z, highidx, nodemask) \
-	for (zone = z->zone;	\
+	for (zone = zonelist_zone(z);	\
 		zone;							\
 		z = next_zones_zonelist(++z, highidx, nodemask),	\
 			zone = zonelist_zone(z))
-- 
2.34.1

[PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Wei Yang]

.meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
set, since we define MEM_KEEP()/MEM_DISCARD() according to
CONFIG_MEMORY_HOTPLUG.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport (IBM) <rppt@kernel.org>
---
 scripts/mod/modpost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f48d72d22dc2..81134403d4d7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -22,6 +22,7 @@
 #include <errno.h>
 #include "modpost.h"
 #include "../../include/linux/license.h"
+#include "../../include/generated/autoconf.h"
 
 static bool module_enabled;
 /* Are we using CONFIG_MODVERSIONS? */
@@ -775,9 +776,14 @@ static void check_section(const char *modname, struct elf_info *elf,
 
 
 
+#if defined(CONFIG_MEMORY_HOTPLUG)
+#define ALL_INIT_DATA_SECTIONS \
+	".init.setup", ".init.rodata", ".init.data"
+#else
 #define ALL_INIT_DATA_SECTIONS \
 	".init.setup", ".init.rodata", ".meminit.rodata", \
 	".init.data", ".meminit.data"
+#endif
 
 #define ALL_PCI_INIT_SECTIONS	\
 	".pci_fixup_early", ".pci_fixup_header", ".pci_fixup_final", \
@@ -786,7 +792,11 @@ static void check_section(const char *modname, struct elf_info *elf,
 
 #define ALL_XXXINIT_SECTIONS ".meminit.*"
 
+#if defined(CONFIG_MEMORY_HOTPLUG)
+#define ALL_INIT_SECTIONS INIT_SECTIONS
+#else
 #define ALL_INIT_SECTIONS INIT_SECTIONS, ALL_XXXINIT_SECTIONS
+#endif
 #define ALL_EXIT_SECTIONS ".exit.*"
 
 #define DATA_SECTIONS ".data", ".data.rel"
-- 
2.34.1

[PATCH 3/3] mm/page_alloc: put __free_pages_core() in __meminit section

[Wei Yang]

Function __free_pages_core() is only used in bootmem init and hot-add
memory init path. Let's put it in __meminit section.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport (IBM) <rppt@kernel.org>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 480e4416131f..c46aedfc9a12 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1219,7 +1219,7 @@ static void __free_pages_ok(struct page *page, unsigned int order,
 	__count_vm_events(PGFREE, 1 << order);
 }
 
-void __free_pages_core(struct page *page, unsigned int order,
+void __meminit __free_pages_core(struct page *page, unsigned int order,
 		enum meminit_context context)
 {
 	unsigned int nr_pages = 1 << order;
-- 
2.34.1

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Andrew Morton]

On Tue,  2 Jul 2024 23:40:07 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
> set, since we define MEM_KEEP()/MEM_DISCARD() according to
> CONFIG_MEMORY_HOTPLUG.

Please describe how this changes modpost behaviour.

Something like: "we're currently not checking for references into
meminit and meminitdata when CONFIG_HOTPLUG=y, which may cause us to
fail to notice incorrect references.".  But I don't think that's
correct.  So what *is* wrong with the current code?

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Masahiro Yamada]

On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
>
> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
> set, since we define MEM_KEEP()/MEM_DISCARD() according to
> CONFIG_MEMORY_HOTPLUG.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Mike Rapoport (IBM) <rppt@kernel.org>
> ---
>  scripts/mod/modpost.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)



NACK.


The section mismatch is performed _unconditionally_.



In the old days, we did this depending on relevant CONFIG options.
It was more than 15 years ago that we stopped doing that.


See this:


commit eb8f689046b857874e964463619f09df06d59fad
Author: Sam Ravnborg <sam@ravnborg.org>
Date:   Sun Jan 20 20:07:28 2008 +0100

    Use separate sections for __dev/__cpu/__mem code/data




So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h

That is what we did in the Linux 2.6.* era, which had much worse
section mismatch coverage.










-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Masahiro Yamada]

On Wed, Jul 3, 2024 at 10:52 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue,  2 Jul 2024 23:40:07 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>
> > .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
> > set, since we define MEM_KEEP()/MEM_DISCARD() according to
> > CONFIG_MEMORY_HOTPLUG.
>
> Please describe how this changes modpost behaviour.
>
> Something like: "we're currently not checking for references into
> meminit and meminitdata when CONFIG_HOTPLUG=y, which may cause us to
> fail to notice incorrect references.".  But I don't think that's
> correct.  So what *is* wrong with the current code?
>


Sigh.

If you do not understand, you should not apply it.

I am surprised that there exists a person who
attempted to apply this.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Wei Yang]

On Wed, Jul 03, 2024 at 11:44:38PM +0900, Masahiro Yamada wrote:
>On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
>> set, since we define MEM_KEEP()/MEM_DISCARD() according to
>> CONFIG_MEMORY_HOTPLUG.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> ---
>>  scripts/mod/modpost.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>
>
>
>NACK.
>
>
>The section mismatch is performed _unconditionally_.
>
>
>
>In the old days, we did this depending on relevant CONFIG options.
>It was more than 15 years ago that we stopped doing that.
>
>
>See this:
>
>
>commit eb8f689046b857874e964463619f09df06d59fad
>Author: Sam Ravnborg <sam@ravnborg.org>
>Date:   Sun Jan 20 20:07:28 2008 +0100
>
>    Use separate sections for __dev/__cpu/__mem code/data
>
>
>
>
>So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
>you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h
>

You mean something like this?

diff --git a/include/linux/init.h b/include/linux/init.h
index 58cef4c2e59a..388f0a4c34e9 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -85,10 +85,12 @@
 #define __exit          __section(".exit.text") __exitused __cold notrace
 
 /* Used for MEMORY_HOTPLUG */
+#ifndef CONFIG_MEMORY_HOTPLUG
 #define __meminit        __section(".meminit.text") __cold notrace \
 						  __latent_entropy
 #define __meminitdata    __section(".meminit.data")
 #define __meminitconst   __section(".meminit.rodata")
+#endif
 
 /* For assembly routines */
 #define __HEAD		.section	".head.text","ax"

>That is what we did in the Linux 2.6.* era, which had much worse
>section mismatch coverage.
>

I guess you mean this is not a good practice.

Then I am confused how we do the mismatch check unconditionally?

After commit 

commit eb8f689046b857874e964463619f09df06d59fad
Author: Sam Ravnborg <sam@ravnborg.org>
Date:   Sun Jan 20 20:07:28 2008 +0100

    Use separate sections for __dev/__cpu/__mem code/data

Sections .meminit.* will be put into INIT_SECTION conditionally, but we always
do the mismatch check unconditionally. It will report mismatch when .meminit.*
is not in INIT_SECTION. It looks not correct to me.

Maybe I am not fully understand your message. Would you mind explaining more
on what is the correct way to do?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Wei Yang]

On Wed, Jul 03, 2024 at 11:44:38PM +0900, Masahiro Yamada wrote:
>On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
>> set, since we define MEM_KEEP()/MEM_DISCARD() according to
>> CONFIG_MEMORY_HOTPLUG.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> ---
>>  scripts/mod/modpost.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>
>
>
>NACK.
>
>
>The section mismatch is performed _unconditionally_.
>
>
>
>In the old days, we did this depending on relevant CONFIG options.
>It was more than 15 years ago that we stopped doing that.
>
>
>See this:
>
>
>commit eb8f689046b857874e964463619f09df06d59fad
>Author: Sam Ravnborg <sam@ravnborg.org>
>Date:   Sun Jan 20 20:07:28 2008 +0100
>
>    Use separate sections for __dev/__cpu/__mem code/data
>
>
>
>
>So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
>you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h
>
>That is what we did in the Linux 2.6.* era, which had much worse
>section mismatch coverage.
>

Masahiro 

If you would give me some suggestions, I'd appreciate it a lot.

The original thing I want to do is to put function __free_pages_core() in
__meminit section, since this function is only used by __init functions and
in memory_hotplug.c.  This means it is save to release it if
CONFIG_MEMORY_HOTPLUG is set.

Then I add __meminit to function __free_pages_core() and face the warning from
modpost.

  WARNING: modpost: vmlinux: section mismatch in reference: generic_online_page+0xa (section: .text) -> __free_pages_core (section: .meminit.text)

A .text function calls init code is not proper. Then I add __meminit to
generic_online_page too. Then I face this warning from modpost.

  WARNING: modpost: vmlinux: generic_online_page: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.

Function generic_online_page() is exported and be used in some modules. And is
not allowed to use init tag.

When looking into the code, this warning is printed by this code:

   #define ALL_INIT_SECTIONS INIT_SECTIONS, ALL_XXXINIT_SECTIONS
   
   if (match(secname, PATTERNS(ALL_INIT_SECTIONS)))
   	warn("%s: %s: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.\n",
   	     mod->name, name);

If my understanding is correct, the code means we can't use a function in init
section, since the code will be released after bootup.

But for this case, when CONFIG_MEMORY_HOTPLUG is set, the section .meminit.*
is not put into the real init sections. So the functions are not released and
could be used by modules. This behavior is introduced in commit
eb8f689046b8(the one you mentioned above).

So the check here is not proper to me. We should exclude .meminit.* from
ALL_INIT_SECTIONS.

Looking forward your suggestion and a proper way to handle this.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm: use zonelist_zone() to get zone

[David Hildenbrand]

On 03.07.24 01:40, Wei Yang wrote:
> Instead of accessing zoneref->zone directly, use zonelist_zone() like
> other places for consistency.
> 
> No functional change.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Mike Rapoport (IBM) <rppt@kernel.org>
> ---
>   include/linux/mmzone.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index cb7f265c2b96..a34a74f5b113 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1690,7 +1690,7 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
>   			zone = zonelist_zone(z))
>   
>   #define for_next_zone_zonelist_nodemask(zone, z, highidx, nodemask) \
> -	for (zone = z->zone;	\
> +	for (zone = zonelist_zone(z);	\
>   		zone;							\
>   		z = next_zones_zonelist(++z, highidx, nodemask),	\
>   			zone = zonelist_zone(z))

Should we do the same in movable_only_nodes as well to be consistent in 
that file?

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/3] mm: use zonelist_zone() to get zone

[Wei Yang]

On Fri, Jul 05, 2024 at 11:03:03AM +0200, David Hildenbrand wrote:
>On 03.07.24 01:40, Wei Yang wrote:
>> Instead of accessing zoneref->zone directly, use zonelist_zone() like
>> other places for consistency.
>> 
>> No functional change.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> ---
>>   include/linux/mmzone.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index cb7f265c2b96..a34a74f5b113 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -1690,7 +1690,7 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
>>   			zone = zonelist_zone(z))
>>   #define for_next_zone_zonelist_nodemask(zone, z, highidx, nodemask) \
>> -	for (zone = z->zone;	\
>> +	for (zone = zonelist_zone(z);	\
>>   		zone;							\
>>   		z = next_zones_zonelist(++z, highidx, nodemask),	\
>>   			zone = zonelist_zone(z))
>
>Should we do the same in movable_only_nodes as well to be consistent in that
>file?
>

Agree, thanks

>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Wei Yang]

On Fri, Jul 05, 2024 at 06:54:56AM +0000, Wei Yang wrote:
>On Wed, Jul 03, 2024 at 11:44:38PM +0900, Masahiro Yamada wrote:
>>On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
>>>
>>> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
>>> set, since we define MEM_KEEP()/MEM_DISCARD() according to
>>> CONFIG_MEMORY_HOTPLUG.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>>> ---
>>>  scripts/mod/modpost.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>
>>
>>
>>NACK.
>>
>>
>>The section mismatch is performed _unconditionally_.
>>
>>
>>
>>In the old days, we did this depending on relevant CONFIG options.
>>It was more than 15 years ago that we stopped doing that.
>>
>>
>>See this:
>>
>>
>>commit eb8f689046b857874e964463619f09df06d59fad
>>Author: Sam Ravnborg <sam@ravnborg.org>
>>Date:   Sun Jan 20 20:07:28 2008 +0100
>>
>>    Use separate sections for __dev/__cpu/__mem code/data
>>
>>
>>
>>
>>So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
>>you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h
>>
>>That is what we did in the Linux 2.6.* era, which had much worse
>>section mismatch coverage.
>>
>
>Masahiro 
>
>If you would give me some suggestions, I'd appreciate it a lot.
>
>The original thing I want to do is to put function __free_pages_core() in
>__meminit section, since this function is only used by __init functions and
>in memory_hotplug.c.  This means it is save to release it if
>CONFIG_MEMORY_HOTPLUG is set.
>
>Then I add __meminit to function __free_pages_core() and face the warning from
>modpost.
>
>  WARNING: modpost: vmlinux: section mismatch in reference: generic_online_page+0xa (section: .text) -> __free_pages_core (section: .meminit.text)
>
>A .text function calls init code is not proper. Then I add __meminit to
>generic_online_page too. Then I face this warning from modpost.
>
>  WARNING: modpost: vmlinux: generic_online_page: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.
>

I guess I found the correct way.

Add __ref to generic_online_page to not issue a warning.

>Function generic_online_page() is exported and be used in some modules. And is
>not allowed to use init tag.
>
>When looking into the code, this warning is printed by this code:
>
>   #define ALL_INIT_SECTIONS INIT_SECTIONS, ALL_XXXINIT_SECTIONS
>   
>   if (match(secname, PATTERNS(ALL_INIT_SECTIONS)))
>   	warn("%s: %s: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.\n",
>   	     mod->name, name);
>
>If my understanding is correct, the code means we can't use a function in init
>section, since the code will be released after bootup.
>
>But for this case, when CONFIG_MEMORY_HOTPLUG is set, the section .meminit.*
>is not put into the real init sections. So the functions are not released and
>could be used by modules. This behavior is introduced in commit
>eb8f689046b8(the one you mentioned above).
>
>So the check here is not proper to me. We should exclude .meminit.* from
>ALL_INIT_SECTIONS.
>
>Looking forward your suggestion and a proper way to handle this.
>
>-- 
>Wei Yang
>Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Masahiro Yamada]

On Sat, Jul 6, 2024 at 3:12 PM Wei Yang <richard.weiyang@gmail.com> wrote:
>
> On Fri, Jul 05, 2024 at 06:54:56AM +0000, Wei Yang wrote:
> >On Wed, Jul 03, 2024 at 11:44:38PM +0900, Masahiro Yamada wrote:
> >>On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
> >>>
> >>> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
> >>> set, since we define MEM_KEEP()/MEM_DISCARD() according to
> >>> CONFIG_MEMORY_HOTPLUG.
> >>>
> >>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
> >>> ---
> >>>  scripts/mod/modpost.c | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>
> >>
> >>
> >>NACK.
> >>
> >>
> >>The section mismatch is performed _unconditionally_.
> >>
> >>
> >>
> >>In the old days, we did this depending on relevant CONFIG options.
> >>It was more than 15 years ago that we stopped doing that.
> >>
> >>
> >>See this:
> >>
> >>
> >>commit eb8f689046b857874e964463619f09df06d59fad
> >>Author: Sam Ravnborg <sam@ravnborg.org>
> >>Date:   Sun Jan 20 20:07:28 2008 +0100
> >>
> >>    Use separate sections for __dev/__cpu/__mem code/data
> >>
> >>
> >>
> >>
> >>So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
> >>you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h
> >>
> >>That is what we did in the Linux 2.6.* era, which had much worse
> >>section mismatch coverage.
> >>
> >
> >Masahiro
> >
> >If you would give me some suggestions, I'd appreciate it a lot.
> >
> >The original thing I want to do is to put function __free_pages_core() in
> >__meminit section, since this function is only used by __init functions and
> >in memory_hotplug.c.  This means it is save to release it if
> >CONFIG_MEMORY_HOTPLUG is set.
> >
> >Then I add __meminit to function __free_pages_core() and face the warning from
> >modpost.
> >
> >  WARNING: modpost: vmlinux: section mismatch in reference: generic_online_page+0xa (section: .text) -> __free_pages_core (section: .meminit.text)
> >
> >A .text function calls init code is not proper. Then I add __meminit to
> >generic_online_page too. Then I face this warning from modpost.
> >
> >  WARNING: modpost: vmlinux: generic_online_page: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.
> >
>
> I guess I found the correct way.
>
> Add __ref to generic_online_page to not issue a warning.



Yes, __ref is used to bypass the section mismatch check.

Some functions in mm/memory_hotplug.c are annotated as __ref
to reference __meminit functions.

Adding __ref is the easy solution.



Having said that, I started to think
eb8f689046b857874e964463619f09df06d59fad was the wrong decision.
I will revert it.











-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] modpost: .meminit.* is not in init section when CONFIG_MEMORY_HOTPLUG set

[Wei Yang]

On Sat, Jul 06, 2024 at 10:50:25PM +0900, Masahiro Yamada wrote:
>On Sat, Jul 6, 2024 at 3:12 PM Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>> On Fri, Jul 05, 2024 at 06:54:56AM +0000, Wei Yang wrote:
>> >On Wed, Jul 03, 2024 at 11:44:38PM +0900, Masahiro Yamada wrote:
>> >>On Wed, Jul 3, 2024 at 8:40 AM Wei Yang <richard.weiyang@gmail.com> wrote:
>> >>>
>> >>> .meminit.* is not put into init section when CONFIG_MEMORY_HOTPLUG is
>> >>> set, since we define MEM_KEEP()/MEM_DISCARD() according to
>> >>> CONFIG_MEMORY_HOTPLUG.
>> >>>
>> >>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> >>> ---
>> >>>  scripts/mod/modpost.c | 10 ++++++++++
>> >>>  1 file changed, 10 insertions(+)
>> >>
>> >>
>> >>
>> >>NACK.
>> >>
>> >>
>> >>The section mismatch is performed _unconditionally_.
>> >>
>> >>
>> >>
>> >>In the old days, we did this depending on relevant CONFIG options.
>> >>It was more than 15 years ago that we stopped doing that.
>> >>
>> >>
>> >>See this:
>> >>
>> >>
>> >>commit eb8f689046b857874e964463619f09df06d59fad
>> >>Author: Sam Ravnborg <sam@ravnborg.org>
>> >>Date:   Sun Jan 20 20:07:28 2008 +0100
>> >>
>> >>    Use separate sections for __dev/__cpu/__mem code/data
>> >>
>> >>
>> >>
>> >>
>> >>So, if you wanted to check this only when CONFIG_MEMORY_HOTPLUG=n,
>> >>you would need to add #ifdef CONFIG_MEMORY_HOTPLUG to include/linux/init.h
>> >>
>> >>That is what we did in the Linux 2.6.* era, which had much worse
>> >>section mismatch coverage.
>> >>
>> >
>> >Masahiro
>> >
>> >If you would give me some suggestions, I'd appreciate it a lot.
>> >
>> >The original thing I want to do is to put function __free_pages_core() in
>> >__meminit section, since this function is only used by __init functions and
>> >in memory_hotplug.c.  This means it is save to release it if
>> >CONFIG_MEMORY_HOTPLUG is set.
>> >
>> >Then I add __meminit to function __free_pages_core() and face the warning from
>> >modpost.
>> >
>> >  WARNING: modpost: vmlinux: section mismatch in reference: generic_online_page+0xa (section: .text) -> __free_pages_core (section: .meminit.text)
>> >
>> >A .text function calls init code is not proper. Then I add __meminit to
>> >generic_online_page too. Then I face this warning from modpost.
>> >
>> >  WARNING: modpost: vmlinux: generic_online_page: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.
>> >
>>
>> I guess I found the correct way.
>>
>> Add __ref to generic_online_page to not issue a warning.
>
>
>
>Yes, __ref is used to bypass the section mismatch check.
>

I am think whether __ref is providing a gate to escape the check of modpost?

>Some functions in mm/memory_hotplug.c are annotated as __ref
>to reference __meminit functions.
>
>Adding __ref is the easy solution.
>
>
>
>Having said that, I started to think
>eb8f689046b857874e964463619f09df06d59fad was the wrong decision.
>I will revert it.
>

Oh, I finally understand it... didn't think that was a wrong decision :-(

>
>
>
>
>
>
>
>
>
>
>-- 
>Best Regards
>Masahiro Yamada

-- 
Wei Yang
Help you, Help me