On Wed, Dec 19, 2018 at 9:44 PM Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > > * Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > > > This series reverts the in-kernel workarounds for inlining issues. > > > > The commit description of 77b0bf55bc67 mentioned > > "We also hope that GCC will eventually get fixed,..." > > > > Now, GCC provides a solution. > > > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html > > explains the new "asm inline" syntax. > > > > The performance issue will be eventually solved. > > > > [About Code cleanups] > > > > I know Nadam Amit is opposed to the full revert. > > He also claims his motivation for macrofying was not only > > performance, but also cleanups. > > > > IIUC, the criticism addresses the code duplication between C and ASM. > > > > If so, I'd like to suggest a different approach for cleanups. > > Please see the last 3 patches. > > IMHO, preprocessor approach is more straight-forward, and readable. > > Basically, this idea should work because it is what we already do for > > __ASM_FORM() etc. > > > > [Quick Test of "asm inline" of GCC 9] > > > > If you want to try "asm inline" feature, the patch is available: > > https://lore.kernel.org/patchwork/patch/1024590/ > > > > The number of symbols for arch/x86/configs/x86_64_defconfig: > > > > nr_symbols > > [1] v4.20-rc7 : 96502 > > [2] [1]+full revert : 96705 (+203) > > [3] [2]+"asm inline": 96568 (-137) > > > > [3]: apply my patch, then replace "asm" -> "asm_inline" > > for _BUG_FLAGS(), refcount_add(), refcount_inc(), refcount_dec(), > > annotate_reachable(), annotate_unreachable() > > > > > > Changes in v3: > > - Split into per-commit revert (per Nadav Amit) > > - Add some cleanups with preprocessor approach > > > > Changes in v2: > > - Revive clean-ups made by 5bdcd510c2ac (per Peter Zijlstra) > > - Fix commit quoting style (per Peter Zijlstra) > > > > Masahiro Yamada (12): > > Revert "x86/jump-labels: Macrofy inline assembly code to work around > > GCC inlining bugs" > > Revert "x86/cpufeature: Macrofy inline assembly code to work around > > GCC inlining bugs" > > Revert "x86/extable: Macrofy inline assembly code to work around GCC > > inlining bugs" > > Revert "x86/paravirt: Work around GCC inlining bugs when compiling > > paravirt ops" > > Revert "x86/bug: Macrofy the BUG table section handling, to work > > around GCC inlining bugs" > > Revert "x86/alternatives: Macrofy lock prefixes to work around GCC > > inlining bugs" > > Revert "x86/refcount: Work around GCC inlining bug" > > Revert "x86/objtool: Use asm macros to work around GCC inlining bugs" > > Revert "kbuild/Makefile: Prepare for using macros in inline assembly > > code to work around asm() related GCC inlining bugs" > > linux/linkage: add ASM() macro to reduce duplication between C/ASM > > code > > x86/alternatives: consolidate LOCK_PREFIX macro > > x86/asm: consolidate ASM_EXTABLE_* macros > > > > Makefile | 9 +-- > > arch/x86/Makefile | 7 --- > > arch/x86/include/asm/alternative-asm.h | 22 +------ > > arch/x86/include/asm/alternative-common.h | 47 +++++++++++++++ > > arch/x86/include/asm/alternative.h | 30 +--------- > > arch/x86/include/asm/asm.h | 46 +++++---------- > > arch/x86/include/asm/bug.h | 98 +++++++++++++------------------ > > arch/x86/include/asm/cpufeature.h | 82 +++++++++++--------------- > > arch/x86/include/asm/jump_label.h | 22 +++++-- > > arch/x86/include/asm/paravirt_types.h | 56 +++++++++--------- > > arch/x86/include/asm/refcount.h | 81 +++++++++++-------------- > > arch/x86/kernel/macros.S | 16 ----- > > include/asm-generic/bug.h | 8 +-- > > include/linux/compiler.h | 56 ++++-------------- > > include/linux/linkage.h | 8 +++ > > scripts/Kbuild.include | 4 +- > > scripts/mod/Makefile | 2 - > > 17 files changed, 249 insertions(+), 345 deletions(-) > > create mode 100644 arch/x86/include/asm/alternative-common.h > > delete mode 100644 arch/x86/kernel/macros.S > > I absolutely agree that this needs to be resolved in v4.20. > > So I did the 1-9 reverts manually myself as well, because I think the > first commit should be reverted fully to get as close to the starting > point as possible (we are late in the cycle) - and came to the attached > interdiff between your series and mine. > > Does this approach look OK to you, or did I miss something? It looks OK to me. I thought the diff was a good cleanup part, but we can deal with it later on, so I do not mind it. Thanks! > Thanks, > > Ingo > > =============> > > entry/calling.h | 2 - > include/asm/jump_label.h | 50 ++++++++++++++++++++++++++++++++++------------- > 2 files changed, 38 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h > index 25e5a6bda8c3..20d0885b00fb 100644 > --- a/arch/x86/entry/calling.h > +++ b/arch/x86/entry/calling.h > @@ -352,7 +352,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_BRANCH_JMP l_yes=.Lafter_call_\@, key=context_tracking_enabled, branch=1 > + STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0 > #endif > call enter_from_user_mode > .Lafter_call_\@: > diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h > index cf88ebf9a4ca..21efc9d07ed9 100644 > --- a/arch/x86/include/asm/jump_label.h > +++ b/arch/x86/include/asm/jump_label.h > @@ -2,6 +2,19 @@ > #ifndef _ASM_X86_JUMP_LABEL_H > #define _ASM_X86_JUMP_LABEL_H > > +#ifndef HAVE_JUMP_LABEL > +/* > + * For better or for worse, if jump labels (the gcc extension) are missing, > + * then the entire static branch patching infrastructure is compiled out. > + * If that happens, the code in here will malfunction. Raise a compiler > + * error instead. > + * > + * In theory, jump labels and the static branch patching infrastructure > + * could be decoupled to fix this. > + */ > +#error asm/jump_label.h included on a non-jump-label kernel > +#endif > + > #define JUMP_LABEL_NOP_SIZE 5 > > #ifdef CONFIG_X86_64 > @@ -53,26 +66,37 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool > > #else /* __ASSEMBLY__ */ > > -.macro STATIC_BRANCH_NOP l_yes:req key:req branch:req > -.Lstatic_branch_nop_\@: > - .byte STATIC_KEY_INIT_NOP > -.Lstatic_branch_no_after_\@: > +.macro STATIC_JUMP_IF_TRUE target, key, def > +.Lstatic_jump_\@: > + .if \def > + /* Equivalent to "jmp.d32 \target" */ > + .byte 0xe9 > + .long \target - .Lstatic_jump_after_\@ > +.Lstatic_jump_after_\@: > + .else > + .byte STATIC_KEY_INIT_NOP > + .endif > .pushsection __jump_table, "aw" > _ASM_ALIGN > - .long .Lstatic_branch_nop_\@ - ., \l_yes - . > - _ASM_PTR \key + \branch - . > + .long .Lstatic_jump_\@ - ., \target - . > + _ASM_PTR \key - . > .popsection > .endm > > -.macro STATIC_BRANCH_JMP l_yes:req key:req branch:req > -.Lstatic_branch_jmp_\@: > - .byte 0xe9 > - .long \l_yes - .Lstatic_branch_jmp_after_\@ > -.Lstatic_branch_jmp_after_\@: > +.macro STATIC_JUMP_IF_FALSE target, key, def > +.Lstatic_jump_\@: > + .if \def > + .byte STATIC_KEY_INIT_NOP > + .else > + /* Equivalent to "jmp.d32 \target" */ > + .byte 0xe9 > + .long \target - .Lstatic_jump_after_\@ > +.Lstatic_jump_after_\@: > + .endif > .pushsection __jump_table, "aw" > _ASM_ALIGN > - .long .Lstatic_branch_jmp_\@ - ., \l_yes - . > - _ASM_PTR \key + \branch - . > + .long .Lstatic_jump_\@ - ., \target - . > + _ASM_PTR \key + 1 - . > .popsection > .endm > > -- Best Regards Masahiro Yamada
