x86: macrofying inline asm

This patch-set deals with an interesting yet stupid problem: kernel code that does not get inlined despite its simplicity. There are several causes for this behavior: "cold" attribute on __init, different function optimization levels; conditional constant computations based on __builtin_constant_p(); and finally large inline assembly blocks.

This patch-set deals with the inline assembly problem. I separated these patches from the others (that were sent in the RFC) for easier inclusion. I also separated the removal of unnecessary new-lines which would be sent separately.

The problem with inline assembly is that inline assembly is often used by the kernel for things that are other than code - for example, assembly directives and data. GCC however is oblivious to the content of the blocks and assumes their cost in space and time is proportional to the number of the perceived assembly "instruction", according to the number of newlines and semicolons. Alternatives, paravirt and other mechanisms are affected, causing code not to be inlined, and degrading compilation quality in general.

The solution that this patch-set carries for this problem is to create an assembly macro, and then call it from the inline assembly block. As a result, the compiler sees a single "instruction" and assigns the more appropriate cost to the code.

To avoid uglification of the code, as many noted, the macros are first precompiled into an assembly file, which is later assembled together with the C files. This also enables to avoid duplicate implementation that was set before for the asm and C code. This can be seen in the exception table changes.

Overall this patch-set slightly increases the kernel size (my build was done using my Ubuntu 18.04 config + localyesconfig for the record):

text data bss dec hex filename
18140829 10224724 2957312 31322865 1ddf2f1 ./vmlinux before 18163608 10227348 2957312 31348268 1de562c ./vmlinux after (+0.1%)

The number of static functions in the image is reduced by 379, but actually inlining is even better, which does not always shows in these
numbers: a function may be inlined causing the calling function not to be inlined.

I ran some limited number of benchmarks, and in general the performance impact is not very notable. You can still see >10 cycles shaved off some syscalls that manipulate page-tables (e.g., mprotect()), in which paravirt caused many functions not to be inlined. In addition this patch-set can prevent issues such as [1], and improves code readability and maintainability.

Update: Rasmus recently caused me (inadvertently) to become paranoid about the dependencies. To clarify: if any of the headers changes, any c file which uses macros that are included in macros.S would be fine as long as it includes the header as well (as it should). Adding an assertion to check this is done might become slightly ugly, and nobody else is concerned about it. Another minor issue is that changes of macros.S would not trigger a global rebuild, but that is pretty similar to changes of the Makefile that do not trigger a rebuild.

[1] https://patchwork.kernel.org/patch/10450037/

v8->v9: * Restoring the '-pipe' parameter (Rasmus)
* Adding Kees's tested-by tag (Kees)

v7->v8: * Add acks (Masahiro, Max)
* Rebase on 4.19 (Ingo)

v6->v7: * Fix context switch tracking (Ingo)
* Fix xtensa build error (Ingo)
* Rebase on 4.18-rc8

v5->v6: * Removing more code from jump-labels (PeterZ)
* Fix build issue on i386 (0-day, PeterZ)

v4->v5: * Makefile fixes (Masahiro, Sam)

v3->v4: * Changed naming of macros in 2 patches (PeterZ)
* Minor cleanup of the paravirt patch

v2->v3: * Several build issues resolved (0-day)
* Wrong comments fix (Josh)
* Change asm vs C order in refcount (Kees)

v1->v2: * Compiling the macros into a separate .s file, improving
readability (Linus)
* Improving assembly formatting, applying most of the comments according to my judgment (Jan)
* Adding exception-table, cpufeature and jump-labels
* Removing new-line cleanup; to be submitted separately

Nadav Amit (10):
xtensa: defining LINKER_SCRIPT for the linker script
Makefile: Prepare for using macros for inline asm
x86: objtool: use asm macro for better compiler decisions
x86: refcount: prevent gcc distortions
x86: alternatives: macrofy locks for better inlining
x86: bug: prevent gcc distortions
x86: prevent inline distortion by paravirt ops
x86: extable: use macros instead of inline assembly
x86: cpufeature: use macros instead of inline assembly
x86: jump-labels: use macros instead of inline assembly

 Makefile                               |  9 ++-
 arch/x86/Makefile                      |  7 ++
 arch/x86/entry/calling.h               |  2 +-
 arch/x86/include/asm/alternative-asm.h | 20 ++++--
 arch/x86/include/asm/alternative.h     | 11 +--
 arch/x86/include/asm/asm.h             | 61 +++++++---------
 arch/x86/include/asm/bug.h             | 98 +++++++++++++++-----------
 arch/x86/include/asm/cpufeature.h      | 82 ++++++++++++---------
 arch/x86/include/asm/jump_label.h      | 77 ++++++++------------
 arch/x86/include/asm/paravirt_types.h  | 56 +++++++--------
 arch/x86/include/asm/refcount.h        | 74 +++++++++++--------
 arch/x86/kernel/macros.S               | 16 +++++
 arch/xtensa/kernel/Makefile            |  4 +-
 include/asm-generic/bug.h              |  8 +--
 include/linux/compiler.h               | 56 +++++++++++----
 scripts/Kbuild.include                 |  4 +-
 scripts/mod/Makefile                   |  2 +
 17 files changed, 331 insertions(+), 256 deletions(-)
 create mode 100644 arch/x86/kernel/macros.S

2.17.1

Defining the LINKER_SCRIPT when building the linker script as being done in other architectures. This is required for the next Makefile changes would otherwise break things.

---
 arch/xtensa/kernel/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 91907590d183..8dff506caf07 100644
@@ -35,8 +35,8 @@ sed-y = -e ':a; s/\*(\([^)]*\)\.text\.unlikely/*(\1.literal.unlikely .{text}.unl
 	-e 's/\.{text}/.text/g'
 
 quiet_cmd__cpp_lds_S = LDS     $@
-cmd__cpp_lds_S = $(CPP) $(cpp_flags) -P -C -Uxtensa -D__ASSEMBLY__ $<    \
-                 | sed $(sed-y) >$@
+cmd__cpp_lds_S = $(CPP) $(cpp_flags) -P -C -Uxtensa -D__ASSEMBLY__ \
+		 -DLINKER_SCRIPT $< | sed $(sed-y) >$@
 
 $(obj)/vmlinux.lds: $(src)/vmlinux.lds.S FORCE
 	$(call if_changed_dep,_cpp_lds_S)

2.17.1

Using macros for inline assembly improves both readability and compilation decisions that are distorted by big assembly blocks that use alternative sections. Compile macros.S and use it to assemble all C files. Currently, only x86 will use it.

---
 Makefile                 | 9 +++++++--
 arch/x86/Makefile        | 7 +++++++
 arch/x86/kernel/macros.S | 7 +++++++
 scripts/Kbuild.include   | 4 +++-
 scripts/mod/Makefile     | 2 ++
 5 files changed, 26 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/kernel/macros.S

index 6c3da3e10f07..6c40d547513c 100644
@@ -1071,7 +1071,7 @@ scripts: scripts_basic asm-generic gcc-plugins $(autoksyms_h)
 # version.h and scripts_basic is processed / created.
 
 # Listed in dependency order
-PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
+PHONY += prepare archprepare macroprepare prepare0 prepare1 prepare2 prepare3
 
 # prepare3 is used to check if we are building in a separate output directory,
 # and if so do:

@@ -1094,7 +1094,9 @@ prepare2: prepare3 outputmakefile asm-generic
 prepare1: prepare2 $(version_h) $(autoksyms_h) include/generated/utsrelease.h
 	$(cmd_crmodverdir)
 
-archprepare: archheaders archscripts prepare1 scripts_basic
+macroprepare: prepare1 archmacros
+
+archprepare: archheaders archscripts macroprepare scripts_basic
 
 prepare0: archprepare gcc-plugins
 	$(Q)$(MAKE) $(build)=.

@@ -1162,6 +1164,9 @@ archheaders:
 PHONY += archscripts
 archscripts:
 
+PHONY += archmacros
+archmacros:
+
 PHONY += __headers
 __headers: $(version_h) scripts_basic uapi-asm-generic archheaders archscripts
 	$(Q)$(MAKE) $(build)=scripts build_unifdef

index 8f6e7eb8ae9f..18a0084f134b 100644
@@ -237,6 +237,13 @@ archscripts: scripts_basic
 archheaders:
 	$(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all
 
+archmacros:
+	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
+
+ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s -Wa,-
+export ASM_MACRO_FLAGS
+KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
+
 ###
 # Kernel objects
 

new file mode 100644
index 000000000000..cfc1c7d1a6eb
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file includes headers whose assembly part includes macros which are
+ * commonly used. The macros are precompiled into assmebly file which is later
+ * assembled together with each compiled file.
+ */

index ce53639a864a..8aeb60eb6ee3 100644
@@ -115,7 +115,9 @@ __cc-option = $(call try-run,\
 
 # Do not attempt to build with gcc plugins during cc-option tests.
 # (And this uses delayed resolution so the flags will be up to date.)
-CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
+# In addition, do not include the asm macros which are built later.
+CC_OPTION_FILTERED = $(GCC_PLUGINS_CFLAGS) $(ASM_MACRO_FLAGS)
+CC_OPTION_CFLAGS = $(filter-out $(CC_OPTION_FILTERED),$(KBUILD_CFLAGS))
 
 # cc-option
 # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)

index 42c5d50f2bcc..a5b4af47987a 100644
@@ -4,6 +4,8 @@ OBJECT_FILES_NON_STANDARD := y
 hostprogs-y	:= modpost mk_elfconfig
 always		:= $(hostprogs-y) empty.o
 
+CFLAGS_REMOVE_empty.o := $(ASM_MACRO_FLAGS)
+
 modpost-objs	:= modpost.o file2alias.o sumversion.o
 
 devicetable-offsets-file := devicetable-offsets.h

2.17.1

From: Logan Gunthorpe
Sent: November 6, 2018 at 6:57:57 PM GMT

Err.. I don’t have a dist-cc environment. I wonder whether something like that would do the trick:

-- >8 —

Subject: [PATCH] x86: Fix dist-cc

---
 arch/x86/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 5b562e464009..81d76cbcffff 100644
@@ -237,9 +237,9 @@ archheaders:
 	$(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all
 
 archmacros:
-	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
+	$(Q)$(MAKE) $(build)=arch/x86/kernel ${objtree}/arch/x86/kernel/macros.s
 
-ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s -Wa,-
+ASM_MACRO_FLAGS = -Wa,${objtree}/arch/x86/kernel/macros.s -Wa,-
 export ASM_MACRO_FLAGS
 KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
 

2.17.1

I tested it to be sure -- but, no, this does not have any affect on distcc. Distcc usually does pre-processing locally and then sends the single output of cpp to a remote for compilation and assembly. It doesn't expect there to be additional assembly files tacked on the command line and isn't going to handle that case correctly. Thus, changing the path of the file won't fix anything.

Logan

From: Logan Gunthorpe
Sent: November 6, 2018 at 8:01:31 PM GMT

Thanks for the explanation.

I played with dist-cc and gave it some thought, but I do not have a solution. I thought that perhaps some hack is possible by using the same preprocessed file can be used as both the source file and the assembly file, but that’s as far as I managed to get.

Ideas? Do people care about it?

Just spit balling, but is there a reason we didn't just put the macros for inline assembly directly in the header? Something like this:

asm(".macro ANNOTATE_UNREACHABLE counter:req\n\t"

    "\\counter:\n\t"
        ".pushsection .discard.unreachable\n\t"
        ".long \\counter\\()b -.\n\t"
        ".popsection\n\t"
    ".endm\n");

#define annotate_unreachable() ({                   \
    asm volatile("ANNOTATE_UNREACHABLE counter=%c0" \
                 : : "i" (__COUNTER__));            \
})

It does mean the macros won't be usable in non-inline assembly, without duplicating them, but do we care about that?

I would have expected people compiling the kernel with distcc to be fairly common -- it certainly speeds up my kernel compiles quite significantly and thus saves me a lot of time.

Logan

From: Logan Gunthorpe
Sent: November 7, 2018 at 6:53:02 PM GMT

HPA indicated more than once that this is wrong (and that was indeed my initial solution), since it is not guaranteed that the compiler would put the macro assembly before its use.

Hmm, that's very unfortunate. I don't really understand why the compiler would not put the macro assembly in the same order as it appears in the code and it would be in the correct order there.

In any case, I've submitted a couple of issues to icecc[1] and distcc[2] to see if they have any ideas about supporting this on their sides.

Thanks,

Logan

[1] https://github.com/icecc/icecream/issues/428
[2] https://github.com/distcc/distcc/issues/312

Apparently gcc will treat them like basic blocks and possibly move them around.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

From: [email protected]
Sent: November 7, 2018 at 9:50:28 PM GMT

Maybe it is possible to break the compilation of each object into two
stages: first, compile the source without assembly, and then take the generated .s file and assemble it with the .s file of the macros.

Does it sounds as something that may work? I guess it should only be done when distcc is used.

In theory it would at least allow the compile step to be distributed, the assembly step would still have to be done locally... It'd be better than nothing, I guess.

It'd also be difficult to know when distribution is being done and that it's necessary to split the steps. We'd have to add an environment variable or something and users would need to know they have to set it when using a distributed compile.

Logan

From: Logan Gunthorpe
Sent: November 8, 2018 at 5:14:58 PM GMT

I don’t think the assembly stage needs to be done locally. gcc can still be used to deploy the assembler. I am not too familiar with distcc, so I don’t know whether the preprocessing supports multiple source-files, and whether it has some strange-behavior when it comes to .S/.s files.

Well, I’ll give it a try.

The problem is that it's the assembly stage that needs the extra macros.s and without that file being transferred somehow, that stage must be done locally.

The distcc guys have a thought that the macros.s file might be able to be transferred using pump mode with a change to include_server.py. I don't think that will work for icecc though.

Logan

From: Logan Gunthorpe
Sent: November 8, 2018 at 8:00:33 PM GMT

I understand, but the idea is to have two stages, for instance:

gcc ... -S -o .tmp_[unit].S [unit].c

and then

gcc ... -D__ASSEMBLY__ arch/x86/kernel/macros.S .tmp_[unit].S

(Yes, I realize the .tmp_[unit].S was already preprocessed, but this way you can combine both inputs)

Unfortunately, as I write this email (and run tests) I realize distcc is too dumb to handle two input files:

"(dcc_scan_args) do we have two inputs? i give up "

Just great. So I guess macros.s would need to be concatenated with .tmp_[unit].s as a separate (local) interim stage.

From: Nadav Amit
Sent: November 8, 2018 at 8:18:23 PM GMT

Err.. I hate makefiles and distcc doesn’t make life easier. For instance, if it sees two source files on the command-line, it freaks out and compiles it locally. It also has an option to distribute assembly, but it is not enabled by default.

Anyhow, can you try the following patch?

-- >8 --

Subject: [PATCH] Makefile: Fix distcc compilation with x86 macros

Introducing the use of asm macros in c-code broke distcc, since it only sends the preprocessed source file. The solution is to break the compilation into two separate phases of compilation and assembly, and between the two concatanate the assembly macros and the compiled (yet not assembled) source file. Since this is less efficient, this compilation mode is only used when make is called with the "DISTCC=y" parameter.

Note that the assembly stage should also be distributable, if distcc is configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE".

---
 Makefile               |  4 +++-
 arch/x86/Makefile      |  4 +++-
 scripts/Makefile.build | 29 +++++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 4 deletions(-)

index 9fce8b91c15f..c07349fc38c7 100644
@@ -743,7 +743,9 @@ KBUILD_CFLAGS   += $(call cc-option, -gsplit-dwarf, -g)
 else
 KBUILD_CFLAGS	+= -g
 endif
-KBUILD_AFLAGS	+= -Wa,-gdwarf-2
+AFLAGS_DEBUG_INFO = -Wa,-gdwarf-2
+export AFLAGS_DEBUG_INFO
+KBUILD_AFLAGS	+= $(AFLAGS_DEBUG_INFO)
 endif
 ifdef CONFIG_DEBUG_INFO_DWARF4
 KBUILD_CFLAGS	+= $(call cc-option, -gdwarf-4,)

index f5d7f4134524..080bd9cbc4e1 100644
@@ -238,7 +238,9 @@ archheaders:
 archmacros:
 	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
 
-ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
+ASM_MACRO_FILE = arch/x86/kernel/macros.s
+export ASM_MACRO_FILE
+ASM_MACRO_FLAGS = -Wa,$(ASM_MACRO_FILE)
 export ASM_MACRO_FLAGS
 KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
 

index 6a6be9f440cf..2b79789a3e10 100644
@@ -155,8 +155,33 @@ $(obj)/%.ll: $(src)/%.c FORCE
 
 quiet_cmd_cc_o_c = CC $(quiet_modtag)  $@
 
+# If distcc is used, then when an assembly macro files is needed, the
+# compilation stage and the assembly stage need to be separated. Providing
+# "DISTCC=y" option enables the separate compilation and assembly.
+cmd_cc_o_c_simple = $(CC) $(c_flags) -c -o $(1) $<
+
+ifeq ($(DISTCC),y)
+a_flags_no_debug = $(filter-out $(AFLAGS_DEBUG_INFO), $(a_flags))
+c_flags_no_macros = $(filter-out $(ASM_MACRO_FLAGS), $(c_flags))
+
+cmd_cc_o_c_two_steps =							\
+	$(CC) $(c_flags_no_macros) $(DISABLE_LTO) -fverbose-asm -S 	\
+		-o $(@D)/.$(@F:.o=.s) $< ;				\
+	cat $(ASM_MACRO_FILE) $(@D)/.$(@F:.o=.s) > 			\
+		$(@D)/.tmp_$(@F:.o=.s);					\
+	$(CC) $(a_flags_no_debug) -c -o $(1) $(@D)/.tmp_$(@F:.o=.s) ; 	\
+	rm -f $(@D)/.$(@F:.o=.s) $(@D)/.tmp_$(@F:.o=.s)			\
+
+cmd_cc_o_c_helper =    							\
+	$(if $(findstring $(ASM_MACRO_FLAGS),$(c_flags)),		\
+		$(call cmd_cc_o_c_two_steps, $(1)),			\
+		$(call cmd_cc_o_c_simple, $(1)))
+else
+cmd_cc_o_c_helper = $(call cmd_cc_o_c_simple, $(1))
+endif
+
 ifndef CONFIG_MODVERSIONS
-cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
+cmd_cc_o_c = $(call cmd_cc_o_c_helper,$@)
 
 else
 # When module versioning is enabled the following steps are executed:

@@ -171,7 +196,7 @@ else
 #   replace the unresolved symbols __crc_exported_symbol with
 #   the actual value of the checksum generated by genksyms
 
-cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
+cmd_cc_o_c = $(call cmd_cc_o_c_helper,$(@D)/.tmp_$(@F))
 
 cmd_modversions_c =								\
 	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\

2.17.1

Very nice, great work! It works as expected. I tested with both distcc and icecc and both work well with DISTCC=y.

Also worth noting, is that distcc has a patch to work without DISTCC=y in pump mode (though pump mode doesn't work super great with the kernel as some of the include processing takes a long time). When I have some free time, I may also try to patch icecc to work with it, as in my experience, it performs better.

Regardless, I think your patch would be very valuable to be in upstream so there's at least some way to use the existing versions of distcc and icecc.

Thanks!

Logan

GCC considers the number of statements in inlined assembly blocks, according to new-lines and semicolons, as an indication to the cost of the block in time and space. This data is distorted by the kernel code, which puts information in alternative sections. As a result, the compiler may perform incorrect inlining and branch optimizations.

In the case of objtool, this distortion is extreme, since anyhow the annotations of objtool are discarded during linkage.

The solution is to set an assembly macro and call it from the inline assembly block. As a result GCC considers the inline assembly block as a single instruction.

This patch slightly increases the kernel size.

text data bss dec hex filename
18140829 10224724 2957312 31322865 1ddf2f1 ./vmlinux before 18140970 10225412 2957312 31323694 1ddf62e ./vmlinux after (+829)

Static text symbols:
Before: 40321
After: 40302 (-19)

---
 arch/x86/kernel/macros.S |  2 ++
 include/linux/compiler.h | 56 ++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 13 deletions(-)

index cfc1c7d1a6eb..cee28c3246dc 100644
@@ -5,3 +5,5 @@
  * commonly used. The macros are precompiled into assmebly file which is later
  * assembled together with each compiled file.
  */
+
+#include <linux/compiler.h>

2.17.1

GCC considers the number of statements in inlined assembly blocks, according to new-lines and semicolons, as an indication to the cost of the block in time and space. This data is distorted by the kernel code, which puts information in alternative sections. As a result, the compiler may perform incorrect inlining and branch optimizations.

The solution is to set an assembly macro and call it from the inlined assembly block. As a result GCC considers the inline assembly block as a single instruction.

This patch allows to inline functions such as __get_seccomp_filter(). Interestingly, this allows more aggressive inlining while reducing the kernel size.

text data bss dec hex filename
18140970 10225412 2957312 31323694 1ddf62e ./vmlinux before 18140140 10225284 2957312 31322736 1ddf270 ./vmlinux after (-958)

Static text symbols:
Before: 40302
After: 40286 (-16)

Functions such as kref_get(), free_user(), fuse_file_get() now get inlined.

---
 arch/x86/include/asm/refcount.h | 74 ++++++++++++++++++++-------------
 arch/x86/kernel/macros.S        |  1 +
 2 files changed, 46 insertions(+), 29 deletions(-)

index 19b90521954c..c92909da0686 100644
@@ -4,6 +4,41 @@
  * x86-specific implementation of refcount_t. Based on PAX_REFCOUNT from
  * PaX/grsecurity.
  */
+
+#ifdef __ASSEMBLY__
+
+#include <asm/asm.h>
+#include <asm/bug.h>
+
+.macro REFCOUNT_EXCEPTION counter:req
+	.pushsection .text..refcount
+111:	lea \counter, %_ASM_CX
+112:	ud2
+	ASM_UNREACHABLE
+	.popsection
+113:	_ASM_EXTABLE_REFCOUNT(112b, 113b)
+.endm
+
+/* Trigger refcount exception if refcount result is negative. */
+.macro REFCOUNT_CHECK_LT_ZERO counter:req
+	js 111f
+	REFCOUNT_EXCEPTION counter="\counter"
+.endm
+
+/* Trigger refcount exception if refcount result is zero or negative. */
+.macro REFCOUNT_CHECK_LE_ZERO counter:req
+	jz 111f
+	REFCOUNT_CHECK_LT_ZERO counter="\counter"
+.endm
+
+/* Trigger refcount exception unconditionally. */
+.macro REFCOUNT_ERROR counter:req
+	jmp 111f
+	REFCOUNT_EXCEPTION counter="\counter"
+.endm
+
+#else /* __ASSEMBLY__ */
+
 #include <linux/refcount.h>
 #include <asm/bug.h>
 

@@ -15,34 +50,11 @@
  * central refcount exception. The fixup address for the exception points
  * back to the regular execution flow in .text.
  */
-#define _REFCOUNT_EXCEPTION				\
-	".pushsection .text..refcount\n"		\
-	"111:\tlea %[counter], %%" _ASM_CX "\n"		\
-	"112:\t" ASM_UD2 "\n"				\
-	ASM_UNREACHABLE					\
-	".popsection\n"					\
-	"113:\n"					\
-	_ASM_EXTABLE_REFCOUNT(112b, 113b)
-
-/* Trigger refcount exception if refcount result is negative. */
-#define REFCOUNT_CHECK_LT_ZERO				\
-	"js 111f\n\t"					\
-	_REFCOUNT_EXCEPTION
-
-/* Trigger refcount exception if refcount result is zero or negative. */
-#define REFCOUNT_CHECK_LE_ZERO				\
-	"jz 111f\n\t"					\
-	REFCOUNT_CHECK_LT_ZERO
-
-/* Trigger refcount exception unconditionally. */
-#define REFCOUNT_ERROR					\
-	"jmp 111f\n\t"					\
-	_REFCOUNT_EXCEPTION
 
 static __always_inline void refcount_add(unsigned int i, refcount_t *r)
 {
 	asm volatile(LOCK_PREFIX "addl %1,%0\n\t"
-		REFCOUNT_CHECK_LT_ZERO
+		"REFCOUNT_CHECK_LT_ZERO counter=\"%[counter]\""
 		: [counter] "+m" (r->refs.counter)
 		: "ir" (i)
 		: "cc", "cx");

@@ -51,7 +63,7 @@ static __always_inline void refcount_add(unsigned int i, refcount_t *r)
 static __always_inline void refcount_inc(refcount_t *r)
 {
 	asm volatile(LOCK_PREFIX "incl %0\n\t"
-		REFCOUNT_CHECK_LT_ZERO
+		"REFCOUNT_CHECK_LT_ZERO counter=\"%[counter]\""
 		: [counter] "+m" (r->refs.counter)
 		: : "cc", "cx");
 }

@@ -59,7 +71,7 @@ static __always_inline void refcount_inc(refcount_t *r)
 static __always_inline void refcount_dec(refcount_t *r)
 {
 	asm volatile(LOCK_PREFIX "decl %0\n\t"
-		REFCOUNT_CHECK_LE_ZERO
+		"REFCOUNT_CHECK_LE_ZERO counter=\"%[counter]\""
 		: [counter] "+m" (r->refs.counter)
 		: : "cc", "cx");
 }

@@ -67,13 +79,15 @@ static __always_inline void refcount_dec(refcount_t *r)
 static __always_inline __must_check
 bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 {
-	GEN_BINARY_SUFFIXED_RMWcc(LOCK_PREFIX "subl", REFCOUNT_CHECK_LT_ZERO,
+	GEN_BINARY_SUFFIXED_RMWcc(LOCK_PREFIX "subl",
+				  "REFCOUNT_CHECK_LT_ZERO counter=\"%0\"",
 				  r->refs.counter, "er", i, "%0", e, "cx");
 }
 
 static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
 {
-	GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl", REFCOUNT_CHECK_LT_ZERO,
+	GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
+				 "REFCOUNT_CHECK_LT_ZERO counter=\"%0\"",
 				 r->refs.counter, "%0", e, "cx");
 }
 

@@ -91,7 +105,7 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 
 		/* Did we try to increment from/to an undesirable state? */
 		if (unlikely(c < 0 || c == INT_MAX || result < c)) {
-			asm volatile(REFCOUNT_ERROR
+			asm volatile("REFCOUNT_ERROR counter=\"%[counter]\""
 				     : : [counter] "m" (r->refs.counter)
 				     : "cc", "cx");
 			break;

@@ -107,4 +121,6 @@ static __always_inline __must_check bool refcount_inc_not_zero(refcount_t *r)
 	return refcount_add_not_zero(1, r);
 }
 
+#endif /* __ASSEMBLY__ */
+
 #endif

index cee28c3246dc..f1fe1d570365 100644
@@ -7,3 +7,4 @@
  */
 
 #include <linux/compiler.h>
+#include <asm/refcount.h>

2.17.1

Yeah, so I kind of had your series on the back-burner (I'm sure you noticed!), mostly because what I complained about in a previous round of review a couple of months ago: that the description of the series and the changelog of every single patch in it is tiptoeing around the *real* problem and never truly describes it:

** This is a GCC bug, plain and simple, and we are uglifying ** ** and complicating kernel assembly code to work it around. **

We'd never ever consider such uglification for Clang, not even _close_.

Sure this would have warranted a passing mention? Instead the changelogs are lovingly calling it a "distortion" as if this was no-one's fault really, and the patch a "solution".

How about calling it a "GCC inlining bug" and a "workaround with costs" which it is in reality, and stop whitewashing the problem?

At the same time I realize that we still need this series because GCC won't get fixed, so as a consolation I wrote the changelog below that explains how it really is, no holds barred.

Since the tone of the changelog is a bit ... frosty, I added this disclaimer:

[ mingo: Wrote new changelog. ]

Let me know if you want me to make it more prominent that you had absolutely no role in writing that changelog.

I'm also somewhat annoyed at the fact that this series carries a boatload of reviewed-by's and acked-by's, yet none of those reviewers found it important to point out the large chasm that is gaping between description and reality.

Thanks,

Ingo

=============>
Subject: x86/refcount: Prevent inlining related GCC distortions
From: Nadav Amit <[email protected]>
Date: Wed, 3 Oct 2018 14:30:54 -0700

The inlining pass of GCC doesn't include an assembler, so it's not aware of basic properties of the generated code, such as its size in bytes, or that there are such things as discontiuous blocks of code and data due to the newfangled linker feature called 'sections' ...

Instead GCC uses a lazy and fragile heuristic: it does a linear count of certain syntactic and whitespace elements in inlined assembly block source code, such as a count of new-lines and semicolons (!), as a poor substitute for "code size and complexity".

Unsurprisingly this heuristic falls over and breaks its neck whith certain common types of kernel code that use inline assembly, such as the frequent practice of putting useful information into alternative sections.

As a result of this fresh, 20+ years old GCC bug, GCC's inlining decisions are effectively disabled for inlined functions that make use of such asm() blocks, because GCC thinks those sections of code are "large" - when in reality they are often result in just a very low number of machine instructions generated.

This absolute lack of inlining provess when GCC comes across such asm() blocks both increases generated kernel code size and causes performance overhead, which is particularly noticeable on paravirt kernels, which make frequent use of these inlining facilities in attemt to stay out of the way when running on baremetal hardware.

Instead of fixing the compiler we use a workaround: we set an assembly macro and call it from the inlined assembly block. As a result GCC considers the inline assembly block as a single instruction. (Which it often isn't but I digress.)

This uglifies and bloats the source code:

  2 files changed, 46 insertions(+), 29 deletions(-)

Yay readability and maintainability, it's not like assembly code is hard to read and maintain ...

This patch allows GCC to inline simple functions such as __get_seccomp_filter().

To no-one's surprise the result is GCC performs more aggressive (read: correct) inlining decisions in these senarios, which reduces the kernel size and presumably also speeds it up:

      text     data     bss      dec     hex  filename
  18140970 10225412 2957312 31323694 1ddf62e  ./vmlinux before
  18140140 10225284 2957312 31322736 1ddf270  ./vmlinux after (-958)

Change in size of static text symbols:

Before: 40302

    After: 40286 (-16)

Functions such as kref_get(), free_user(), fuse_file_get() now get inlined. Hurray!

We also hope that GCC will eventually get fixed, but we are not holding our breath for that. Yet we are optimistic, it might still happen, any decade now.

[ mingo: Wrote new changelog. ]

---

Another problem I just realized is that we now include arch/x86/kernel/macros.S in every translation pass when building the kernel, right?

But arch/x86/kernel/macros.S expands to a pretty large hiearchy of header files:

  $ make arch/x86/kernel/macros.s

  $ cat $(grep include arch/x86/kernel/macros.s | cut -d\" -f2 | sort | uniq) | wc -l
  4128

That's 4,100 extra lines of code to be preprocessed for every translation unit, of which there are tens of thousands. More if other pieces of code get macrofied in this fasion in the future.

If we assume that a typical distribution kernel build has ~20,000 translation units then this change adds 82,560,000 more lines to be preprocessed, just to work around a stupid GCC bug?

I'm totally unhappy about that. Can we do this without adding macros.S?

It's also a pretty stupidly central file anyway that moves source code away from where it's used.

Thanks,

Ingo

It's not just for working around a stupid GCC bug, but it also has a huge potential for cleaning up the inline asm in general.

I would like to know if there is an actual number for the build overhead (an actual benchmark); I have asked for that once already.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Sorry but that's just plain false. For example this patch:

x86: cpufeature: use macros instead of inline assembly

... adds an extra macro indirection layer called STATIC_CPU_HAS, just to macrofy a single asm() statement that was perfectly readable and on-topic already...

There are some other cases where macrofying is also a cleanup due to sharing and naming a common pattern of code, but this is by no means an absolute quality of this approach.

Thanks,

Ingo

I can run some tests. (@hpa: I thought you asked about the -pipe overhead; perhaps I misunderstood).

I guess you regard to the preprocessing of the assembler. Note that the C preprocessing of macros.S obviously happens only once. That’s the reason I assumed it’s not that expensive.

Anyhow, I remember that we discussed at some point doing something like ‘asm(“.include XXX.s”)’ and somebody said it is not good, but I don’t remember why and don’t see any reason it is so. Unless I am missing something, I think it is possible to take each individual header and preprocess the assembly part of into a separate .s file. Then we can put in the C part of the header ‘asm(".include XXX.s”)’.

What do you think?

Ingo: I wasn't talking necessarily about the specifics of each bit, but rather the general concept about being able to use macros in inlines... I can send you something I have been working on in the background, but have been holding off on because of this, in the morning my time.

But that's no excuse for not doing it right.

Global asm() statements are problematic because there is no guarantee where in the assembly source they will end up. You'd almost need to intercept the assembly output, move all the includes to the top, and then process...

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Ok, agreed about that part - and some of the patches did improve readability.

Also, the 275 lines macros.s is a lot nicer than the 4,200 lines macros.S.

Also, I'm not against using workarounds when the benefits are larger than the costs, but I am against *hiding* the fact that these are workarounds and that for some of them there are costs.

BTW., I have applied most of the series to tip:x86/kbuild already, and will push them out later today after some testing. I didn't apply the final 3 patches as they have dependencies, but applied the basics and fixed up the changelogs.

So you can rely on this.

Thanks,

Ingo

Agreed, of course.

Wonderful.

Here is the horrible code I mentioned yesterday. This is about implementing the immediate-patching framework that Linus and others have discussed (it helps both performance and kernel hardening):

Warning: this stuff can cause serious damage to your eyes, and this is a just a small chunk of the whole mess; and relying on gas macros, as brain damaged as they are, really is much, much cleaner than not:

	http://www.zytor.com/~hpa/foo.S

	-hpa

Funny. Immediate-patching is what I was playing with when I encountered the gcc issue. Performance got worse instead of improving (or at least staying the same), because inlining got crazy.

Anyhow, wait for my soon-to-be-sent RFC in which I define a macro called “call” (to reduce the retpoline overhead) before you talk about damage to the eyes.

Doing it well is *hard*, as I discovered. The actual patch is much, much, larger.

-hpa

I'm wondering if a production version should look more like:

patch_point:
mov $whatever, [target]
.pushsection ".immediate_patches"
.quad .Lpatch_point
.popsection

and let objtool parse the resulting assembled output and emit an entry in some table mapping patch_point to the actual address and size of the immediate to be patched.

Putting the generation of the ersatz code in objtool is an interesting idea, although I'm wondering if these macros, as awful as they are, wouldn't have generic applicability (what they do is they allow the cold branch of an asm to push temporaries to the stack rather than having to have gcc clobber them.)

I'll think about it.

-hpa

Heh, I did a POC in userspace some years ago for loading an "eventually constant" value into a register - the idea being to avoid a load in cases like kmemcache_alloc(foo_cachep) or kmemcache_free(foo_cachep, p), and I assume this is something along the same lines? I didn't do anything with it since I had no idea if the performance gain would be worth it, and at the time (before __ro_after_init) there was no good way to know that the values would really be constant eventually. Also, I had hoped to come up with a way to avoid having to annotate the loads.

I just tried expanding this to deal with some of the hash tables sized at init time which I can see was also mentioned on LKML some time ago. I'm probably missing something fundamental, but there's some sorta working code at https://github.com/villemoes/rai which is not too horrible (IMO). Attaching gcc at various times and doing disassembly shows that the patching does take place. Can I get you to take a look at raimacros.S and tell me why that won't work?

Thanks,
Rasmus

Well, tests are unlikely to show the overhead of extra lines of this magnitude, unless done very carefully, yet the added bloat exists and is not even mentioned by the changelog, it just says:

Subject: [PATCH v9 02/10] Makefile: Prepare for using macros for inline asm

Using macros for inline assembly improves both readability and compilation decisions that are distorted by big assembly blocks that use alternative sections. Compile macros.S and use it to assemble all C files. Currently, only x86 will use it.

True - so first we build macros.s, and that gets included in every C file build, right?

macros.s is smaller: 275 lines only in the distro test build I tried, which looks a lot better than my first 4,200 lines guesstimate.

Hm, this looks quite complex - macros.s is better I think. Also, 275 straight assembly lines is a lot better than 4,200.

Another, separate question I wanted to ask: how do we ensure that the kernel stays fixed? I.e. is there some tooling we can use to actually measure whether there's bad inlining decisions done, to detect all these bad patterns that cause bad GCC code generation?

Thanks,

Ingo

The assembly output from GCC is quite volumious; I doubt tacking a few hundred lines on will matter one iota.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Right.

I’m really not into it, and hpa reminded me why it wouldn’t work. For some reason I thought the order of macros doesn’t matter in asm (I probably should go to sleep).

Good question. First, I’ll indicate that this patch-set does not handle all the issues. There is still the issue of conditional use of __builtin_constant_p().

One indication for bad inlining decisions is the inlined functions have multiple (non-inlined) instances in the binary and are short. I don’t have an automatic solution, but you can try, for example to run:

nm --print-size ./vmlinux | grep ' t ' | cut -d' ' -f2- | sort | uniq -c | \
	grep -v '^      1' | sort -n -r | head -n 5

There are however many false positives. After these patches, for example, I get:

     11 000000000000012f t jhash
      7 0000000000000017 t dst_output
      6 0000000000000011 t kzalloc
      5 000000000000002f t acpi_os_allocate_zeroed
      5 0000000000000029 t acpi_os_allocate

jhash() should not have been inlined in my mind, and should have a non-inlined implementation. dst_output() is used as a function pointer. kzalloc() and the next two suffer from the __builtin_constant_p() problem I described in the past.

Regards,
Nadav

Ok, that's useful info.

The histogram suggests that with all your patches applied the kernel is now in a pretty good state in terms of inlining decisions, right?

Are you using defconfig or a reasonable distro-config for your tests?

Thanks,

Ingo

It was just an example that I ran on the kernel I built right now (with a custom config). Please don’t regard these results as anything indicative.

I think it is best to take the kernel and run localyesconfig for testing.

Taking Ubuntu 18.04 and doing the same gives the following results:

     12 000000000000012f t jhash
      7 0000000000000017 t dst_output
      7 0000000000000014 t init_once
      5 00000000000000d8 t jhash2
      5 000000000000004e t put_page
      5 000000000000002f t acpi_os_allocate_zeroed
      5 0000000000000029 t acpi_os_allocate
      5 0000000000000028 t umask_show
      5 0000000000000011 t kzalloc
      4 0000000000000053 t trace_xhci_dbg_quirks

Not awful, but not great.

It is a bit hard to fix the __builtin_constant_p() problem without having some negative side-effects.

Reminder: __builtin_constant_p() is evaluated after the inlining decision are done. You can use __builtin_choose_expr() instead of an “if”s and instead of ternary operators when evaluating __builtin_constant_p() to solve the problem. However, this causes the compiler not to know sometimes that a value is constant because __builtin_choose_expr () evaluation happens too early. This __builtin_choose_expr() problem is the reason for put_page() and kzalloc() are not being inlined. Clang, again, does not suffer from this problem.

Anyhow, it may be a good practice to try to get rid of the rest. For example, dst_discard() has four instances because it is always given as a function pointer. So it should not have been inlined.

Regards,
Nadav

Ok, agreed - and this makes the numbers you provided pretty representative.

Good - now that all of my concerns were addressed I'd like to merge the remaining 3 patches as well - but they are conflicting with ongoing x86 work in tip:x86/core. The extable conflict is trivial, the jump-label conflict a bit more involved.

Could you please pick up the updated changelogs below and resolve the conflicts against tip:master or tip:x86/build and submit the remaining patches as well?

Thanks,

Ingo

=============>
commit b82b0b611740c7c88050ba743c398af7eb920029

Author: Nadav Amit <[email protected]>
Date:   Wed Oct 3 14:31:00 2018 -0700

x86/jump-labels: Macrofy inline assembly code to work around GCC inlining bugs

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is also a minor cleanup for the jump-label code.

As a result the code size is slightly increased, but inlining decisions are better:

text data bss dec hex filename
18163528 10226300 2957312 31347140 1de51c4 ./vmlinux before 18163608 10227348 2957312 31348268 1de562c ./vmlinux after (+1128)

And functions such as intel_pstate_adjust_policy_max(), kvm_cpu_accept_dm_intr(), kvm_register_readl() are inlined.

commit dfc243615d43bb477d1d16a0064fc3d69ade5b3a

Author: Nadav Amit <[email protected]>
Date:   Wed Oct 3 14:30:59 2018 -0700

x86/cpufeature: Macrofy inline assembly code to work around GCC inlining bugs

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is pretty pointless indirection in the static_cpu_has() case, but is worth it to improve overall inlining quality.

The patch slightly increases the kernel size:

text data bss dec hex filename
18162879 10226256 2957312 31346447 1de4f0f ./vmlinux before 18163528 10226300 2957312 31347140 1de51c4 ./vmlinux after (+693)

And enables the inlining of function such as free_ldt_pgtables().

commit 4021bdcd351fd63d8d5e74264ee18d09388f0221

Author: Nadav Amit <[email protected]>
Date:   Wed Oct 3 14:30:58 2018 -0700

x86/extable: Macrofy inline assembly code to work around GCC inlining bugs

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is also a minor cleanup for the exception table code.

Text size goes up a bit:

text data bss dec hex filename
18162555 10226288 2957312 31346155 1de4deb ./vmlinux before 18162879 10226256 2957312 31346447 1de4f0f ./vmlinux after (+292)

But this allows the inlining of functions such as nested_vmx_exit_reflected(), set_segment_reg(), __copy_xstate_to_user() which is a net benefit.

FWIW, gcc guys are not too averse to the idea of enhancing gcc inline asm syntax with a statement which specifies its size so that gcc can use it to do better inlining cost estimation than counting lines.

Lemme work the process ...

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Thanks!

Ingo

These are the last 3 patches from the "macrofying inline asm" series after rebasing on TIP, per Ingo's request.

For the record: the new commit log was written by Ingo.

Nadav Amit (3):
x86/extable: Macrofy inline assembly code to work around GCC inlining
bugs
x86/cpufeature: Macrofy inline assembly code to work around GCC
inlining bugs
x86/jump-labels: Macrofy inline assembly code to work around GCC
inlining bugs

 arch/x86/entry/calling.h          |  2 +-
 arch/x86/include/asm/asm.h        | 53 ++++++++------------
 arch/x86/include/asm/cpufeature.h | 82 ++++++++++++++++++-------------
 arch/x86/include/asm/jump_label.h | 72 +++++++--------------------
 arch/x86/kernel/macros.S          |  3 ++
 5 files changed, 89 insertions(+), 123 deletions(-)

2.17.1

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is also a minor cleanup for the exception table code.

Text size goes up a bit:

text data bss dec hex filename
18162555 10226288 2957312 31346155 1de4deb ./vmlinux before 18162879 10226256 2957312 31346447 1de4f0f ./vmlinux after (+292)

But this allows the inlining of functions such as nested_vmx_exit_reflected(), set_segment_reg(), __copy_xstate_to_user() which is a net benefit.

---
 arch/x86/include/asm/asm.h | 53 ++++++++++++++------------------------
 arch/x86/kernel/macros.S   |  1 +
 2 files changed, 21 insertions(+), 33 deletions(-)

index 6467757bb39f..21b086786404 100644
@@ -120,12 +120,25 @@
 /* Exception table entry */
 #ifdef __ASSEMBLY__
 # define _ASM_EXTABLE_HANDLE(from, to, handler)			\
-	.pushsection "__ex_table","a" ;				\
-	.balign 4 ;						\
-	.long (from) - . ;					\
-	.long (to) - . ;					\
-	.long (handler) - . ;					\
+	ASM_EXTABLE_HANDLE from to handler
+
+.macro ASM_EXTABLE_HANDLE from:req to:req handler:req
+	.pushsection "__ex_table","a"
+	.balign 4
+	.long (\from) - .
+	.long (\to) - .
+	.long (\handler) - .
 	.popsection
+.endm
+#else /* __ASSEMBLY__ */
+
+# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
+	"ASM_EXTABLE_HANDLE from=" #from " to=" #to		\
+	" handler=\"" #handler "\"\n\t"
+
+/* For C file, we already have NOKPROBE_SYMBOL macro */
+
+#endif /* __ASSEMBLY__ */
 
 # define _ASM_EXTABLE(from, to)					\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)

@@ -148,6 +161,7 @@
 	_ASM_PTR (entry);					\
 	.popsection
 
+#ifdef __ASSEMBLY__
 .macro ALIGN_DESTINATION
 	/* check for bad alignment of destination */
 	movl %edi,%ecx

@@ -171,34 +185,7 @@
 	_ASM_EXTABLE_UA(100b, 103b)
 	_ASM_EXTABLE_UA(101b, 103b)
 	.endm
-
-#else
-# define _EXPAND_EXTABLE_HANDLE(x) #x
-# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
-	" .pushsection \"__ex_table\",\"a\"\n"			\
-	" .balign 4\n"						\
-	" .long (" #from ") - .\n"				\
-	" .long (" #to ") - .\n"				\
-	" .long (" _EXPAND_EXTABLE_HANDLE(handler) ") - .\n"	\
-	" .popsection\n"
-
-# define _ASM_EXTABLE(from, to)					\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)
-
-# define _ASM_EXTABLE_UA(from, to)				\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_uaccess)
-
-# define _ASM_EXTABLE_FAULT(from, to)				\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
-
-# define _ASM_EXTABLE_EX(from, to)				\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
-
-# define _ASM_EXTABLE_REFCOUNT(from, to)			\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_refcount)
-
-/* For C file, we already have NOKPROBE_SYMBOL macro */
-#endif
+#endif /* __ASSEMBLY__ */
 
 #ifndef __ASSEMBLY__
 /*

index 71d8b716b111..7baa40d5bf16 100644
@@ -11,3 +11,4 @@
 #include <asm/alternative-asm.h>
 #include <asm/bug.h>
 #include <asm/paravirt.h>
+#include <asm/asm.h>

2.17.1

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is pretty pointless indirection in the static_cpu_has() case, but is worth it to improve overall inlining quality.

The patch slightly increases the kernel size:

text data bss dec hex filename
18162879 10226256 2957312 31346447 1de4f0f ./vmlinux before 18163528 10226300 2957312 31347140 1de51c4 ./vmlinux after (+693)

And enables the inlining of function such as free_ldt_pgtables().

---
 arch/x86/include/asm/cpufeature.h | 82 ++++++++++++++++++-------------
 arch/x86/kernel/macros.S          |  1 +
 2 files changed, 48 insertions(+), 35 deletions(-)

index aced6c9290d6..7d442722ef24 100644
@@ -2,10 +2,10 @@
 #ifndef _ASM_X86_CPUFEATURE_H
 #define _ASM_X86_CPUFEATURE_H
 
-#include <asm/processor.h>
-
-#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
 
+#include <asm/processor.h>
 #include <asm/asm.h>
 #include <linux/bitops.h>
 

@@ -161,37 +161,10 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
  */
 static __always_inline __pure bool _static_cpu_has(u16 bit)
 {
-	asm_volatile_goto("1: jmp 6f\n"
-		 "2:\n"
-		 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
-			 "((5f-4f) - (2b-1b)),0x90\n"
-		 "3:\n"
-		 ".section .altinstructions,\"a\"\n"
-		 " .long 1b - .\n"		/* src offset */
-		 " .long 4f - .\n"		/* repl offset */
-		 " .word %P[always]\n"		/* always replace */
-		 " .byte 3b - 1b\n"		/* src len */
-		 " .byte 5f - 4f\n"		/* repl len */
-		 " .byte 3b - 2b\n"		/* pad len */
-		 ".previous\n"
-		 ".section .altinstr_replacement,\"ax\"\n"
-		 "4: jmp %l[t_no]\n"
-		 "5:\n"
-		 ".previous\n"
-		 ".section .altinstructions,\"a\"\n"
-		 " .long 1b - .\n"		/* src offset */
-		 " .long 0\n"			/* no replacement */
-		 " .word %P[feature]\n"		/* feature bit */
-		 " .byte 3b - 1b\n"		/* src len */
-		 " .byte 0\n"			/* repl len */
-		 " .byte 0\n"			/* pad len */
-		 ".previous\n"
-		 ".section .altinstr_aux,\"ax\"\n"
-		 "6:\n"
-		 " testb %[bitnum],%[cap_byte]\n"
-		 " jnz %l[t_yes]\n"
-		 " jmp %l[t_no]\n"
-		 ".previous\n"
+	asm_volatile_goto("STATIC_CPU_HAS bitnum=%[bitnum] "
+			  "cap_byte=\"%[cap_byte]\" "
+			  "feature=%P[feature] t_yes=%l[t_yes] "
+			  "t_no=%l[t_no] always=%P[always]"
 		 : : [feature]  "i" (bit),
 		     [always]   "i" (X86_FEATURE_ALWAYS),
 		     [bitnum]   "i" (1 << (bit & 7)),

@@ -226,5 +199,44 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
 					boot_cpu_data.x86_model
 
-#endif /* defined(__KERNEL__) && !defined(__ASSEMBLY__) */
+#else /* __ASSEMBLY__ */
+
+.macro STATIC_CPU_HAS bitnum:req cap_byte:req feature:req t_yes:req t_no:req always:req
+1:
+	jmp 6f
+2:
+	.skip -(((5f-4f) - (2b-1b)) > 0) * ((5f-4f) - (2b-1b)),0x90
+3:
+	.section .altinstructions,"a"
+	.long 1b - .		/* src offset */
+	.long 4f - .		/* repl offset */
+	.word \always		/* always replace */
+	.byte 3b - 1b		/* src len */
+	.byte 5f - 4f		/* repl len */
+	.byte 3b - 2b		/* pad len */
+	.previous
+	.section .altinstr_replacement,"ax"
+4:
+	jmp \t_no
+5:
+	.previous
+	.section .altinstructions,"a"
+	.long 1b - .		/* src offset */
+	.long 0			/* no replacement */
+	.word \feature		/* feature bit */
+	.byte 3b - 1b		/* src len */
+	.byte 0			/* repl len */
+	.byte 0			/* pad len */
+	.previous
+	.section .altinstr_aux,"ax"
+6:
+	testb \bitnum,\cap_byte
+	jnz \t_yes
+	jmp \t_no
+	.previous
+.endm
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __KERNEL__ */
 #endif /* _ASM_X86_CPUFEATURE_H */

index 7baa40d5bf16..bf8b9c93e255 100644
@@ -12,3 +12,4 @@
 #include <asm/bug.h>
 #include <asm/paravirt.h>
 #include <asm/asm.h>
+#include <asm/cpufeature.h>

2.17.1

As described in:

77b0bf55bc67: ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")

GCC's inlining heuristics are broken with common asm() patterns used in kernel code, resulting in the effective disabling of inlining.

The workaround is to set an assembly macro and call it from the inline assembly block - which is also a minor cleanup for the jump-label code.

As a result the code size is slightly increased, but inlining decisions are better:

text data bss dec hex filename
18163528 10226300 2957312 31347140 1de51c4 ./vmlinux before 18163608 10227348 2957312 31348268 1de562c ./vmlinux after (+1128)

And functions such as intel_pstate_adjust_policy_max(), kvm_cpu_accept_dm_intr(), kvm_register_readl() are inlined.

---
 arch/x86/entry/calling.h          |  2 +-
 arch/x86/include/asm/jump_label.h | 72 ++++++++-----------------------
 arch/x86/kernel/macros.S          |  1 +
 3 files changed, 20 insertions(+), 55 deletions(-)

index 352e70cd33e8..708b46a54578 100644
@@ -338,7 +338,7 @@ For 32-bit we have the following conventions - kernel is built with
 .macro CALL_enter_from_user_mode
 #ifdef CONFIG_CONTEXT_TRACKING
 #ifdef HAVE_JUMP_LABEL
-	STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
+	STATIC_BRANCH_JMP l_yes=.Lafter_call_\@, key=context_tracking_enabled, branch=1
 #endif
 	call enter_from_user_mode
 .Lafter_call_\@:

index bf8b9c93e255..161c95059044 100644
@@ -13,3 +13,4 @@
 #include <asm/paravirt.h>
 #include <asm/asm.h>
 #include <asm/cpufeature.h>
+#include <asm/jump_label.h>

2.17.1

For the record, I summarized my analysis of the poor function inlining decisions in Linux in the following blog-post:

https://nadav.amit.zone/blog/linux-inline

Regards,
Nadav

So, I’m sorry for missing your comment about misrepresenting the problem.

Feel free to do whatever you want with the commit message (just fix the typo in “attemt"). As long as you don’t NAK the patches or send me to redo them, it’s fine. I just want to clarify few things for you to consider.

First, you are right that clang does not have this issue (I checked), but the behavior of gcc is clearly documented - once you know what to look for.

Second, I think the end result is not as ugly as you make it sound (and maybe not ugly at all). Using this patch-set, you can write big blocks of inlined assembly code without having those disgusting C macros. You can also share the same code between inline asm and asm files.

You can have a look, for example, on ALTERNATIVE which is defined both as assembly macro and C macro. Is the C macro readable? Is it easy to maintain two different version? I do have a patch that merges the two implementations together (which I still didn’t send, since I wait for the infra to be applied first), and I think makes much more sense.

Finally, note that it’s not as if the binary always becomes smaller. Overall, with the full patch-set it is slightly bigger. But still, that’s how it was supposed to be if gcc wasn’t doing things badly.

Thanks again,
Nadav

So what I cited was the changelog for the refcount patch, which according to your measurements reduced kernel image size.

For other patches where size grew I left that text intact.

And yes, in this particular case a slight increase in kernel size might actually be beneficial, as we get a lot more straight-line execution in exchange. So this is not a show-stopper property as long as the bloat isn't large.

Thanks,

Ingo

GCC considers the number of statements in inlined assembly blocks, according to new-lines and semicolons, as an indication to the cost of the block in time and space. This data is distorted by the kernel code, which puts information in alternative sections. As a result, the compiler may perform incorrect inlining and branch optimizations.

The solution is to set an assembly macro and call it from the inlined assembly block. As a result GCC considers the inline assembly block as a single instruction.

This patch handles the LOCK prefix, allowing more aggresive inlining.

text data bss dec hex filename
18140140 10225284 2957312 31322736 1ddf270 ./vmlinux before 18146889 10225380 2957312 31329581 1de0d2d ./vmlinux after (+6845)

Static text symbols:
Before: 40286
After: 40218 (-68)

---
 arch/x86/include/asm/alternative-asm.h | 20 ++++++++++++++------
 arch/x86/include/asm/alternative.h     | 11 ++---------
 arch/x86/kernel/macros.S               |  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)

index 31b627b43a8e..8e4ea39e55d0 100644
@@ -7,16 +7,24 @@
 #include <asm/asm.h>
 
 #ifdef CONFIG_SMP
-	.macro LOCK_PREFIX
-672:	lock
+.macro LOCK_PREFIX_HERE
 	.pushsection .smp_locks,"a"
 	.balign 4
-	.long 672b - .
+	.long 671f - .		# offset
 	.popsection
-	.endm
+671:
+.endm
+
+.macro LOCK_PREFIX insn:vararg
+	LOCK_PREFIX_HERE
+	lock \insn
+.endm
 #else
-	.macro LOCK_PREFIX
-	.endm
+.macro LOCK_PREFIX_HERE
+.endm
+
+.macro LOCK_PREFIX insn:vararg
+.endm
 #endif
 
 /*

index 4cd6a3b71824..d7faa16622d8 100644
@@ -31,15 +31,8 @@
  */
 
 #ifdef CONFIG_SMP
-#define LOCK_PREFIX_HERE \
-		".pushsection .smp_locks,\"a\"\n"	\
-		".balign 4\n"				\
-		".long 671f - .\n" /* offset */		\
-		".popsection\n"				\
-		"671:"
-
-#define LOCK_PREFIX LOCK_PREFIX_HERE "\n\tlock; "
-
+#define LOCK_PREFIX_HERE "LOCK_PREFIX_HERE\n\t"
+#define LOCK_PREFIX "LOCK_PREFIX "
 #else /* ! CONFIG_SMP */
 #define LOCK_PREFIX_HERE ""
 #define LOCK_PREFIX ""

index f1fe1d570365..852487a9fc56 100644
@@ -8,3 +8,4 @@
 
 #include <linux/compiler.h>
 #include <asm/refcount.h>
+#include <asm/alternative-asm.h>

2.17.1

GCC considers the number of statements in inlined assembly blocks, according to new-lines and semicolons, as an indication to the cost of the block in time and space. This data is distorted by the kernel code, which puts information in alternative sections. As a result, the compiler may perform incorrect inlining and branch optimizations.

The solution is to set an assembly macro and call it from the inlinedv assembly block. As a result GCC considers the inline assembly block as a single instruction.

This patch increases the kernel size:

text data bss dec hex filename
18146889 10225380 2957312 31329581 1de0d2d ./vmlinux before 18147336 10226688 2957312 31331336 1de1408 ./vmlinux after (+1755)

But enables more aggressive inlining (and probably branch decisions). The number of static text symbols in vmlinux is lower.

Static text symbols:
Before: 40218
After: 40053 (-165)

---
 arch/x86/include/asm/bug.h | 98 ++++++++++++++++++++++----------------
 arch/x86/kernel/macros.S   |  1 +
 include/asm-generic/bug.h  |  8 ++--
 3 files changed, 61 insertions(+), 46 deletions(-)

index 6804d6642767..5090035e6d16 100644
@@ -4,6 +4,8 @@
 
 #include <linux/stringify.h>
 
+#ifndef __ASSEMBLY__
+
 /*
  * Despite that some emulators terminate on UD2, we use it for WARN().
  *

@@ -20,53 +22,15 @@
 
 #define LEN_UD2		2
 
-#ifdef CONFIG_GENERIC_BUG
-
-#ifdef CONFIG_X86_32
-# define __BUG_REL(val)	".long " __stringify(val)
-#else
-# define __BUG_REL(val)	".long " __stringify(val) " - 2b"
-#endif
-
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-
-#define _BUG_FLAGS(ins, flags)						\
-do {									\
-	asm volatile("1:\t" ins "\n"					\
-		     ".pushsection __bug_table,\"aw\"\n"		\
-		     "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"	\
-		     "\t"  __BUG_REL(%c0) "\t# bug_entry::file\n"	\
-		     "\t.word %c1"        "\t# bug_entry::line\n"	\
-		     "\t.word %c2"        "\t# bug_entry::flags\n"	\
-		     "\t.org 2b+%c3\n"					\
-		     ".popsection"					\
-		     : : "i" (__FILE__), "i" (__LINE__),		\
-			 "i" (flags),					\
-			 "i" (sizeof(struct bug_entry)));		\
-} while (0)
-
-#else /* !CONFIG_DEBUG_BUGVERBOSE */
-
 #define _BUG_FLAGS(ins, flags)						\
 do {									\
-	asm volatile("1:\t" ins "\n"					\
-		     ".pushsection __bug_table,\"aw\"\n"		\
-		     "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"	\
-		     "\t.word %c0"        "\t# bug_entry::flags\n"	\
-		     "\t.org 2b+%c1\n"					\
-		     ".popsection"					\
-		     : : "i" (flags),					\
+	asm volatile("ASM_BUG ins=\"" ins "\" file=%c0 line=%c1 "	\
+		     "flags=%c2 size=%c3"				\
+		     : : "i" (__FILE__), "i" (__LINE__),                \
+			 "i" (flags),                                   \
 			 "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
-#endif /* CONFIG_DEBUG_BUGVERBOSE */
-
-#else
-
-#define _BUG_FLAGS(ins, flags)  asm volatile(ins)
-
-#endif /* CONFIG_GENERIC_BUG */
-
 #define HAVE_ARCH_BUG
 #define BUG()							\
 do {								\

@@ -82,4 +46,54 @@ do {								\
 
 #include <asm-generic/bug.h>
 
+#else /* __ASSEMBLY__ */
+
+#ifdef CONFIG_GENERIC_BUG
+
+#ifdef CONFIG_X86_32
+.macro __BUG_REL val:req
+	.long \val
+.endm
+#else
+.macro __BUG_REL val:req
+	.long \val - 2b
+.endm
+#endif
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+.macro ASM_BUG ins:req file:req line:req flags:req size:req
+1:	\ins
+	.pushsection __bug_table,"aw"
+2:	__BUG_REL val=1b	# bug_entry::bug_addr
+	__BUG_REL val=\file	# bug_entry::file
+	.word \line		# bug_entry::line
+	.word \flags		# bug_entry::flags
+	.org 2b+\size
+	.popsection
+.endm
+
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
+.macro ASM_BUG ins:req file:req line:req flags:req size:req
+1:	\ins
+	.pushsection __bug_table,"aw"
+2:	__BUG_REL val=1b	# bug_entry::bug_addr
+	.word \flags		# bug_entry::flags
+	.org 2b+\size
+	.popsection
+.endm
+
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
+#else /* CONFIG_GENERIC_BUG */
+
+.macro ASM_BUG ins:req file:req line:req flags:req size:req
+	\ins
+.endm
+
+#endif /* CONFIG_GENERIC_BUG */
+
+#endif /* __ASSEMBLY__ */
+
 #endif /* _ASM_X86_BUG_H */

index 852487a9fc56..66ccb8e823b1 100644
@@ -9,3 +9,4 @@
 #include <linux/compiler.h>
 #include <asm/refcount.h>
 #include <asm/alternative-asm.h>
+#include <asm/bug.h>

index 20561a60db9c..cdafa5edea49 100644
@@ -17,10 +17,8 @@
 #ifndef __ASSEMBLY__
 #include <linux/kernel.h>
 
-#ifdef CONFIG_BUG
-
-#ifdef CONFIG_GENERIC_BUG
 struct bug_entry {
+#ifdef CONFIG_GENERIC_BUG
 #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
 	unsigned long	bug_addr;
 #else

@@ -35,8 +33,10 @@ struct bug_entry {
 	unsigned short	line;
 #endif
 	unsigned short	flags;
-};
 #endif	/* CONFIG_GENERIC_BUG */
+};
+
+#ifdef CONFIG_BUG
 
 /*
  * Don't use BUG() or BUG_ON() unless there's really no way out; one

2.17.1

GCC considers the number of statements in inlined assembly blocks, according to new-lines and semicolons, as an indication to the cost of the block in time and space. This data is distorted by the kernel code, which puts information in alternative sections. As a result, the compiler may perform incorrect inlining and branch optimizations.

The solution is to set an assembly macro and call it from the inlined assembly block. As a result GCC considers the inline assembly block as a single instruction.

The effect of the patch is a more aggressive inlining, which also causes a size increase of kernel.

text data bss dec hex filename
18147336 10226688 2957312 31331336 1de1408 ./vmlinux before 18162555 10226288 2957312 31346155 1de4deb ./vmlinux after (+14819)

Static text symbols:
Before: 40053
After: 39942 (-111)

---
 arch/x86/include/asm/paravirt_types.h | 56 +++++++++++++--------------
 arch/x86/kernel/macros.S              |  1 +
 2 files changed, 28 insertions(+), 29 deletions(-)

index 4b75acc23b30..83ce282eed0a 100644
@@ -346,23 +346,11 @@ extern struct pv_lock_ops pv_lock_ops;
 #define paravirt_clobber(clobber)		\
 	[paravirt_clobber] "i" (clobber)
 
-/*
- * Generate some code, and mark it as patchable by the
- * apply_paravirt() alternate instruction patcher.
- */
-#define _paravirt_alt(insn_string, type, clobber)	\
-	"771:\n\t" insn_string "\n" "772:\n"		\
-	".pushsection .parainstructions,\"a\"\n"	\
-	_ASM_ALIGN "\n"					\
-	_ASM_PTR " 771b\n"				\
-	"  .byte " type "\n"				\
-	"  .byte 772b-771b\n"				\
-	"  .short " clobber "\n"			\
-	".popsection\n"
-
 /* Generate patchable code, with the default asm parameters. */
-#define paravirt_alt(insn_string)					\
-	_paravirt_alt(insn_string, "%c[paravirt_typenum]", "%c[paravirt_clobber]")
+#define paravirt_call							\
+	"PARAVIRT_CALL type=\"%c[paravirt_typenum]\""			\
+	" clobber=\"%c[paravirt_clobber]\""				\
+	" pv_opptr=\"%c[paravirt_opptr]\";"
 
 /* Simple instruction patching code. */
 #define NATIVE_LABEL(a,x,b) "\n\t.globl " a #x "_" #b "\n" a #x "_" #b ":\n\t"

@@ -390,16 +378,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
 
 int paravirt_disable_iospace(void);
 
-/*
- * This generates an indirect call based on the operation type number.
- * The type number, computed in PARAVIRT_PATCH, is derived from the
- * offset into the paravirt_patch_template structure, and can therefore be
- * freely converted back into a structure offset.
- */
-#define PARAVIRT_CALL					\
-	ANNOTATE_RETPOLINE_SAFE				\
-	"call *%c[paravirt_opptr];"
-
 /*
  * These macros are intended to wrap calls through one of the paravirt
  * ops structs, so that they can be later identified and patched at

@@ -537,7 +515,7 @@ int paravirt_disable_iospace(void);
 		/* since this condition will never hold */		\
 		if (sizeof(rettype) > sizeof(unsigned long)) {		\
 			asm volatile(pre				\
-				     paravirt_alt(PARAVIRT_CALL)	\
+				     paravirt_call			\
 				     post				\
 				     : call_clbr, ASM_CALL_CONSTRAINT	\
 				     : paravirt_type(op),		\

@@ -547,7 +525,7 @@ int paravirt_disable_iospace(void);
 			__ret = (rettype)((((u64)__edx) << 32) | __eax); \
 		} else {						\
 			asm volatile(pre				\
-				     paravirt_alt(PARAVIRT_CALL)	\
+				     paravirt_call			\
 				     post				\
 				     : call_clbr, ASM_CALL_CONSTRAINT	\
 				     : paravirt_type(op),		\

@@ -574,7 +552,7 @@ int paravirt_disable_iospace(void);
 		PVOP_VCALL_ARGS;					\
 		PVOP_TEST_NULL(op);					\
 		asm volatile(pre					\
-			     paravirt_alt(PARAVIRT_CALL)		\
+			     paravirt_call				\
 			     post					\
 			     : call_clbr, ASM_CALL_CONSTRAINT		\
 			     : paravirt_type(op),			\

@@ -694,6 +672,26 @@ struct paravirt_patch_site {
 extern struct paravirt_patch_site __parainstructions[],
 	__parainstructions_end[];
 
+#else	/* __ASSEMBLY__ */
+
+/*
+ * This generates an indirect call based on the operation type number.
+ * The type number, computed in PARAVIRT_PATCH, is derived from the
+ * offset into the paravirt_patch_template structure, and can therefore be
+ * freely converted back into a structure offset.
+ */
+.macro PARAVIRT_CALL type:req clobber:req pv_opptr:req
+771:	ANNOTATE_RETPOLINE_SAFE
+	call *\pv_opptr
+772:	.pushsection .parainstructions,"a"
+	_ASM_ALIGN
+	_ASM_PTR 771b
+	.byte \type
+	.byte 772b-771b
+	.short \clobber
+	.popsection
+.endm
+
 #endif	/* __ASSEMBLY__ */
 
 #endif	/* _ASM_X86_PARAVIRT_TYPES_H */

index 66ccb8e823b1..71d8b716b111 100644
@@ -10,3 +10,4 @@
 #include <asm/refcount.h>
 #include <asm/alternative-asm.h>
 #include <asm/bug.h>
+#include <asm/paravirt.h>

2.17.1

Use assembly macros for exception-tables and call them from inline assembly. This not only makes the code more readable and allows to avoid the duplicate implementation, but also improves compilation decision, specifically inline decisions which GCC base on the number of new lines in inline assembly.

text data bss dec hex filename
18162555 10226288 2957312 31346155 1de4deb ./vmlinux before 18162879 10226256 2957312 31346447 1de4f0f ./vmlinux after (+292)

This allows to inline functions such as nested_vmx_exit_reflected(), set_segment_reg(), __copy_xstate_to_user().

---
 arch/x86/include/asm/asm.h | 61 +++++++++++++++++---------------------
 arch/x86/kernel/macros.S   |  1 +
 2 files changed, 28 insertions(+), 34 deletions(-)

index 990770f9e76b..e47a2256c64e 100644
@@ -117,28 +117,44 @@
 # define CC_OUT(c) [_cc_ ## c] "=qm"
 #endif
 
-/* Exception table entry */
 #ifdef __ASSEMBLY__
 # define _ASM_EXTABLE_HANDLE(from, to, handler)			\
-	.pushsection "__ex_table","a" ;				\
-	.balign 4 ;						\
-	.long (from) - . ;					\
-	.long (to) - . ;					\
-	.long (handler) - . ;					\
-	.popsection
+	ASM_EXTABLE_HANDLE from to handler
+
+#else /* __ASSEMBLY__ */
+
+# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
+	"ASM_EXTABLE_HANDLE from=" #from " to=" #to		\
+	" handler=\"" #handler "\"\n\t"
+
+/* For C file, we already have NOKPROBE_SYMBOL macro */
+
+#endif /* __ASSEMBLY__ */
 
-# define _ASM_EXTABLE(from, to)					\
+#define _ASM_EXTABLE(from, to)					\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)
 
-# define _ASM_EXTABLE_FAULT(from, to)				\
+#define _ASM_EXTABLE_FAULT(from, to)				\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
 
-# define _ASM_EXTABLE_EX(from, to)				\
+#define _ASM_EXTABLE_EX(from, to)				\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
 
-# define _ASM_EXTABLE_REFCOUNT(from, to)			\
+#define _ASM_EXTABLE_REFCOUNT(from, to)				\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_refcount)
 
+/* Exception table entry */
+#ifdef __ASSEMBLY__
+
+.macro ASM_EXTABLE_HANDLE from:req to:req handler:req
+	.pushsection "__ex_table","a"
+	.balign 4
+	.long (\from) - .
+	.long (\to) - .
+	.long (\handler) - .
+	.popsection
+.endm
+
 # define _ASM_NOKPROBE(entry)					\
 	.pushsection "_kprobe_blacklist","aw" ;			\
 	_ASM_ALIGN ;						\

@@ -169,29 +185,6 @@
 	_ASM_EXTABLE(101b,103b)
 	.endm
 
-#else
-# define _EXPAND_EXTABLE_HANDLE(x) #x
-# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
-	" .pushsection \"__ex_table\",\"a\"\n"			\
-	" .balign 4\n"						\
-	" .long (" #from ") - .\n"				\
-	" .long (" #to ") - .\n"				\
-	" .long (" _EXPAND_EXTABLE_HANDLE(handler) ") - .\n"	\
-	" .popsection\n"
-
-# define _ASM_EXTABLE(from, to)					\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)
-
-# define _ASM_EXTABLE_FAULT(from, to)				\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
-
-# define _ASM_EXTABLE_EX(from, to)				\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
-
-# define _ASM_EXTABLE_REFCOUNT(from, to)			\
-	_ASM_EXTABLE_HANDLE(from, to, ex_handler_refcount)
-
-/* For C file, we already have NOKPROBE_SYMBOL macro */
 #endif
 
 #ifndef __ASSEMBLY__

index 71d8b716b111..7baa40d5bf16 100644
@@ -11,3 +11,4 @@
 #include <asm/alternative-asm.h>
 #include <asm/bug.h>
 #include <asm/paravirt.h>
+#include <asm/asm.h>

2.17.1

Use assembly macros for static_cpu_has() and call them from inline assembly. This not only makes the code more readable, but also improves compilation decision, specifically inline decisions which GCC base on the number of new lines in inline assembly.

The patch slightly increases the kernel size:

text data bss dec hex filename
18162879 10226256 2957312 31346447 1de4f0f ./vmlinux before 18163528 10226300 2957312 31347140 1de51c4 ./vmlinux after (+693)

And enables the inlining of function such as free_ldt_pgtables().

---
 arch/x86/include/asm/cpufeature.h | 82 ++++++++++++++++++-------------
 arch/x86/kernel/macros.S          |  1 +
 2 files changed, 48 insertions(+), 35 deletions(-)

index aced6c9290d6..7d442722ef24 100644
@@ -2,10 +2,10 @@
 #ifndef _ASM_X86_CPUFEATURE_H
 #define _ASM_X86_CPUFEATURE_H
 
-#include <asm/processor.h>
-
-#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
 
+#include <asm/processor.h>
 #include <asm/asm.h>
 #include <linux/bitops.h>
 

@@ -161,37 +161,10 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
  */
 static __always_inline __pure bool _static_cpu_has(u16 bit)
 {
-	asm_volatile_goto("1: jmp 6f\n"
-		 "2:\n"
-		 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
-			 "((5f-4f) - (2b-1b)),0x90\n"
-		 "3:\n"
-		 ".section .altinstructions,\"a\"\n"
-		 " .long 1b - .\n"		/* src offset */
-		 " .long 4f - .\n"		/* repl offset */
-		 " .word %P[always]\n"		/* always replace */
-		 " .byte 3b - 1b\n"		/* src len */
-		 " .byte 5f - 4f\n"		/* repl len */
-		 " .byte 3b - 2b\n"		/* pad len */
-		 ".previous\n"
-		 ".section .altinstr_replacement,\"ax\"\n"
-		 "4: jmp %l[t_no]\n"
-		 "5:\n"
-		 ".previous\n"
-		 ".section .altinstructions,\"a\"\n"
-		 " .long 1b - .\n"		/* src offset */
-		 " .long 0\n"			/* no replacement */
-		 " .word %P[feature]\n"		/* feature bit */
-		 " .byte 3b - 1b\n"		/* src len */
-		 " .byte 0\n"			/* repl len */
-		 " .byte 0\n"			/* pad len */
-		 ".previous\n"
-		 ".section .altinstr_aux,\"ax\"\n"
-		 "6:\n"
-		 " testb %[bitnum],%[cap_byte]\n"
-		 " jnz %l[t_yes]\n"
-		 " jmp %l[t_no]\n"
-		 ".previous\n"
+	asm_volatile_goto("STATIC_CPU_HAS bitnum=%[bitnum] "
+			  "cap_byte=\"%[cap_byte]\" "
+			  "feature=%P[feature] t_yes=%l[t_yes] "
+			  "t_no=%l[t_no] always=%P[always]"
 		 : : [feature]  "i" (bit),
 		     [always]   "i" (X86_FEATURE_ALWAYS),
 		     [bitnum]   "i" (1 << (bit & 7)),

@@ -226,5 +199,44 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
 					boot_cpu_data.x86_model
 
-#endif /* defined(__KERNEL__) && !defined(__ASSEMBLY__) */
+#else /* __ASSEMBLY__ */
+
+.macro STATIC_CPU_HAS bitnum:req cap_byte:req feature:req t_yes:req t_no:req always:req
+1:
+	jmp 6f
+2:
+	.skip -(((5f-4f) - (2b-1b)) > 0) * ((5f-4f) - (2b-1b)),0x90
+3:
+	.section .altinstructions,"a"
+	.long 1b - .		/* src offset */
+	.long 4f - .		/* repl offset */
+	.word \always		/* always replace */
+	.byte 3b - 1b		/* src len */
+	.byte 5f - 4f		/* repl len */
+	.byte 3b - 2b		/* pad len */
+	.previous
+	.section .altinstr_replacement,"ax"
+4:
+	jmp \t_no
+5:
+	.previous
+	.section .altinstructions,"a"
+	.long 1b - .		/* src offset */
+	.long 0			/* no replacement */
+	.word \feature		/* feature bit */
+	.byte 3b - 1b		/* src len */
+	.byte 0			/* repl len */
+	.byte 0			/* pad len */
+	.previous
+	.section .altinstr_aux,"ax"
+6:
+	testb \bitnum,\cap_byte
+	jnz \t_yes
+	jmp \t_no
+	.previous
+.endm
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __KERNEL__ */
 #endif /* _ASM_X86_CPUFEATURE_H */

index 7baa40d5bf16..bf8b9c93e255 100644
@@ -12,3 +12,4 @@
 #include <asm/bug.h>
 #include <asm/paravirt.h>
 #include <asm/asm.h>
+#include <asm/cpufeature.h>

2.17.1

Use assembly macros for jump-labels and call them from inline assembly. This not only makes the code more readable, but also improves compilation decision, specifically inline decisions which GCC base on the number of new lines in inline assembly.

As a result the code size is slightly increased.

text data bss dec hex filename
18163528 10226300 2957312 31347140 1de51c4 ./vmlinux before 18163608 10227348 2957312 31348268 1de562c ./vmlinux after (+1128)

And functions such as intel_pstate_adjust_policy_max(), kvm_cpu_accept_dm_intr(), kvm_register_readl() are inlined.

---
 arch/x86/entry/calling.h          |  2 +-
 arch/x86/include/asm/jump_label.h | 77 +++++++++++--------------------
 arch/x86/kernel/macros.S          |  1 +
 3 files changed, 30 insertions(+), 50 deletions(-)

index 352e70cd33e8..708b46a54578 100644
@@ -338,7 +338,7 @@ For 32-bit we have the following conventions - kernel is built with
 .macro CALL_enter_from_user_mode
 #ifdef CONFIG_CONTEXT_TRACKING
 #ifdef HAVE_JUMP_LABEL
-	STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
+	STATIC_BRANCH_JMP l_yes=.Lafter_call_\@, key=context_tracking_enabled, branch=1
 #endif
 	call enter_from_user_mode
 .Lafter_call_\@:

index bf8b9c93e255..161c95059044 100644
@@ -13,3 +13,4 @@
 #include <asm/paravirt.h>
 #include <asm/asm.h>
 #include <asm/cpufeature.h>
+#include <asm/jump_label.h>

2.17.1

Hi people,

this is an attempt to see whether gcc's inline asm heuristic when estimating inline asm statements' cost for better inlining can be improved.

AFAIU, the problematic arises when one ends up using a lot of inline asm statements in the kernel but due to the inline asm cost estimation heuristic which counts lines, I think, for example like in this here macro:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/cpufeature.h#n162

the resulting code ends up not inlining the functions themselves which use this macro. I.e., you see a CALL <function> instead of its body getting inlined directly.

Even though it should be because the actual instructions are only a couple in most cases and all those other directives end up in another section anyway.

The issue is explained below in the forwarded mail in a larger detail too.

Now, Richard suggested doing something like:

1) inline asm ("...")
2) asm ("..." : : : : <size-expr>)
3) asm ("...") __attribute__((asm_size(<size-expr>)));

with which user can tell gcc what the size of that inline asm statement is and thus allow for more precise cost estimation and in the end better inlining.

And FWIW 3) looks pretty straight-forward to me because attributes are pretty common anyways.

But I'm sure there are other options and I'm sure people will have better/different ideas so feel free to chime in.

Thx.

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

GCC already estimates the *size* of inline asm, and this is required *for correctness*. So any workaround that works against this will only end in tears.

Taking size as an estimate of cost is not very good. But in your motivating example you actually *do* care mostly about size, namely, for the inlining decisions.

So I guess the real issue is that the inline asm size estimate for x86 isn't very good (since it has to be pessimistic, and x86 insns can be huge)?

What would the semantics of this be?

This potentially conflicts with the syntax for asm goto.

Eww.

More precise *size* estimates, yes. And if the user lies he should not be surprised to get assembler errors, etc.

I don't like 2) either. But 1) looks interesting, depends what its semantics would be? "Don't count this insn's size for inlining decisions", maybe?

Another option is to just force inlining for those few functions where GCC currently makes an inlining decision you don't like. Or are there more than a few?

Segher

I didn't say it didn't - but the heuristic could use improving.

Well, the size thing could be just a "parameter" or "hint" of sorts, to tell gcc to inline the function X which is inlining the asm statement into the function Y which is calling function X. If you look at the patchset, it is moving everything to asm macros where gcc is apparently able to do better inlining.

Why?

Yes.

Another option would be if gcc parses the inline asm directly and does a more precise size estimation. Which is a lot more involved and complicated solution so I guess we wanna look at the simpler ones first.

:-)

Or simply "this asm statement has a size of 1" to mean, inline it everywhere. Which has the same caveats as above.

I'm afraid they're more than a few and this should work automatically, if possible.

Thx.

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

How? It is as sharp an estimate as can be *already*: number of insns times maximum size per insn.

If you get this wrong, conditional branches (and similar things, but conditional branches usually hit first, and hurt most) will stop working correctly, unless binutils uses relaxation for those on your architecture (most don't).

Yes, that will cause fewer problems I think: do not override size _at all_, but give a hint to the inliner.

Attributes are clumsy and clunky and kludgy.

It never is well-defined how attributes interact, and the more attributes we define and use, the more that matters.

Which is *impossible* to do. Inline assembler is free-form text.

"Has minimum length" then (size 1 cannot work on most archs).

Would counting *all* asms as having minimum length for inlining decisions work? Will that give bad side effects?

Or since this problem is quite specific to x86, maybe some target hook is wanted? Things work quite well elsewhere as-is, degrading that is not a good idea.

Segher