| Message ID | 20240702234008.19101-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] mm: use zonelist_zone() to get zone | expand |
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?
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.
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.
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?
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.
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
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.
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
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"
.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(+)