2021-11-20 16:53:44

by Alex Xu (Hello71)

[permalink] [raw]
Subject: [PATCH 1/2] kbuild: use perl instead of shell to get file size

This makes it easier to get the size of multiple files. Perl is already
a requirement for all builds to do header checks, so this is not an
additional dependency.
---
arch/arm/boot/deflate_xip_data.sh | 2 +-
arch/powerpc/boot/wrapper | 2 +-
scripts/Makefile.lib | 9 ++-------
scripts/file-size.pl | 8 ++++++++
scripts/file-size.sh | 4 ----
scripts/link-vmlinux.sh | 4 ++--
6 files changed, 14 insertions(+), 15 deletions(-)
create mode 100755 scripts/file-size.pl
delete mode 100755 scripts/file-size.sh

diff --git a/arch/arm/boot/deflate_xip_data.sh b/arch/arm/boot/deflate_xip_data.sh
index 304495c3c2c5..14cfa2babb93 100755
--- a/arch/arm/boot/deflate_xip_data.sh
+++ b/arch/arm/boot/deflate_xip_data.sh
@@ -43,7 +43,7 @@ data_start=$(($__data_loc - $base_offset))
data_end=$(($_edata_loc - $base_offset))

# Make sure data occupies the last part of the file.
-file_end=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" "$XIPIMAGE")
+file_end=$(${PERL} "${srctree}/scripts/file-size.pl" "$XIPIMAGE")
if [ "$file_end" != "$data_end" ]; then
printf "end of xipImage doesn't match with _edata_loc (%#x vs %#x)\n" \
$(($file_end + $base_offset)) $_edata_loc 1>&2
diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 9184eda780fd..9f9ee8613432 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -380,7 +380,7 @@ vmz="$tmpdir/`basename \"$kernel\"`.$ext"

# Calculate the vmlinux.strip size
${CROSS}objcopy $objflags "$kernel" "$vmz.$$"
-strip_size=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" "$vmz.$$")
+strip_size=$(${PERL} "${srctree}/scripts/file-size.pl" "$vmz.$$")

if [ -z "$cacheit" -o ! -f "$vmz$compression" -o "$vmz$compression" -ot "$kernel" ]; then
# recompress the image if we need to
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index d1f865b8c0cb..ca901814986a 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -379,13 +379,8 @@ dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)

# Bzip2 and LZMA do not include size in file... so we have to fake that;
# append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell \
-dec_size=0; \
-for F in $(real-prereqs); do \
- fsize=$$($(CONFIG_SHELL) $(srctree)/scripts/file-size.sh $$F); \
- dec_size=$$(expr $$dec_size + $$fsize); \
-done; \
-printf "%08x\n" $$dec_size | \
+total_size = $(shell $(PERL) $(srctree)/scripts/file-size.pl $(real-prereqs))
+size_append = printf $(shell printf "%08x\n" $(total_size) | \
sed 's/\(..\)/\1 /g' | { \
read ch0 ch1 ch2 ch3; \
for ch in $$ch3 $$ch2 $$ch1 $$ch0; do \
diff --git a/scripts/file-size.pl b/scripts/file-size.pl
new file mode 100755
index 000000000000..170bb6d048fa
--- /dev/null
+++ b/scripts/file-size.pl
@@ -0,0 +1,8 @@
+#!/usr/bin/perl -w
+# SPDX-License-Identifier: GPL-2.0
+my $total = 0;
+foreach (@ARGV) {
+ @stat = stat $_ or die "$_: $!";
+ $total += $stat[7];
+}
+print "$total\n";
diff --git a/scripts/file-size.sh b/scripts/file-size.sh
deleted file mode 100755
index 7eb7423416b5..000000000000
--- a/scripts/file-size.sh
+++ /dev/null
@@ -1,4 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-set -- $(ls -dn "$1")
-printf '%s\n' "$5"
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 5cdd9bc5c385..c3fa38bd18ab 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -384,8 +384,8 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
kallsyms_step 2

# step 3
- size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso_prev})
- size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
+ size1=$(${PERL} "${srctree}/scripts/file-size.pl" ${kallsymso_prev})
+ size2=$(${PERL} "${srctree}/scripts/file-size.pl" ${kallsymso})

if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then
kallsyms_step 3
--
2.34.0



2021-11-20 16:53:52

by Alex Xu (Hello71)

[permalink] [raw]
Subject: [PATCH 2/2] kbuild: pass --stream-size --no-content-size to zstd

Otherwise, it allocates 2 GB of memory at once. Even though the majority
of this memory is never touched, the default heuristic overcommit
refuses this request if less than 2 GB of RAM+swap is currently
available. This results in "zstd: error 11 : Allocation error : not
enough memory" and the kernel failing to build.

When the size is specified, zstd will reduce the memory request
appropriately. For typical kernel sizes of ~32 MB, the largest mmap
request will be reduced to 512 MB, which will succeed on all but the
smallest devices.

For inputs around this size, --stream-size --no-content-size may
slightly decrease the compressed size, or slightly increase it:
https://github.com/facebook/zstd/issues/2848.

Signed-off-by: Alex Xu (Hello71) <[email protected]>
---
scripts/Makefile.lib | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index ca901814986a..13d756fbcdc7 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -468,10 +468,10 @@ quiet_cmd_xzmisc = XZMISC $@
# be used because it would require zstd to allocate a 128 MB buffer.

quiet_cmd_zstd = ZSTD $@
- cmd_zstd = { cat $(real-prereqs) | $(ZSTD) -19; $(size_append); } > $@
+ cmd_zstd = { cat $(real-prereqs) | $(ZSTD) --stream-size=$(total_size) --no-content-size -19; $(size_append); } > $@

quiet_cmd_zstd22 = ZSTD22 $@
- cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
+ cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) --stream-size=$(total_size) --no-content-size -22 --ultra; $(size_append); } > $@

# ASM offsets
# ---------------------------------------------------------------------------
--
2.34.0


2021-11-22 19:20:19

by Nick Terrell

[permalink] [raw]
Subject: Re: [PATCH 2/2] kbuild: pass --stream-size --no-content-size to zstd



> On Nov 20, 2021, at 8:53 AM, Alex Xu (Hello71) <[email protected]> wrote:
>
> Otherwise, it allocates 2 GB of memory at once. Even though the majority
> of this memory is never touched, the default heuristic overcommit
> refuses this request if less than 2 GB of RAM+swap is currently
> available. This results in "zstd: error 11 : Allocation error : not
> enough memory" and the kernel failing to build.
>
> When the size is specified, zstd will reduce the memory request
> appropriately. For typical kernel sizes of ~32 MB, the largest mmap
> request will be reduced to 512 MB, which will succeed on all but the
> smallest devices.
>
> For inputs around this size, --stream-size --no-content-size may
> slightly decrease the compressed size, or slightly increase it:
> https://github.com/facebook/zstd/issues/2848.

I like the change in general. However, I have a question of portability.
The --stream-size option was added in zstd-1.4.4 released November
2019 [0]. That means the build will fail for people with older zstd
versions. As a reference, Ubuntu Bionic (18.04) includes zstd-1.3.3,
and Ubuntu Focal (20.04) has zstd-1.4.4. So Bionic users will have to
install zstd manually to build the kernel.

I’d prefer to err on the side of caution for portability. I’d say once
Bionic hits end of life, it’d be safe to require zstd-1.4.4 or newer.
However, I see the need to reduce memory usage, as this issue has
come up before.

Could we detect the zstd version with `zstd --version` and pass
--stream-size if we detect the version is >= v1.4.4? That add some
complexity, but I think it would be worth it. Just be sure to document
why we are detecting support for --stream-size, and why it is important
to pass to zstd.

Best,
Nick Terrell

[0] https://github.com/facebook/zstd/releases/tag/v1.4.4

> Signed-off-by: Alex Xu (Hello71) <[email protected]>
> ---
> scripts/Makefile.lib | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index ca901814986a..13d756fbcdc7 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -468,10 +468,10 @@ quiet_cmd_xzmisc = XZMISC $@
> # be used because it would require zstd to allocate a 128 MB buffer.
>
> quiet_cmd_zstd = ZSTD $@
> - cmd_zstd = { cat $(real-prereqs) | $(ZSTD) -19; $(size_append); } > $@
> + cmd_zstd = { cat $(real-prereqs) | $(ZSTD) --stream-size=$(total_size) --no-content-size -19; $(size_append); } > $@
>
> quiet_cmd_zstd22 = ZSTD22 $@
> - cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
> + cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) --stream-size=$(total_size) --no-content-size -22 --ultra; $(size_append); } > $@
>
> # ASM offsets
> # ---------------------------------------------------------------------------
> --
> 2.34.0
>