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

[Wei Yang <richard.weiyang@gmail.com>]

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