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

From: Allen Webb <allenwebb@google.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	gregkh@linuxfoundation.org, christophe.leroy@csgroup.eu,
	nick.alcock@oracle.com
Subject: Re: [PATCH v10 08/11] build: Add modules.builtin.alias
Date: Wed, 19 Jul 2023 14:51:48 -0500	[thread overview]
Message-ID: <CAJzde04MmfyGeAzQ_7FW-0sATk7TT-MkxCbNPSzb-94wK6nhkA@mail.gmail.com> (raw)
In-Reply-To: <ZG22iPLED+SJsEFa@bombadil.infradead.org>

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
> >