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

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

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