[PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

Supporting two different intermediate-artifact packaging schemes
was only ever intended as a temporary transition.

This has so far caused no problems for powerpc, after a small fix
for how the arch invoked ar. So now allow any arch to select the
option, continue defaulting to N.

Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
The next step will be to have archs always select THIN_ARCHIVES
when they are known to work. Then remove the option entirely.

x86 has always just worked for me, so that should be easy.

 arch/Kconfig         | 19 ++++++++++++++++---
 arch/powerpc/Kconfig |  8 --------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 6c00e5b00f8b..28e64cb65dd5 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -511,10 +511,23 @@ config CC_STACKPROTECTOR_STRONG
 endchoice
 
 config THIN_ARCHIVES
-	bool
+	bool "Build the kernel using thin archives"
+	default n
 	help
-	  Select this if the architecture wants to use thin archives
-	  instead of ld -r to create the built-in.o files.
+	  Enable this if you want to use thin archives (binutils `ar` thin
+	  archive format) rather than `ld -r`, to create the built-in.o and
+	  other intermediate packaging steps in the kernel build.
+
+	  This option reduces disk space for builds, especially important for
+	  debug builds and IO-constrained environments. It gives the linker
+	  more flexibility in assembling sections. And it is more amenable to
+	  link-time-optimization that may be implemented in future.
+
+	  The intention is for all architectures to move to thin archives
+	  soon, and the ld -r support code removed.
+
+	  If unsure, say N.
+	  Kernel hackers, say Y and test/fix your arch!
 
 config LD_DEAD_CODE_DATA_ELIMINATION
 	bool
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6eb70c96ec5e..9274d9faf122 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -483,14 +483,6 @@ config MPROFILE_KERNEL
 	depends on PPC64 && CPU_LITTLE_ENDIAN
 	def_bool !DISABLE_MPROFILE_KERNEL
 
-config USE_THIN_ARCHIVES
-	bool "Build the kernel using thin archives"
-	default n
-	select THIN_ARCHIVES
-	help
-	  Build the kernel using thin archives.
-	  If you're unsure say N.
-
 config IOMMU_HELPER
 	def_bool PPC64
 
-- 
2.11.0

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Stephen Rothwell]

Hi Nick,

On Mon, 29 May 2017 18:11:03 +1000 Nicholas Piggin <npiggin@gmail.com> wrote:
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 6c00e5b00f8b..28e64cb65dd5 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -511,10 +511,23 @@ config CC_STACKPROTECTOR_STRONG
>  endchoice
>  
>  config THIN_ARCHIVES
> -	bool
> +	bool "Build the kernel using thin archives"
> +	default n

Of course, this will be enabled by allmodconfig, etc builds ...

-- 
Cheers,
Stephen Rothwell

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

On Mon, 29 May 2017 20:33:55 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Nick,
> 
> On Mon, 29 May 2017 18:11:03 +1000 Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 6c00e5b00f8b..28e64cb65dd5 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -511,10 +511,23 @@ config CC_STACKPROTECTOR_STRONG
> >  endchoice
> >  
> >  config THIN_ARCHIVES
> > -	bool
> > +	bool "Build the kernel using thin archives"
> > +	default n  
> 
> Of course, this will be enabled by allmodconfig, etc builds ...
> 

Yes that is true. Thanks for making a note of it -- I guess the
changelog could be interpreted as implying no change to any
existing build without arch enablement. I didn't intend to mislead
there.

Thanks,
Nick

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Linus Torvalds]

On Mon, May 29, 2017 at 1:11 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> The next step will be to have archs always select THIN_ARCHIVES
> when they are known to work. Then remove the option entirely.

Honestly, why even do that?

If we really think that thin archives is the way to go, and it's known
to work on ppc, x86 and arm, why not just do it unconditionally?

I hate our kconfig process as-is, and asking about odd binutils things
that nobody knows the answer to is just rude. Plus it will actually be
really really painful to find problems triggered by this, because it
will be some magical config detail that nobody thinks of.

In contrast, if we just enable this unconditionally,  and it causes
problems, they will show up the culprit trivially with "git bisect".

So either thin archives work or they don't. Don't make it some painful
drawn-out process that actually makes it harder to test rather than
easier.

                          Linus

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Linus Torvalds]

On Mon, May 29, 2017 at 10:03 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So either thin archives work or they don't. Don't make it some painful
> drawn-out process that actually makes it harder to test rather than
> easier.

Side note: what would make them not work? Are there known bugs in 'ar'
wrt thin archives, versioning issues, or other issues?

                 Linus

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Sam Ravnborg]

Hi Nicholas.

> Supporting two different intermediate-artifact packaging schemes
> was only ever intended as a temporary transition.
> 
> This has so far caused no problems for powerpc, after a small fix
> for how the arch invoked ar. So now allow any arch to select the
> option, continue defaulting to N.

Alan Modra recommended this approach several years ago, and I think
Stephen was the first to implement this for the kernel.
It would be good to have the rational whay ar is better than ld -r
included in the commit message for later reference.

I also recall that using ar gave some small kernel size reductions
from last time we played with this.
This info could also be nice to give a rough idea of the impact.

Added Alan to list of receivers.

	Sam

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

On Mon, 29 May 2017 10:14:50 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, May 29, 2017 at 10:03 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So either thin archives work or they don't. Don't make it some painful
> > drawn-out process that actually makes it harder to test rather than
> > easier.  
>
> Side note: what would make them not work? Are there known bugs in 'ar'
> wrt thin archives, versioning issues, or other issues?

We haven't run into any bugs AFAIK. It's been pretty solid.

*thin* archives does require binutils 2.19. That's nearly 10 years
old, but we advertise 2.12 minimum at the moment. We'd have make "T"
an optional argument if we turn it on unconditionally. Regular
archives shouldn't be significantly worse than ld -r though, so yeah
maybe we could do that.

powerpc required a build fix:

43c9127d94d

-override AR    := GNUTARGET=elf$(BITS)-$(GNUTARGET) $(AR)
+KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)

So there could be a couple of small things like this.

Thanks,
Nick

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

On Mon, 29 May 2017 20:52:51 +0200
Sam Ravnborg <sam@ravnborg.org> wrote:

> Hi Nicholas.
> 
> > Supporting two different intermediate-artifact packaging schemes
> > was only ever intended as a temporary transition.
> > 
> > This has so far caused no problems for powerpc, after a small fix
> > for how the arch invoked ar. So now allow any arch to select the
> > option, continue defaulting to N.  
> 
> Alan Modra recommended this approach several years ago, and I think
> Stephen was the first to implement this for the kernel.
> It would be good to have the rational whay ar is better than ld -r
> included in the commit message for later reference.

We *are* using Stephen's thin archives build infrastructure in the
kernel already. Only powerpc is using it so far: a5967db9af

The big one for powerpc build is that  the linker keeps relative
location of input sections the same, so as you link into larger
built-in.o files, there is less opportunity to place code optimally.
x86 may not care so much, but some other archs will.

The build output size improvement is nice for everyone though.

> I also recall that using ar gave some small kernel size reductions
> from last time we played with this.
> This info could also be nice to give a rough idea of the impact.

There is some explanation in sfr's patch. It's not thin archives
as such, but rather we have to link with --whole-archive, but it
was a very small impact.

IIRC today's final link is technically buggy without that option,
it's just that in practice the top-level built-in.o files pull in
so much that the linker will never discard one.

Thanks,
Nick

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Linus Torvalds]

On Mon, May 29, 2017 at 4:13 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> *thin* archives does require binutils 2.19. That's nearly 10 years
> old, but we advertise 2.12 minimum at the moment.

I think we're ok with upgrading that. We've been allowing some
ridiculously old tools at times. I think "feature is 10 years old and
actively used by other projects" (Qt uses it from some googling, for
example) is more than sufficient.

If somebody has user space older than that, they presumably aren't
building new kernels either.

                     Linus

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Linus Torvalds]

On Mon, May 29, 2017 at 4:13 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> We haven't run into any bugs AFAIK. It's been pretty solid.

I tried just changing the THIN_ARCHIVES "bool" into "def_bool y" to
test it on my laptop, and certainly saw neither build problems nor
problems with the resulting kernel image.

So honestly, I'd really rather just see the change happen
unconditionally, and finding problems much quicker and easier that way
(in addition to making the conversion much simpler and not asking
people odd questions).

                    Linus

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

On Mon, 29 May 2017 20:22:41 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, May 29, 2017 at 4:13 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > *thin* archives does require binutils 2.19. That's nearly 10 years
> > old, but we advertise 2.12 minimum at the moment.  
> 
> I think we're ok with upgrading that. We've been allowing some
> ridiculously old tools at times. I think "feature is 10 years old and
> actively used by other projects" (Qt uses it from some googling, for
> example) is more than sufficient.

Well we might have to. I couldn't make thick archives work easily
because we have multiple levels of built-in.o, and with thick you
get an archive of an archive which the linker does not understand.
Fixing that would mean unpacking the archive and repacking it for
every built-in.o input you link -- too painful.

I suspect Google uses the format internally too because they added
the initial support.

https://sourceware.org/ml/binutils/2008-03/msg00150.html

Support has been pervasively added to binutils -- nm, objdump, ld,
etc. I think it falls into the category of well supported.

> 
> If somebody has user space older than that, they presumably aren't
> building new kernels either.

Yeah I think it's reasonable to drop support for such old toolchain
if we have a reason for it.

I'll send a patch to 0day and if that passes, I'll see if it's
something Stephen would put into linux-next.

Thanks,
Nick

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Arnd Bergmann]

On Mon, May 29, 2017 at 10:11 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> Supporting two different intermediate-artifact packaging schemes
> was only ever intended as a temporary transition.
>
> This has so far caused no problems for powerpc, after a small fix
> for how the arch invoked ar. So now allow any arch to select the
> option, continue defaulting to N.
>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> The next step will be to have archs always select THIN_ARCHIVES
> when they are known to work. Then remove the option entirely.
>
> x86 has always just worked for me, so that should be easy.

I have build-tested many thousand randconfig kernels on arm32 with
this option enabled, and did not run into build-time regressions
besides some initial problems from a broken binutils snapshot
(all released binutils versions should be fine).

       Arnd

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Masahiro Yamada]

Hi.

2017-06-01 6:13 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, May 29, 2017 at 10:11 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>> Supporting two different intermediate-artifact packaging schemes
>> was only ever intended as a temporary transition.
>>
>> This has so far caused no problems for powerpc, after a small fix
>> for how the arch invoked ar. So now allow any arch to select the
>> option, continue defaulting to N.
>>
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> The next step will be to have archs always select THIN_ARCHIVES
>> when they are known to work. Then remove the option entirely.
>>
>> x86 has always just worked for me, so that should be easy.
>
> I have build-tested many thousand randconfig kernels on arm32 with
> this option enabled, and did not run into build-time regressions
> besides some initial problems from a broken binutils snapshot
> (all released binutils versions should be fine).
>


Please let me mention two advantages of using THIN_ARCHIVES.



[1]
By switching to CONFIG_THIN_ARCHIVES,
we can get back building allyesconfig on ARM.
(probably because the final link is given more flexibility
to insert veneers)

This is a much cleaner solution than the following patch:
https://patchwork.kernel.org/patch/6120441/



[2]
Thick archive does not work well with garbage collection
(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION).

CONFIG_LD_DEAD_CODE_DATA_ELIMINATION can not deal with
static functions with the same function name.


The following is the test code I played with.



diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3dcd7ec..a267145 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -13,6 +13,7 @@ config ARM64
        select ARCH_HAS_ELF_RANDOMIZE
        select ARCH_HAS_GCOV_PROFILE_ALL
        select ARCH_HAS_GIGANTIC_PAGE
+       select LD_DEAD_CODE_DATA_ELIMINATION
        select ARCH_HAS_KCOV
        select ARCH_HAS_SET_MEMORY
        select ARCH_HAS_SG_CHAIN
--- a/init/main.c
+++ b/init/main.c
@@ -485,6 +485,8 @@ static void __init mm_init(void)
        ioremap_huge_init();
 }

+void this_is_called(void);
+
 asmlinkage __visible void __init start_kernel(void)
 {
        char *command_line;
@@ -676,6 +678,9 @@ asmlinkage __visible void __init start_kernel(void)
        }

        /* Do the rest non-__init'ed, we're now alive */
+
+       this_is_called();
+
        rest_init();
 }

diff --git a/kernel/Makefile b/kernel/Makefile
index 72aa080..b938d4e 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -9,7 +9,8 @@ obj-y     = fork.o exec_domain.o panic.o \
            extable.o params.o \
            kthread.o sys_ni.o nsproxy.o \
            notifier.o ksysfs.o cred.o reboot.o \
-           async.o range.o smpboot.o ucount.o
+           async.o range.o smpboot.o ucount.o \
+           foo.o bar.o

 obj-$(CONFIG_MULTIUSER) += groups.o

diff --git a/kernel/bar.c b/kernel/bar.c
new file mode 100644
index 0000000..3b1431e
--- /dev/null
+++ b/kernel/bar.c
@@ -0,0 +1,31 @@
+#include <linux/printk.h>
+
+static void greeting(void)
+{
+       printk("hello world0\n");
+       printk("hello world1\n");
+       printk("hello world2\n");
+       printk("hello world3\n");
+       printk("hello world4\n");
+       printk("hello world5\n");
+       printk("hello world6\n");
+       printk("hello world7\n");
+       printk("hello world8\n");
+       printk("hello world9\n");
+}
+
+void this_is_not_called1(void)
+{
+       int i;
+
+       for (i = 0; i < 100; i++)
+               greeting();
+}
+
+void this_is_not_called2(void)
+{
+       int i;
+
+       for (i = 0; i < 1000; i++)
+               greeting();
+}
diff --git a/kernel/foo.c b/kernel/foo.c
new file mode 100644
index 0000000..10c8e8b
--- /dev/null
+++ b/kernel/foo.c
@@ -0,0 +1,31 @@
+#include <linux/printk.h>
+
+static void greeting(void)
+{
+       printk("goodbye world0\n");
+       printk("goodbye world1\n");
+       printk("goodbye world2\n");
+       printk("goodbye world3\n");
+       printk("goodbye world4\n");
+       printk("goodbye world5\n");
+       printk("goodbye world6\n");
+       printk("goodbye world7\n");
+       printk("goodbye world8\n");
+       printk("goodbye world9\n");
+}
+
+void this_is_called(void)
+{
+       int i;
+
+       for (i = 0; i < 100; i++)
+               greeting();
+}
+
+void this_is_not_called0(void)
+{
+       int i;
+
+       for (i = 0; i < 1000; i++)
+               greeting();
+}



The function greeting() in kernel/foo.c is called,
whereas greeting() in kernel/bar.c is not called.

The latter should be removed from the final binary,
but both are kept.


$ aarch64-linux-gnu-nm  vmlinux | grep 'greeting\|this_is_'
ffff0000080f7420 t greeting
ffff0000080f74a8 t greeting
ffff0000080f7530 T this_is_called





By selecting CONFIG_THIN_ARCHIVES, greeting()
in kernel/bar.c is removed correctly.


$ aarch64-linux-gnu-nm  vmlinux | grep 'greeting\|this_is_'
ffff0000080f2bf0 t greeting
ffff0000080f2c78 T this_is_called





-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: provide THIN_ARCHIVES option for all architectures

[Nicholas Piggin]

On Mon, 5 Jun 2017 08:17:26 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi.
> 
> 2017-06-01 6:13 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> > On Mon, May 29, 2017 at 10:11 AM, Nicholas Piggin <npiggin@gmail.com> wrote:  
> >> Supporting two different intermediate-artifact packaging schemes
> >> was only ever intended as a temporary transition.
> >>
> >> This has so far caused no problems for powerpc, after a small fix
> >> for how the arch invoked ar. So now allow any arch to select the
> >> option, continue defaulting to N.
> >>
> >> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >> The next step will be to have archs always select THIN_ARCHIVES
> >> when they are known to work. Then remove the option entirely.
> >>
> >> x86 has always just worked for me, so that should be easy.  
> >
> > I have build-tested many thousand randconfig kernels on arm32 with
> > this option enabled, and did not run into build-time regressions
> > besides some initial problems from a broken binutils snapshot
> > (all released binutils versions should be fine).
> >  
> 
> 
> Please let me mention two advantages of using THIN_ARCHIVES.

[snip]

Thank you for the post. I ran it through 0day and found some issues
with a few archs that I'm working through. Some changes are needed to
the generic thin archives build scripts, and some arch tweaks required.
So it's not 100% trivial.

4.13 might still be a realistic target if I make some progress.

Thanks,
Nick