diff mbox series

[v10,08/11] build: Add modules.builtin.alias

Message ID 20230406190030.968972-9-allenwebb@google.com
State New
Headers show
Series [v10,01/11] rockchip-mailbox: Remove unneeded MODULE_DEVICE_TABLE | expand

Commit Message

Allen Webb April 6, 2023, 7 p.m. UTC
Generate modules.builtin.alias using modpost and install it with the
modules.

Signed-off-by: Allen Webb <allenwebb@google.com>
---
 .gitignore               |  1 +
 Makefile                 |  1 +
 scripts/Makefile.modpost | 15 +++++++++++++++
 3 files changed, 17 insertions(+)

Comments

Allen Webb July 19, 2023, 7:51 p.m. UTC | #1
I finally got a chance to go through the comments and work on a
follow-up to this series, but it probably makes sense to get this
sorted ahead of the follow-up (if possible).

On Wed, May 24, 2023 at 2:02 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, Apr 06, 2023 at 02:00:27PM -0500, Allen Webb wrote:
> > Generate modules.builtin.alias using modpost and install it with the
> > modules.
>
> Why? This is probably one of the more important commits and the
> commit log is pretty slim.
>
> > Signed-off-by: Allen Webb <allenwebb@google.com>
> > ---
> >  .gitignore               |  1 +
> >  Makefile                 |  1 +
> >  scripts/Makefile.modpost | 15 +++++++++++++++
> >  3 files changed, 17 insertions(+)
> >
> > diff --git a/.gitignore b/.gitignore
> > index 13a7f08a3d73..ddaa622bddac 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -71,6 +71,7 @@ modules.order
> >  /System.map
> >  /Module.markers
> >  /modules.builtin
> > +/modules.builtin.alias
> >  /modules.builtin.modinfo
> >  /modules.nsdeps
> >
> > diff --git a/Makefile b/Makefile
> > index a2c310df2145..43dcc1ea5fcf 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1578,6 +1578,7 @@ __modinst_pre:
> >       fi
> >       @sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
> >       @cp -f modules.builtin $(MODLIB)/
> > +     @cp -f modules.builtin.alias $(MODLIB)/
> >       @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> >
> >  endif # CONFIG_MODULES
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index 0980c58d8afc..e3ecc17a7a19 100644
> > --- a/scripts/Makefile.modpost
> > +++ b/scripts/Makefile.modpost
> > @@ -15,6 +15,7 @@
> >  # 2) modpost is then used to
> >  # 3)  create one <module>.mod.c file per module
> >  # 4)  create one Module.symvers file with CRC for all exported symbols
> > +# 5)  create modules.builtin.alias the aliases for built-in modules
>
> Does everyone want that file?

Not everyone needs it so we could exclude it, but the cost of adding
it isn't that high. I am fine with putting it behind a config, though
we would need to decide whether to have it default on/off.

>
> >  # Step 3 is used to place certain information in the module's ELF
> >  # section, including information such as:
> > @@ -63,6 +64,20 @@ modpost-args += -T $(MODORDER)
> >  modpost-deps += $(MODORDER)
> >  endif
> >
> > +ifneq ($(wildcard vmlinux.o),)
> > +output-builtin.alias := modules.builtin.alias
> > +modpost-args += -b .modules.builtin.alias.in
> > +.modules.builtin.alias.in: $(output-symdump)
> > +     @# Building $(output-symdump) generates .modules.builtin.alias.in as a
> > +     @# side effect.
> > +     @[ -e $@ ] || $(MODPOST) -b .modules.builtin.alias.in vmlinux.o
>
> Does using -b create a delay in builds ? What is the effect on build
> time on a typical 4-core or 8-core build? Does everyone want it?

Here is some data I collected related to build time and memory usage impact:

Without builtin.alias:
TIME="real %e\nuser %U\nsys %S\nres-max %M" time scripts/mod/modpost
-E -o Module.symvers -T modules.order
ERROR: modpost: "__x86_return_thunk"
[arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "kernel_fpu_end" [arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "hchacha_block_generic"
[arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "boot_cpu_data" [arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "static_key_enable" [arch/x86/crypto/chacha-x86_64.ko]
undefined!
ERROR: modpost: "cpu_has_xfeatures" [arch/x86/crypto/chacha-x86_64.ko]
undefined!
ERROR: modpost: "crypto_register_skciphers"
[arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "crypto_unregister_skciphers"
[arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "__stack_chk_fail" [arch/x86/crypto/chacha-x86_64.ko] undefined!
ERROR: modpost: "memset" [arch/x86/crypto/chacha-x86_64.ko] undefined!
WARNING: modpost: suppressed 17432 unresolved symbol warnings because
there were too many)
Command exited with non-zero status 1
real 0.44
user 0.43
sys 0.01
res-max 4896

With builtin.alias:
TIME="real %e\nuser %U\nsys %S\nres-max %M" time scripts/mod/modpost
-E -o Module.symvers -T modules.order -b .modules.builtin.alias.in
vmlinux.o
real 1.43
user 1.38
sys 0.05
res-max 51920

Notice that modpost only uses a single core, so multicore isn't really
as much of a factor here. While it more than triples the time required
for the modpost operation the difference is only about one second of
CPU time. The memory usage is much larger when generating
modules.builtin.alias because of the size of vmlinux.o.

My biggest performance related concern is actually the size difference
of vmlinux caused by the modules.h changes, but it looks like that is
negligible (24KiB):

Without builtin.alias:
du vmlinux.o
663048  vmlinux.o

With builtin.alias:
du vmlinux.o
663072  vmlinux.o

>
> Should we add a new option which lets people decide if they want this
> at build time or not?

I don't feel strongly that there should or should not be a config for
this. On the side for a config the extra second of CPU time and space
taken up by the modules.builtin.alias file would add up across all the
builds and machines, so removing it where it isn't used would help
mitigate that. On the flip side if it isn't used widely enough, it is
more likely that breakages are missed until someone who actually uses
it notices.

Please let me know if you feel strongly either way given the data.

Thanks,
Allen

>
>   Luis
>
> > +
> > +$(output-builtin.alias): .modules.builtin.alias.in
> > +     sort -o $@ $^
> > +
> > +__modpost: $(output-builtin.alias)
> > +endif
> > +
> >  ifeq ($(KBUILD_EXTMOD),)
> >
> >  # Generate the list of in-tree objects in vmlinux
> > --
> > 2.39.2
> >
Luis Chamberlain July 26, 2023, 6:30 p.m. UTC | #2
Please cc Alexander and Alessandro in future patch series as well,
as they could likley be interested in your work too.

On Wed, Jul 19, 2023 at 02:51:48PM -0500, Allen Webb wrote:
> I finally got a chance to go through the comments and work on a
> follow-up to this series, but it probably makes sense to get this
> sorted ahead of the follow-up (if possible).
> 
> On Wed, May 24, 2023 at 2:02 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Thu, Apr 06, 2023 at 02:00:27PM -0500, Allen Webb wrote:
> > > Generate modules.builtin.alias using modpost and install it with the
> > > modules.
> >
> > Why? This is probably one of the more important commits and the
> > commit log is pretty slim.
> >
> > > Signed-off-by: Allen Webb <allenwebb@google.com>
> > > ---
> > >  .gitignore               |  1 +
> > >  Makefile                 |  1 +
> > >  scripts/Makefile.modpost | 15 +++++++++++++++
> > >  3 files changed, 17 insertions(+)
> > >
> > > diff --git a/.gitignore b/.gitignore
> > > index 13a7f08a3d73..ddaa622bddac 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -71,6 +71,7 @@ modules.order
> > >  /System.map
> > >  /Module.markers
> > >  /modules.builtin
> > > +/modules.builtin.alias
> > >  /modules.builtin.modinfo
> > >  /modules.nsdeps
> > >
> > > diff --git a/Makefile b/Makefile
> > > index a2c310df2145..43dcc1ea5fcf 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1578,6 +1578,7 @@ __modinst_pre:
> > >       fi
> > >       @sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
> > >       @cp -f modules.builtin $(MODLIB)/
> > > +     @cp -f modules.builtin.alias $(MODLIB)/
> > >       @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> > >
> > >  endif # CONFIG_MODULES
> > > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > > index 0980c58d8afc..e3ecc17a7a19 100644
> > > --- a/scripts/Makefile.modpost
> > > +++ b/scripts/Makefile.modpost
> > > @@ -15,6 +15,7 @@
> > >  # 2) modpost is then used to
> > >  # 3)  create one <module>.mod.c file per module
> > >  # 4)  create one Module.symvers file with CRC for all exported symbols
> > > +# 5)  create modules.builtin.alias the aliases for built-in modules
> >
> > Does everyone want that file?
> 
> Not everyone needs it so we could exclude it, but the cost of adding
> it isn't that high. I am fine with putting it behind a config, though
> we would need to decide whether to have it default on/off.

We didn't know the cost until I asked, it was the point of asking.
Perhaps Nick, Alessandro or Alexander could use it too later.

> > >  # Step 3 is used to place certain information in the module's ELF
> > >  # section, including information such as:
> > > @@ -63,6 +64,20 @@ modpost-args += -T $(MODORDER)
> > >  modpost-deps += $(MODORDER)
> > >  endif
> > >
> > > +ifneq ($(wildcard vmlinux.o),)
> > > +output-builtin.alias := modules.builtin.alias
> > > +modpost-args += -b .modules.builtin.alias.in
> > > +.modules.builtin.alias.in: $(output-symdump)
> > > +     @# Building $(output-symdump) generates .modules.builtin.alias.in as a
> > > +     @# side effect.
> > > +     @[ -e $@ ] || $(MODPOST) -b .modules.builtin.alias.in vmlinux.o
> >
> > Does using -b create a delay in builds ? What is the effect on build
> > time on a typical 4-core or 8-core build? Does everyone want it?
> 
> Here is some data I collected related to build time and memory usage impact:
> 
> Without builtin.alias:
> TIME="real %e\nuser %U\nsys %S\nres-max %M" time scripts/mod/modpost
> -E -o Module.symvers -T modules.order
> ERROR: modpost: "__x86_return_thunk"
> [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "kernel_fpu_end" [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "hchacha_block_generic"
> [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "boot_cpu_data" [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "static_key_enable" [arch/x86/crypto/chacha-x86_64.ko]
> undefined!
> ERROR: modpost: "cpu_has_xfeatures" [arch/x86/crypto/chacha-x86_64.ko]
> undefined!
> ERROR: modpost: "crypto_register_skciphers"
> [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "crypto_unregister_skciphers"
> [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "__stack_chk_fail" [arch/x86/crypto/chacha-x86_64.ko] undefined!
> ERROR: modpost: "memset" [arch/x86/crypto/chacha-x86_64.ko] undefined!
> WARNING: modpost: suppressed 17432 unresolved symbol warnings because
> there were too many)
> Command exited with non-zero status 1
> real 0.44
> user 0.43
> sys 0.01
> res-max 4896
> 
> With builtin.alias:
> TIME="real %e\nuser %U\nsys %S\nres-max %M" time scripts/mod/modpost
> -E -o Module.symvers -T modules.order -b .modules.builtin.alias.in
> vmlinux.o
> real 1.43
> user 1.38
> sys 0.05
> res-max 51920
> 
> Notice that modpost only uses a single core, so multicore isn't really
> as much of a factor here. While it more than triples the time required
> for the modpost operation the difference is only about one second of
> CPU time. The memory usage is much larger when generating
> modules.builtin.alias because of the size of vmlinux.o.

The modpost impact time of about 1 second for a type of config you used
should be described in your commit log and or kconfig entry to enable
this.

> My biggest performance related concern is actually the size difference
> of vmlinux caused by the modules.h changes, but it looks like that is
> negligible (24KiB):

And this size too.

24 KiB is not that small, so I'd prefer we kconfig'ize it for now and
have those who need it to select it. If we later all want it, we can
default to yes but for now default to no. The default today by
kconfig is to no so an empty default is fine.

> Without builtin.alias:
> du vmlinux.o
> 663048  vmlinux.o
> 
> With builtin.alias:
> du vmlinux.o
> 663072  vmlinux.o

What type of configuration was used? allyesconfig?

> >
> > Should we add a new option which lets people decide if they want this
> > at build time or not?
> 
> I don't feel strongly that there should or should not be a config for
> this. On the side for a config the extra second of CPU time and space
> taken up by the modules.builtin.alias file would add up across all the
> builds and machines, so removing it where it isn't used would help
> mitigate that. On the flip side if it isn't used widely enough, it is
> more likely that breakages are missed until someone who actually uses
> it notices.
> 
> Please let me know if you feel strongly either way given the data.

For now I'd prefer a kconfig option, it's easy to default to y later,
but saving 64 KiB seems like a desirable thing for some folks.

  Luis
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 13a7f08a3d73..ddaa622bddac 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,7 @@  modules.order
 /System.map
 /Module.markers
 /modules.builtin
+/modules.builtin.alias
 /modules.builtin.modinfo
 /modules.nsdeps
 
diff --git a/Makefile b/Makefile
index a2c310df2145..43dcc1ea5fcf 100644
--- a/Makefile
+++ b/Makefile
@@ -1578,6 +1578,7 @@  __modinst_pre:
 	fi
 	@sed 's:^\(.*\)\.o$$:kernel/\1.ko:' modules.order > $(MODLIB)/modules.order
 	@cp -f modules.builtin $(MODLIB)/
+	@cp -f modules.builtin.alias $(MODLIB)/
 	@cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
 
 endif # CONFIG_MODULES
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 0980c58d8afc..e3ecc17a7a19 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -15,6 +15,7 @@ 
 # 2) modpost is then used to
 # 3)  create one <module>.mod.c file per module
 # 4)  create one Module.symvers file with CRC for all exported symbols
+# 5)  create modules.builtin.alias the aliases for built-in modules
 
 # Step 3 is used to place certain information in the module's ELF
 # section, including information such as:
@@ -63,6 +64,20 @@  modpost-args += -T $(MODORDER)
 modpost-deps += $(MODORDER)
 endif
 
+ifneq ($(wildcard vmlinux.o),)
+output-builtin.alias := modules.builtin.alias
+modpost-args += -b .modules.builtin.alias.in
+.modules.builtin.alias.in: $(output-symdump)
+	@# Building $(output-symdump) generates .modules.builtin.alias.in as a
+	@# side effect.
+	@[ -e $@ ] || $(MODPOST) -b .modules.builtin.alias.in vmlinux.o
+
+$(output-builtin.alias): .modules.builtin.alias.in
+	sort -o $@ $^
+
+__modpost: $(output-builtin.alias)
+endif
+
 ifeq ($(KBUILD_EXTMOD),)
 
 # Generate the list of in-tree objects in vmlinux