Re: [PATCH] kbuild: Use --strip-unneeded with INSTALL_MOD_STRIP

[Alexandre Ghiti]

Hi Masahiro,

On 05/02/2025 16:00, Masahiro Yamada wrote:
> On Wed, Feb 5, 2025 at 3:29 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> > On Tue, Feb 04, 2025 at 01:04:26PM +0900, Masahiro Yamada wrote:
> > > On Sat, Feb 1, 2025 at 6:33 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> > > > On Sat, Feb 01, 2025 at 12:10:02AM +0900, Masahiro Yamada wrote:
> > > > > On Fri, Jan 31, 2025 at 3:54 PM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> > > > > > On Thu, Jan 30, 2025 at 08:52:45PM -0700, Nathan Chancellor wrote:
> > > > > > > On Wed, Jan 22, 2025 at 07:17:26PM -0800, Charlie Jenkins wrote:
> > > > > > > > On riscv, kernel modules end up with a significant number of local
> > > > > > > > symbols. This becomes apparent when compiling modules with debug symbols
> > > > > > > > enabled. Using amdgpu.ko as an example of a large module, on riscv the
> > > > > > > > size is 754MB (no stripping), 53MB (--strip-debug), and 21MB
> > > > > > > > (--strip-unneeded). ON x86, amdgpu.ko is 482MB (no stripping), 21MB
> > > > > > > > (--strip-debug), and 20MB (--strip-unneeded).
> > > > > > > >
> > > > > > > > Use --strip-unneeded instead of --strip-debug to strip modules so
> > > > > > > > decrease the size of the resulting modules. This is particularly
> > > > > > > > relevant for riscv, but also marginally aids other architectures.
> > > > > > > >
> > > > > > > > Signed-off-by: Charlie Jenkins <charlie@xxxxxxxxxxxx>
> > > > > > > Is there any sort of regression risk with this patch? If so, another
> > > > > > > option may be to give another level to INSTALL_MOD_STRIP like 2 so that
> > > > > > > INSTALL_MOD_STRIP=1 continues to behave as before but people can easily
> > > > > > > opt into this option. No strong opinion because I am not sure but was
> > > > > > > not sure if it was considered.
> > > > > > I do not believe this would cause regressions. The description on gnu
> > > > > > strip is:
> > > > > >
> > > > > > "Remove all symbols that are not needed for relocation processing in
> > > > > > addition to debugging symbols and sections stripped by --strip-debug."
> > > > > >
> > > > > > The description on llvm-strip is:
> > > > > >
> > > > > > "Remove from the output all local or undefined symbols that are not
> > > > > > required by relocations. Also remove all debug sections."
> > > > > >
> > > > > > gnu strip --strip-unneeded strips slightly more aggressively but it does
> > > > > > not appear this causes any issues.
> > > > > >
> > > > > > > Regardless:
> > > > > > >
> > > > > > > Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > > > > > Thanks!
> > > > >
> > > > > It is true --strip-unneeded drops a lot of compiler-generated symbols, but
> > > > > it also drops real symbols that originate in the source code.
> > > > >
> > > > > So, this would give user-visible changes for kallsyms at least.
> > > > Adding INSTALL_MOD_STRIP="--strip-unneeded" would be sufficient for
> > > > riscv. However, this has the downside that riscv will require different
> > > > flags than other architectures to get reasonably sized modules.
> > > You can use INSTALL_MOD_STRIP=--strip-unneeded for all architecture if you like.
> > >
> > > I assume this is a riscv issue. Specifically, riscv gcc.
> > > With LLVM=1, I see much smaller riscv modules using INSTALL_MOD_STRIP=1.
> > >
> > > --strip-unneeded is needlessly aggressive for other architectures,
> > > and I do not see a good reason to change the default.
> > Yes it is primarily an issue with riscv GCC. I was hoping for something
> > more standardized so that other people using riscv GCC wouldn't
> > encounter this. Would it be reasonable to add this flag by default only
> > for the riscv architecture, or do you think it's just better to leave it
> > up to the user's choice?
> The latter.
>
> INSTALL_MOD_STRIP=1 passes --strip-debug.
> This is clearly documented in Documentation/kbuild/makefiles.rst
> and it has worked like that for many years, with no exception.
>
> If users want it to work differently, the flexibility is already there.
>
> If INSTALL_MOD_STRIP=1 worked differently, silently only for riscv,
> I would regard it as insane behavior.

I find it a bit "harsh" for users to know that on riscv, they need to 
set INSTALL_MOD_STRIP to have modules with a "normal" size.

So I'd rather have it set by default for riscv, would the following be 
acceptable for you?

diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 13fbc0f942387..c49b9dda20cd1 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -6,6 +6,8 @@
 # for more details.
 #

+INSTALL_MOD_STRIP := --strip-unneeded
+
 LDFLAGS_vmlinux := -z norelro
 ifeq ($(CONFIG_RELOCATABLE),y)
        LDFLAGS_vmlinux += -shared -Bsymbolic -z notext --emit-relocs
diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
index 1628198f3e830..e60895761b73b 100644
--- a/scripts/Makefile.modinst
+++ b/scripts/Makefile.modinst
@@ -77,6 +77,8 @@ quiet_cmd_install = INSTALL $@
 # are installed. If INSTALL_MOD_STRIP is '1', then the default option
 # --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will 
be used
 # as the options to the strip command.
+# Read arch-specific Makefile to set INSTALL_MOD_STRIP as needed.
+include $(srctree)/arch/$(SRCARCH)/Makefile
 ifdef INSTALL_MOD_STRIP

 ifeq ($(INSTALL_MOD_STRIP),1)


Thanks,

Alex