Date:   Wed, 25 Aug 2021 14:16:01 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, joro@...tes.org,
        boris.ostrovsky@...cle.com, jgross@...e.com, x86@...nel.org,
        mbenes@...e.com, rostedt@...dmis.org, dvyukov@...gle.com,
        elver@...gle.com, sam@...nborg.org, masahiroy@...nel.org,
        hca@...ux.ibm.com, nickhu@...estech.com, vgupta@...nel.org
Subject: Re: [PATCH v2 06/24] x86: Always inline
 context_tracking_guest_enter()

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@...hat.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@...hat.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
> 

Powered by blists - more mailing lists