* Re: [bpf-next PATCH v2 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
2023-10-18 15:19 ` [bpf-next PATCH v2 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
@ 2023-10-19 8:47 ` Jiri Olsa
2023-10-23 0:33 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 3+ messages in thread
From: Jiri Olsa @ 2023-10-19 8:47 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Alan Maguire, Nicolas Schier,
Miguel Ojeda, 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,
Nathan Chancellor, Nick Desaulniers, Nicolas Schier, Song Liu,
Stanislav Fomichev, Wedson Almeida Filho, Yonghong Song, bpf,
rust-for-linux
On Thu, Oct 19, 2023 at 12:19:48AM +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 executed 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>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
> Reviewed-by: Nicolas Schier <n.schier@avm.de>
> Tested-by: Miguel Ojeda <ojeda@kernel.org>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
nice!
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
> ---
>
> Changes in v2:
> - Fix a typo in commit description
> - Update MAINTAINERS
>
> MAINTAINERS | 2 +-
> Makefile | 4 +---
> scripts/Makefile.btf | 19 +++++++++++++++++++
> scripts/pahole-flags.sh | 30 ------------------------------
> 4 files changed, 21 insertions(+), 34 deletions(-)
> create mode 100644 scripts/Makefile.btf
> delete mode 100755 scripts/pahole-flags.sh
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 35977b269d5e..a08d558b1aaa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3742,7 +3742,7 @@ F: net/sched/act_bpf.c
> F: net/sched/cls_bpf.c
> F: samples/bpf/
> F: scripts/bpf_doc.py
> -F: scripts/pahole-flags.sh
> +F: scripts/Makefile.btf
> F: scripts/pahole-version.sh
> F: tools/bpf/
> F: tools/lib/bpf/
> 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] 3+ messages in thread* Re: [bpf-next PATCH v2 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh
2023-10-18 15:19 ` [bpf-next PATCH v2 2/4] kbuild: avoid too many execution of scripts/pahole-flags.sh Masahiro Yamada
2023-10-19 8:47 ` Jiri Olsa
@ 2023-10-23 0:33 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 3+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-10-23 0:33 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Alan Maguire, Nicolas Schier, Miguel Ojeda,
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, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Song Liu, Stanislav Fomichev,
Wedson Almeida Filho, Yonghong Song, bpf, rust-for-linux
On 10/18/23 12:19, 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 executed 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>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
> Reviewed-by: Nicolas Schier <n.schier@avm.de>
> Tested-by: Miguel Ojeda <ojeda@kernel.org>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> [...]
> @@ -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
Would've used a tab, for legibility sake.
> include-$(CONFIG_KASAN) += scripts/Makefile.kasan
> include-$(CONFIG_KCSAN) += scripts/Makefile.kcsan
> include-$(CONFIG_KMSAN) += scripts/Makefile.kmsan
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 3+ messages in thread