Re: [PATCH v2 06/24] x86: Always inline context_tracking_guest_enter()

Peter Zijlstra
2021-08-25T12:18:44Z

On Fri, Aug 20, 2021 at 04:34:40PM -0700, Josh Poimboeuf wrote:

> WARNING: This patch may shrink your patch set and improve your sanity.

I'll go rebuild me some kernels to see how many __always_inline things I
can drop.

Lets add a few Cc's though.

> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH
>
> Modpost's section mismatch detection warns when a non-init function
> references an __init function. The warning is needed because __init
> memory is freed during boot, so the non-init function might end up
> calling into some random memory.
>
> CONFIG_DEBUG_SECTION_MISMATCH is intended to root out more of these
> issues by adding the -fno-inline-functions-called-once compiler flag,
> which forces once-called static functions to not be inlined. As
> described in the option's help description:
>
> - Add the option -fno-inline-functions-called-once to gcc commands.
> When inlining a function annotated with __init in a non-init
> function, we would lose the section information and thus
> the analysis would not catch the illegal reference.
> This option tells gcc to inline less (but it does result in
> a larger kernel).
>
> It's true that if an __init function is inlined in a non-init function,
> it's no longer __init. But that's not actually a problem.
>
> If an inlined __init function doesn't access any other __init functions
> or data, the inlining is harmless.
>
> If the inlined __init function does access other __init functions/data,
> __init relocations will be generated for a non-init section, and
> modpost's section mismatch detection will discover it.
>
> So the option isn't actually useful. It's not even useful for finding
> issues on other configs, which might have different inlining decisions,
> because with -O2, once-called static functions are always inlined. And
> anyway, nowadays we have build bots everywhere, which are great at
> finding such edge cases.
>
> I get the feeling this option has been responsible for dozens of
> "convert inline to __always_inline" patches over the years. Such
> patches usually complain about the compiler's inlining decisions being
> unpredictable. Turns out the culprit is this
> CONFIG_DEBUG_SECTION_MISMATCH option, at least for once-called
> functions.
>
> This option really has no benefits, and only drawbacks, causing needless
> churn. Remove it.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> Makefile | 5 -----
> arch/arc/configs/tb10x_defconfig | 1 -
> arch/nds32/configs/defconfig | 1 -
> arch/s390/configs/debug_defconfig | 1 -
> arch/s390/configs/defconfig | 1 -
> lib/Kconfig.debug | 21 -------------------
> .../selftests/wireguard/qemu/debug.config | 1 -
> 7 files changed, 31 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c19d1638da25..195d4f4914e3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -913,11 +913,6 @@ KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING)
> KBUILD_AFLAGS += $(CC_FLAGS_USING)
> endif
>
> -# We trigger additional mismatches with less inlining
> -ifdef CONFIG_DEBUG_SECTION_MISMATCH
> -KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
> -endif
> -
> ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
> KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections
> LDFLAGS_vmlinux += --gc-sections
> diff --git a/arch/arc/configs/tb10x_defconfig b/arch/arc/configs/tb10x_defconfig
> index a12656ec0072..5acf8cc3e7b0 100644
> --- a/arch/arc/configs/tb10x_defconfig
> +++ b/arch/arc/configs/tb10x_defconfig
> @@ -96,7 +96,6 @@ CONFIG_STRIP_ASM_SYMS=y
> CONFIG_DEBUG_FS=y
> CONFIG_HEADERS_INSTALL=y
> CONFIG_HEADERS_CHECK=y
> -CONFIG_DEBUG_SECTION_MISMATCH=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_MEMORY_INIT=y
> CONFIG_DEBUG_STACKOVERFLOW=y
> diff --git a/arch/nds32/configs/defconfig b/arch/nds32/configs/defconfig
> index f9a89cf00aa6..61b5b83bd20e 100644
> --- a/arch/nds32/configs/defconfig
> +++ b/arch/nds32/configs/defconfig
> @@ -93,7 +93,6 @@ CONFIG_GDB_SCRIPTS=y
> CONFIG_READABLE_ASM=y
> CONFIG_HEADERS_INSTALL=y
> CONFIG_HEADERS_CHECK=y
> -CONFIG_DEBUG_SECTION_MISMATCH=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_KERNEL=y
> CONFIG_PANIC_ON_OOPS=y
> diff --git a/arch/s390/configs/debug_defconfig b/arch/s390/configs/debug_defconfig
> index b88184019af9..68104f82cc9d 100644
> --- a/arch/s390/configs/debug_defconfig
> +++ b/arch/s390/configs/debug_defconfig
> @@ -782,7 +782,6 @@ CONFIG_DEBUG_INFO=y
> CONFIG_DEBUG_INFO_DWARF4=y
> CONFIG_GDB_SCRIPTS=y
> CONFIG_HEADERS_INSTALL=y
> -CONFIG_DEBUG_SECTION_MISMATCH=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_PAGEALLOC=y
> CONFIG_PAGE_OWNER=y
> diff --git a/arch/s390/configs/defconfig b/arch/s390/configs/defconfig
> index 1667a3cdcf0a..a5e7eb0276c6 100644
> --- a/arch/s390/configs/defconfig
> +++ b/arch/s390/configs/defconfig
> @@ -767,7 +767,6 @@ CONFIG_DYNAMIC_DEBUG=y
> CONFIG_DEBUG_INFO=y
> CONFIG_DEBUG_INFO_DWARF4=y
> CONFIG_GDB_SCRIPTS=y
> -CONFIG_DEBUG_SECTION_MISMATCH=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_WX=y
> CONFIG_PTDUMP_DEBUGFS=y
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e5cdf98f50c2..93f9c00783b2 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -381,27 +381,6 @@ config HEADERS_INSTALL
> user-space program samples. It is also needed by some features such
> as uapi header sanity checks.
>
> -config DEBUG_SECTION_MISMATCH
> - bool "Enable full Section mismatch analysis"
> - help
> - The section mismatch analysis checks if there are illegal
> - references from one section to another section.
> - During linktime or runtime, some sections are dropped;
> - any use of code/data previously in these sections would
> - most likely result in an oops.
> - In the code, functions and variables are annotated with
> - __init,, etc. (see the full list in include/linux/init.h),
> - which results in the code/data being placed in specific sections.
> - The section mismatch analysis is always performed after a full
> - kernel build, and enabling this option causes the following
> - additional step to occur:
> - - Add the option -fno-inline-functions-called-once to gcc commands.
> - When inlining a function annotated with __init in a non-init
> - function, we would lose the section information and thus
> - the analysis would not catch the illegal reference.
> - This option tells gcc to inline less (but it does result in
> - a larger kernel).
> -
> config SECTION_MISMATCH_WARN_ONLY
> bool "Make section mismatch errors non-fatal"
> default y
> diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config
> index fe07d97df9fa..83588bb08fde 100644
> --- a/tools/testing/selftests/wireguard/qemu/debug.config
> +++ b/tools/testing/selftests/wireguard/qemu/debug.config
> @@ -57,7 +57,6 @@ CONFIG_USER_STACKTRACE_SUPPORT=y
> CONFIG_DEBUG_SG=y
> CONFIG_DEBUG_NOTIFIERS=y
> CONFIG_X86_DEBUG_FPU=y
> -CONFIG_DEBUG_SECTION_MISMATCH=y
> CONFIG_DEBUG_PAGEALLOC=y
> CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y
> --
> 2.31.1
>