| 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?
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(+)