rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
       [not found] <20231017103742.130927-1-masahiroy@kernel.org>
@ 2023-10-17 10:37 ` Masahiro Yamada
  2023-10-17 12:14   ` Alan Maguire
                     ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Masahiro Yamada @ 2023-10-17 10:37 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

scripts/pahole-flags.sh is executed so many times.

You can check how many times it is invoked during the build, as follows:

  $ cat <<EOF >> scripts/pahole-flags.sh
  > echo "scripts/pahole-flags.sh was executed" >&2
  > EOF

  $ make -s
  scripts/pahole-flags.sh was executed
  scripts/pahole-flags.sh was executed
  scripts/pahole-flags.sh was executed
  scripts/pahole-flags.sh was executed
  scripts/pahole-flags.sh was executed
    [ lots of repeated lines suppressed... ]

This scripts is exectuted more than 20 times during the kernel build
because PAHOLE_FLAGS is a recursively expanded variable and exported
to sub-processes.

With the GNU Make >= 4.4, it is executed more than 60 times because
exported variables are also passed to other $(shell ) invocations.
Without careful coding, it is known to cause an exponential fork
explosion. [1]

The use of $(shell ) in an exported recursive variable is likely wrong
because $(shell ) is always evaluated due to the 'export' keyword, and
the evaluation can occur multiple times by the nature of recursive
variables.

Convert the shell script to a Makefile, which is included only when
CONFIG_DEBUG_INFO_BTF=y.

[1]: https://savannah.gnu.org/bugs/index.php?64746

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Makefile                |  4 +---
 scripts/Makefile.btf    | 19 +++++++++++++++++++
 scripts/pahole-flags.sh | 30 ------------------------------
 3 files changed, 20 insertions(+), 33 deletions(-)
 create mode 100644 scripts/Makefile.btf
 delete mode 100755 scripts/pahole-flags.sh

diff --git a/Makefile b/Makefile
index fed9a6cc3665..eaddec67e5e1 100644
--- a/Makefile
+++ b/Makefile
@@ -513,8 +513,6 @@ LZ4		= lz4c
 XZ		= xz
 ZSTD		= zstd
 
-PAHOLE_FLAGS	= $(shell PAHOLE=$(PAHOLE) $(srctree)/scripts/pahole-flags.sh)
-
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
 		  -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
 NOSTDINC_FLAGS :=
@@ -605,7 +603,6 @@ export KBUILD_RUSTFLAGS RUSTFLAGS_KERNEL RUSTFLAGS_MODULE
 export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
 export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_RUSTFLAGS_MODULE KBUILD_LDFLAGS_MODULE
 export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL KBUILD_RUSTFLAGS_KERNEL
-export PAHOLE_FLAGS
 
 # Files to ignore in find ... statements
 
@@ -1002,6 +999,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 # include additional Makefiles when needed
 include-y			:= scripts/Makefile.extrawarn
 include-$(CONFIG_DEBUG_INFO)	+= scripts/Makefile.debug
+include-$(CONFIG_DEBUG_INFO_BTF)+= scripts/Makefile.btf
 include-$(CONFIG_KASAN)		+= scripts/Makefile.kasan
 include-$(CONFIG_KCSAN)		+= scripts/Makefile.kcsan
 include-$(CONFIG_KMSAN)		+= scripts/Makefile.kmsan
diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
new file mode 100644
index 000000000000..82377e470aed
--- /dev/null
+++ b/scripts/Makefile.btf
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+
+pahole-ver := $(CONFIG_PAHOLE_VERSION)
+pahole-flags-y :=
+
+# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
+ifeq ($(call test-le, $(pahole-ver), 121),y)
+pahole-flags-$(call test-ge, $(pahole-ver), 118)	+= --skip_encoding_btf_vars
+endif
+
+pahole-flags-$(call test-ge, $(pahole-ver), 121)	+= --btf_gen_floats
+
+pahole-flags-$(call test-ge, $(pahole-ver), 122)	+= -j
+
+pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
+
+pahole-flags-$(call test-ge, $(pahole-ver), 125)	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
+
+export PAHOLE_FLAGS := $(pahole-flags-y)
diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
deleted file mode 100755
index 728d55190d97..000000000000
--- a/scripts/pahole-flags.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-
-extra_paholeopt=
-
-if ! [ -x "$(command -v ${PAHOLE})" ]; then
-	exit 0
-fi
-
-pahole_ver=$($(dirname $0)/pahole-version.sh ${PAHOLE})
-
-if [ "${pahole_ver}" -ge "118" ] && [ "${pahole_ver}" -le "121" ]; then
-	# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
-	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_vars"
-fi
-if [ "${pahole_ver}" -ge "121" ]; then
-	extra_paholeopt="${extra_paholeopt} --btf_gen_floats"
-fi
-if [ "${pahole_ver}" -ge "122" ]; then
-	extra_paholeopt="${extra_paholeopt} -j"
-fi
-if [ "${pahole_ver}" -ge "124" ]; then
-	# see PAHOLE_HAS_LANG_EXCLUDE
-	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
-fi
-if [ "${pahole_ver}" -ge "125" ]; then
-	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
-fi
-
-echo ${extra_paholeopt}
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
@ 2023-10-17 12:14   ` Alan Maguire
  2023-10-17 12:15   ` Jiri Olsa
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Alan Maguire @ 2023-10-17 12:14 UTC (permalink / raw)
  To: Masahiro Yamada, linux-kbuild
  Cc: linux-kernel, Alex Gaynor, Alexei Starovoitov, Alice Ryhl,
	Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On 17/10/2023 11:37, Masahiro Yamada wrote:
> scripts/pahole-flags.sh is executed so many times.
> 
> You can check how many times it is invoked during the build, as follows:
> 
>   $ cat <<EOF >> scripts/pahole-flags.sh
>   > echo "scripts/pahole-flags.sh was executed" >&2
>   > EOF
> 
>   $ make -s
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>     [ lots of repeated lines suppressed... ]
> 
> This scripts is exectuted more than 20 times during the kernel build
> because PAHOLE_FLAGS is a recursively expanded variable and exported
> to sub-processes.
> 
> With the GNU Make >= 4.4, it is executed more than 60 times because
> exported variables are also passed to other $(shell ) invocations.
> Without careful coding, it is known to cause an exponential fork
> explosion. [1]
> 
> The use of $(shell ) in an exported recursive variable is likely wrong
> because $(shell ) is always evaluated due to the 'export' keyword, and
> the evaluation can occur multiple times by the nature of recursive
> variables.
> 
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.
> 
> [1]: https://savannah.gnu.org/bugs/index.php?64746
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Looks like a neat change to me. I tested with CONFIG_DEBUG_INFO_BTF=y
builds for full kernel build and 'make M=modpath' module-only builds,
and both work well with correct pahole flags. Feel free to add
either/both of

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
Tested-by: Alan Maguire <alan.maguire@oracle.com>

> ---
> 
>  Makefile                |  4 +---
>  scripts/Makefile.btf    | 19 +++++++++++++++++++
>  scripts/pahole-flags.sh | 30 ------------------------------
>  3 files changed, 20 insertions(+), 33 deletions(-)
>  create mode 100644 scripts/Makefile.btf
>  delete mode 100755 scripts/pahole-flags.sh
> 
> diff --git a/Makefile b/Makefile
> index fed9a6cc3665..eaddec67e5e1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -513,8 +513,6 @@ LZ4		= lz4c
>  XZ		= xz
>  ZSTD		= zstd
>  
> -PAHOLE_FLAGS	= $(shell PAHOLE=$(PAHOLE) $(srctree)/scripts/pahole-flags.sh)
> -
>  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
>  		  -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
>  NOSTDINC_FLAGS :=
> @@ -605,7 +603,6 @@ export KBUILD_RUSTFLAGS RUSTFLAGS_KERNEL RUSTFLAGS_MODULE
>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_RUSTFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL KBUILD_RUSTFLAGS_KERNEL
> -export PAHOLE_FLAGS
>  
>  # Files to ignore in find ... statements
>  
> @@ -1002,6 +999,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  # include additional Makefiles when needed
>  include-y			:= scripts/Makefile.extrawarn
>  include-$(CONFIG_DEBUG_INFO)	+= scripts/Makefile.debug
> +include-$(CONFIG_DEBUG_INFO_BTF)+= scripts/Makefile.btf
>  include-$(CONFIG_KASAN)		+= scripts/Makefile.kasan
>  include-$(CONFIG_KCSAN)		+= scripts/Makefile.kcsan
>  include-$(CONFIG_KMSAN)		+= scripts/Makefile.kmsan
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> new file mode 100644
> index 000000000000..82377e470aed
> --- /dev/null
> +++ b/scripts/Makefile.btf
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +pahole-ver := $(CONFIG_PAHOLE_VERSION)
> +pahole-flags-y :=
> +
> +# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> +ifeq ($(call test-le, $(pahole-ver), 121),y)
> +pahole-flags-$(call test-ge, $(pahole-ver), 118)	+= --skip_encoding_btf_vars
> +endif
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 121)	+= --btf_gen_floats
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 122)	+= -j
> +
> +pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 125)	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> +
> +export PAHOLE_FLAGS := $(pahole-flags-y)
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> deleted file mode 100755
> index 728d55190d97..000000000000
> --- a/scripts/pahole-flags.sh
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -
> -extra_paholeopt=
> -
> -if ! [ -x "$(command -v ${PAHOLE})" ]; then
> -	exit 0
> -fi
> -
> -pahole_ver=$($(dirname $0)/pahole-version.sh ${PAHOLE})
> -
> -if [ "${pahole_ver}" -ge "118" ] && [ "${pahole_ver}" -le "121" ]; then
> -	# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> -	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_vars"
> -fi
> -if [ "${pahole_ver}" -ge "121" ]; then
> -	extra_paholeopt="${extra_paholeopt} --btf_gen_floats"
> -fi
> -if [ "${pahole_ver}" -ge "122" ]; then
> -	extra_paholeopt="${extra_paholeopt} -j"
> -fi
> -if [ "${pahole_ver}" -ge "124" ]; then
> -	# see PAHOLE_HAS_LANG_EXCLUDE
> -	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> -fi
> -if [ "${pahole_ver}" -ge "125" ]; then
> -	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> -fi
> -
> -echo ${extra_paholeopt}

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
  2023-10-17 12:14   ` Alan Maguire
@ 2023-10-17 12:15   ` Jiri Olsa
  2023-10-17 12:29   ` Nicolas Schier
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jiri Olsa @ 2023-10-17 12:15 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, John Fastabend, KP Singh, Martin KaFai Lau, Miguel Ojeda,
	Nathan Chancellor, Nick Desaulniers, Nicolas Schier, Song Liu,
	Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song, bpf,
	rust-for-linux

On Tue, Oct 17, 2023 at 07:37:40PM +0900, Masahiro Yamada wrote:
> scripts/pahole-flags.sh is executed so many times.
> 
> You can check how many times it is invoked during the build, as follows:
> 
>   $ cat <<EOF >> scripts/pahole-flags.sh
>   > echo "scripts/pahole-flags.sh was executed" >&2
>   > EOF
> 
>   $ make -s
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>     [ lots of repeated lines suppressed... ]
> 
> This scripts is exectuted more than 20 times during the kernel build
> because PAHOLE_FLAGS is a recursively expanded variable and exported
> to sub-processes.
> 
> With the GNU Make >= 4.4, it is executed more than 60 times because
> exported variables are also passed to other $(shell ) invocations.
> Without careful coding, it is known to cause an exponential fork
> explosion. [1]

nice :-\

> 
> The use of $(shell ) in an exported recursive variable is likely wrong
> because $(shell ) is always evaluated due to the 'export' keyword, and
> the evaluation can occur multiple times by the nature of recursive
> variables.
> 
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.

looks good.. could you please resend this patch with bpf-next in subject
so CI tests would trigger for it?

thanks,
jirka

> 
> [1]: https://savannah.gnu.org/bugs/index.php?64746
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  Makefile                |  4 +---
>  scripts/Makefile.btf    | 19 +++++++++++++++++++
>  scripts/pahole-flags.sh | 30 ------------------------------
>  3 files changed, 20 insertions(+), 33 deletions(-)
>  create mode 100644 scripts/Makefile.btf
>  delete mode 100755 scripts/pahole-flags.sh
> 
> diff --git a/Makefile b/Makefile
> index fed9a6cc3665..eaddec67e5e1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -513,8 +513,6 @@ LZ4		= lz4c
>  XZ		= xz
>  ZSTD		= zstd
>  
> -PAHOLE_FLAGS	= $(shell PAHOLE=$(PAHOLE) $(srctree)/scripts/pahole-flags.sh)
> -
>  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
>  		  -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
>  NOSTDINC_FLAGS :=
> @@ -605,7 +603,6 @@ export KBUILD_RUSTFLAGS RUSTFLAGS_KERNEL RUSTFLAGS_MODULE
>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_RUSTFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL KBUILD_RUSTFLAGS_KERNEL
> -export PAHOLE_FLAGS
>  
>  # Files to ignore in find ... statements
>  
> @@ -1002,6 +999,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  # include additional Makefiles when needed
>  include-y			:= scripts/Makefile.extrawarn
>  include-$(CONFIG_DEBUG_INFO)	+= scripts/Makefile.debug
> +include-$(CONFIG_DEBUG_INFO_BTF)+= scripts/Makefile.btf
>  include-$(CONFIG_KASAN)		+= scripts/Makefile.kasan
>  include-$(CONFIG_KCSAN)		+= scripts/Makefile.kcsan
>  include-$(CONFIG_KMSAN)		+= scripts/Makefile.kmsan
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> new file mode 100644
> index 000000000000..82377e470aed
> --- /dev/null
> +++ b/scripts/Makefile.btf
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +pahole-ver := $(CONFIG_PAHOLE_VERSION)
> +pahole-flags-y :=
> +
> +# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> +ifeq ($(call test-le, $(pahole-ver), 121),y)
> +pahole-flags-$(call test-ge, $(pahole-ver), 118)	+= --skip_encoding_btf_vars
> +endif
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 121)	+= --btf_gen_floats
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 122)	+= -j
> +
> +pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 125)	+= --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> +
> +export PAHOLE_FLAGS := $(pahole-flags-y)
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> deleted file mode 100755
> index 728d55190d97..000000000000
> --- a/scripts/pahole-flags.sh
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -
> -extra_paholeopt=
> -
> -if ! [ -x "$(command -v ${PAHOLE})" ]; then
> -	exit 0
> -fi
> -
> -pahole_ver=$($(dirname $0)/pahole-version.sh ${PAHOLE})
> -
> -if [ "${pahole_ver}" -ge "118" ] && [ "${pahole_ver}" -le "121" ]; then
> -	# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> -	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_vars"
> -fi
> -if [ "${pahole_ver}" -ge "121" ]; then
> -	extra_paholeopt="${extra_paholeopt} --btf_gen_floats"
> -fi
> -if [ "${pahole_ver}" -ge "122" ]; then
> -	extra_paholeopt="${extra_paholeopt} -j"
> -fi
> -if [ "${pahole_ver}" -ge "124" ]; then
> -	# see PAHOLE_HAS_LANG_EXCLUDE
> -	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> -fi
> -if [ "${pahole_ver}" -ge "125" ]; then
> -	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> -fi
> -
> -echo ${extra_paholeopt}
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
  2023-10-17 12:14   ` Alan Maguire
  2023-10-17 12:15   ` Jiri Olsa
@ 2023-10-17 12:29   ` Nicolas Schier
  2023-10-17 15:21   ` Miguel Ojeda
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Nicolas Schier @ 2023-10-17 12:29 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

> Subject: kbuild: avoid too many execution of scripts/pahole-flags.sh
executions?

On Tue, Oct 17, 2023 at 07:37:40PM +0900, Masahiro Yamada wrote:
> scripts/pahole-flags.sh is executed so many times.
> 
> You can check how many times it is invoked during the build, as follows:
> 
>   $ cat <<EOF >> scripts/pahole-flags.sh
>   > echo "scripts/pahole-flags.sh was executed" >&2
>   > EOF
> 
>   $ make -s
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>     [ lots of repeated lines suppressed... ]
> 
> This scripts is exectuted more than 20 times during the kernel build

executed

> because PAHOLE_FLAGS is a recursively expanded variable and exported
> to sub-processes.
> 
> With the GNU Make >= 4.4, it is executed more than 60 times because
> exported variables are also passed to other $(shell ) invocations.
> Without careful coding, it is known to cause an exponential fork
> explosion. [1]
> 
> The use of $(shell ) in an exported recursive variable is likely wrong
> because $(shell ) is always evaluated due to the 'export' keyword, and
> the evaluation can occur multiple times by the nature of recursive
> variables.
> 
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.
> 
> [1]: https://savannah.gnu.org/bugs/index.php?64746
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

Thanks, looks good to me!

Reviewed-by: Nicolas Schier <n.schier@avm.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
                     ` (2 preceding siblings ...)
  2023-10-17 12:29   ` Nicolas Schier
@ 2023-10-17 15:21   ` Miguel Ojeda
  2023-10-18 12:14     ` Masahiro Yamada
  2023-10-17 19:56   ` Andrii Nakryiko
  2023-10-23  0:30   ` Martin Rodriguez Reboredo
  5 siblings, 1 reply; 9+ messages in thread
From: Miguel Ojeda @ 2023-10-17 15:21 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On Tue, Oct 17, 2023 at 12:38 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.
>
> [1]: https://savannah.gnu.org/bugs/index.php?64746
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

The field in `MAINTAINERS` should be removed:

    F: scripts/pahole-flags.sh

But other than that, it looks good to me! I tried it for a given
config and it does call `pahole` with the same flags.

Tested-by: Miguel Ojeda <ojeda@kernel.org>
Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
                     ` (3 preceding siblings ...)
  2023-10-17 15:21   ` Miguel Ojeda
@ 2023-10-17 19:56   ` Andrii Nakryiko
  2023-10-18  5:00     ` Masahiro Yamada
  2023-10-23  0:30   ` Martin Rodriguez Reboredo
  5 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2023-10-17 19:56 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On Tue, Oct 17, 2023 at 3:38 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> scripts/pahole-flags.sh is executed so many times.
>
> You can check how many times it is invoked during the build, as follows:
>
>   $ cat <<EOF >> scripts/pahole-flags.sh
>   > echo "scripts/pahole-flags.sh was executed" >&2
>   > EOF
>
>   $ make -s
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>   scripts/pahole-flags.sh was executed
>     [ lots of repeated lines suppressed... ]
>
> This scripts is exectuted more than 20 times during the kernel build
> because PAHOLE_FLAGS is a recursively expanded variable and exported
> to sub-processes.
>
> With the GNU Make >= 4.4, it is executed more than 60 times because
> exported variables are also passed to other $(shell ) invocations.
> Without careful coding, it is known to cause an exponential fork
> explosion. [1]
>
> The use of $(shell ) in an exported recursive variable is likely wrong
> because $(shell ) is always evaluated due to the 'export' keyword, and
> the evaluation can occur multiple times by the nature of recursive
> variables.
>
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.
>
> [1]: https://savannah.gnu.org/bugs/index.php?64746
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  Makefile                |  4 +---
>  scripts/Makefile.btf    | 19 +++++++++++++++++++
>  scripts/pahole-flags.sh | 30 ------------------------------
>  3 files changed, 20 insertions(+), 33 deletions(-)
>  create mode 100644 scripts/Makefile.btf
>  delete mode 100755 scripts/pahole-flags.sh
>
> diff --git a/Makefile b/Makefile
> index fed9a6cc3665..eaddec67e5e1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -513,8 +513,6 @@ LZ4         = lz4c
>  XZ             = xz
>  ZSTD           = zstd
>
> -PAHOLE_FLAGS   = $(shell PAHOLE=$(PAHOLE) $(srctree)/scripts/pahole-flags.sh)

What if we just used := here? Wouldn't it avoid unnecessary multiple executions?

I don't make Makefile.btf approach, just curious why := doesn't work,
if it doesn't.

> -
>  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
>                   -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
>  NOSTDINC_FLAGS :=
> @@ -605,7 +603,6 @@ export KBUILD_RUSTFLAGS RUSTFLAGS_KERNEL RUSTFLAGS_MODULE
>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_RUSTFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL KBUILD_RUSTFLAGS_KERNEL
> -export PAHOLE_FLAGS
>
>  # Files to ignore in find ... statements
>
> @@ -1002,6 +999,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  # include additional Makefiles when needed
>  include-y                      := scripts/Makefile.extrawarn
>  include-$(CONFIG_DEBUG_INFO)   += scripts/Makefile.debug
> +include-$(CONFIG_DEBUG_INFO_BTF)+= scripts/Makefile.btf
>  include-$(CONFIG_KASAN)                += scripts/Makefile.kasan
>  include-$(CONFIG_KCSAN)                += scripts/Makefile.kcsan
>  include-$(CONFIG_KMSAN)                += scripts/Makefile.kmsan
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> new file mode 100644
> index 000000000000..82377e470aed
> --- /dev/null
> +++ b/scripts/Makefile.btf
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +pahole-ver := $(CONFIG_PAHOLE_VERSION)
> +pahole-flags-y :=
> +
> +# pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> +ifeq ($(call test-le, $(pahole-ver), 121),y)
> +pahole-flags-$(call test-ge, $(pahole-ver), 118)       += --skip_encoding_btf_vars
> +endif
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 121)       += --btf_gen_floats
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 122)       += -j
> +
> +pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
> +
> +pahole-flags-$(call test-ge, $(pahole-ver), 125)       += --skip_encoding_btf_inconsistent_proto --btf_gen_optimized
> +
> +export PAHOLE_FLAGS := $(pahole-flags-y)
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> deleted file mode 100755
> index 728d55190d97..000000000000
> --- a/scripts/pahole-flags.sh
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -
> -extra_paholeopt=
> -
> -if ! [ -x "$(command -v ${PAHOLE})" ]; then
> -       exit 0
> -fi
> -
> -pahole_ver=$($(dirname $0)/pahole-version.sh ${PAHOLE})
> -
> -if [ "${pahole_ver}" -ge "118" ] && [ "${pahole_ver}" -le "121" ]; then
> -       # pahole 1.18 through 1.21 can't handle zero-sized per-CPU vars
> -       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_vars"
> -fi
> -if [ "${pahole_ver}" -ge "121" ]; then
> -       extra_paholeopt="${extra_paholeopt} --btf_gen_floats"
> -fi
> -if [ "${pahole_ver}" -ge "122" ]; then
> -       extra_paholeopt="${extra_paholeopt} -j"
> -fi
> -if [ "${pahole_ver}" -ge "124" ]; then
> -       # see PAHOLE_HAS_LANG_EXCLUDE
> -       extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> -fi
> -if [ "${pahole_ver}" -ge "125" ]; then
> -       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> -fi
> -
> -echo ${extra_paholeopt}
> --
> 2.40.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 19:56   ` Andrii Nakryiko
@ 2023-10-18  5:00     ` Masahiro Yamada
  0 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2023-10-18  5:00 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On Wed, Oct 18, 2023 at 4:57 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 3:38 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > scripts/pahole-flags.sh is executed so many times.
> >
> > You can check how many times it is invoked during the build, as follows:
> >
> >   $ cat <<EOF >> scripts/pahole-flags.sh
> >   > echo "scripts/pahole-flags.sh was executed" >&2
> >   > EOF
> >
> >   $ make -s
> >   scripts/pahole-flags.sh was executed
> >   scripts/pahole-flags.sh was executed
> >   scripts/pahole-flags.sh was executed
> >   scripts/pahole-flags.sh was executed
> >   scripts/pahole-flags.sh was executed
> >     [ lots of repeated lines suppressed... ]
> >
> > This scripts is exectuted more than 20 times during the kernel build
> > because PAHOLE_FLAGS is a recursively expanded variable and exported
> > to sub-processes.
> >
> > With the GNU Make >= 4.4, it is executed more than 60 times because
> > exported variables are also passed to other $(shell ) invocations.
> > Without careful coding, it is known to cause an exponential fork
> > explosion. [1]
> >
> > The use of $(shell ) in an exported recursive variable is likely wrong
> > because $(shell ) is always evaluated due to the 'export' keyword, and
> > the evaluation can occur multiple times by the nature of recursive
> > variables.
> >
> > Convert the shell script to a Makefile, which is included only when
> > CONFIG_DEBUG_INFO_BTF=y.
> >
> > [1]: https://savannah.gnu.org/bugs/index.php?64746
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  Makefile                |  4 +---
> >  scripts/Makefile.btf    | 19 +++++++++++++++++++
> >  scripts/pahole-flags.sh | 30 ------------------------------
> >  3 files changed, 20 insertions(+), 33 deletions(-)
> >  create mode 100644 scripts/Makefile.btf
> >  delete mode 100755 scripts/pahole-flags.sh
> >
> > diff --git a/Makefile b/Makefile
> > index fed9a6cc3665..eaddec67e5e1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -513,8 +513,6 @@ LZ4         = lz4c
> >  XZ             = xz
> >  ZSTD           = zstd
> >
> > -PAHOLE_FLAGS   = $(shell PAHOLE=$(PAHOLE) $(srctree)/scripts/pahole-flags.sh)
>
> What if we just used := here? Wouldn't it avoid unnecessary multiple executions?


Yeah, := is less silly than =.


But, I do not like to run the script for non-build targets
such as 'make clean', 'make help', etc.

Also, when building with CONFIG_DEBUG_INFO_BTF=n,
the shell is forked to compute PAHOLE_FLAGS,
which we know are unnecessary.





--
Best Regards

Masahiro Yamada

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 15:21   ` Miguel Ojeda
@ 2023-10-18 12:14     ` Masahiro Yamada
  0 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2023-10-18 12:14 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: linux-kbuild, linux-kernel, Alex Gaynor, Alexei Starovoitov,
	Alice Ryhl, Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On Wed, Oct 18, 2023 at 12:21 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 12:38 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Convert the shell script to a Makefile, which is included only when
> > CONFIG_DEBUG_INFO_BTF=y.
> >
> > [1]: https://savannah.gnu.org/bugs/index.php?64746
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> The field in `MAINTAINERS` should be removed:
>
>     F: scripts/pahole-flags.sh


I will replace it with

F: scripts/Makefile.btf


Thanks.



>
> But other than that, it looks good to me! I tried it for a given
> config and it does call `pahole` with the same flags.
>
> Tested-by: Miguel Ojeda <ojeda@kernel.org>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
>
> Cheers,
> Miguel



-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
  2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
                     ` (4 preceding siblings ...)
  2023-10-17 19:56   ` Andrii Nakryiko
@ 2023-10-23  0:30   ` Martin Rodriguez Reboredo
  5 siblings, 0 replies; 9+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-10-23  0:30 UTC (permalink / raw)
  To: Masahiro Yamada, linux-kbuild
  Cc: linux-kernel, Alex Gaynor, Alexei Starovoitov, Alice Ryhl,
	Andreas Hindborg, Andrii Nakryiko, Benno Lossin,
	Björn Roy Baron, Boqun Feng, Daniel Borkmann, Gary Guo,
	Hao Luo, Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
	Miguel Ojeda, Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Song Liu, Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song,
	bpf, rust-for-linux

On 10/17/23 07:37, Masahiro Yamada wrote:
> scripts/pahole-flags.sh is executed so many times.
> 
> You can check how many times it is invoked during the build, as follows:
> 
>    $ cat <<EOF >> scripts/pahole-flags.sh
>    > echo "scripts/pahole-flags.sh was executed" >&2
>    > EOF
> 
>    $ make -s
>    scripts/pahole-flags.sh was executed
>    scripts/pahole-flags.sh was executed
>    scripts/pahole-flags.sh was executed
>    scripts/pahole-flags.sh was executed
>    scripts/pahole-flags.sh was executed
>      [ lots of repeated lines suppressed... ]
> 
> This scripts is exectuted more than 20 times during the kernel build
> because PAHOLE_FLAGS is a recursively expanded variable and exported
> to sub-processes.
> 
> With the GNU Make >= 4.4, it is executed more than 60 times because
> exported variables are also passed to other $(shell ) invocations.
> Without careful coding, it is known to cause an exponential fork
> explosion. [1]
> 
> The use of $(shell ) in an exported recursive variable is likely wrong
> because $(shell ) is always evaluated due to the 'export' keyword, and
> the evaluation can occur multiple times by the nature of recursive
> variables.
> 
> Convert the shell script to a Makefile, which is included only when
> CONFIG_DEBUG_INFO_BTF=y.
> 
> [1]: https://savannah.gnu.org/bugs/index.php?64746
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> [...]
> +include-$(CONFIG_DEBUG_INFO_BTF)+= scripts/Makefile.btf

Would have used a tab.

> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-23  0:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20231017103742.130927-1-masahiroy@kernel.org>
2023-10-17 10:37 ` [PATCH 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
2023-10-17 12:14   ` Alan Maguire
2023-10-17 12:15   ` Jiri Olsa
2023-10-17 12:29   ` Nicolas Schier
2023-10-17 15:21   ` Miguel Ojeda
2023-10-18 12:14     ` Masahiro Yamada
2023-10-17 19:56   ` Andrii Nakryiko
2023-10-18  5:00     ` Masahiro Yamada
2023-10-23  0:30   ` Martin Rodriguez Reboredo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).